Merge pull request #2492 from realshuting/bugfixes

bugfixes - dynamic webhooks, e2e failure

pkg/generate/generate.go

@@ -5,11 +5,12 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"github.com/kyverno/kyverno/pkg/engine/response"
 	"reflect"
 	"strings"
 	"time"
 
+	"github.com/kyverno/kyverno/pkg/engine/response"
+
 	"github.com/go-logr/logr"
 	kyverno "github.com/kyverno/kyverno/pkg/api/kyverno/v1"
 	pkgcommon "github.com/kyverno/kyverno/pkg/common"
@@ -34,7 +35,7 @@ func (c *Controller) processGR(gr *kyverno.GenerateRequest) error {
 	var genResources []kyverno.ResourceSpec
 
 	// 1 - Check if the resource exists
-	resource, err = getResource(c.client, gr.Spec.Resource)
+	resource, err = getResource(c.client, gr.Spec.Resource, c.log)
 	if err != nil {
 		// Don't update status
 		logger.V(3).Info("resource does not exist or is pending creation, re-queueing", "details", err.Error())

pkg/generate/resource.go

@@ -1,23 +1,42 @@
 package generate
 
 import (
+	"time"
+
+	logr "github.com/go-logr/logr"
 	kyverno "github.com/kyverno/kyverno/pkg/api/kyverno/v1"
+	"github.com/kyverno/kyverno/pkg/common"
 	dclient "github.com/kyverno/kyverno/pkg/dclient"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
-func getResource(client *dclient.Client, resourceSpec kyverno.ResourceSpec) (*unstructured.Unstructured, error) {
-	if resourceSpec.Kind == "Namespace" {
-		resourceSpec.Namespace = ""
+func getResource(client *dclient.Client, resourceSpec kyverno.ResourceSpec, log logr.Logger) (*unstructured.Unstructured, error) {
+
+	get := func() (*unstructured.Unstructured, error) {
+		if resourceSpec.Kind == "Namespace" {
+			resourceSpec.Namespace = ""
+		}
+		resource, err := client.GetResource(resourceSpec.APIVersion, resourceSpec.Kind, resourceSpec.Namespace, resourceSpec.Name)
+		if err != nil {
+			return nil, err
+		}
+
+		if resource.GetDeletionTimestamp() != nil {
+			return nil, nil
+		}
+
+		return resource, nil
 	}
-	resource, err := client.GetResource(resourceSpec.APIVersion, resourceSpec.Kind, resourceSpec.Namespace, resourceSpec.Name)
-	if err != nil {
+
+	retry := func() error {
+		_, err := get()
+		return err
+	}
+
+	f := common.RetryFunc(time.Second, 30*time.Second, retry, log.WithName("getResource"))
+	if err := f(); err != nil {
 		return nil, err
 	}
 
-	if resource.GetDeletionTimestamp() != nil {
-		return nil, nil
-	}
-
-	return resource, nil
+	return get()
 }

pkg/webhookconfig/configmanager.go

@@ -373,8 +373,10 @@ func (m *webhookConfigManager) reconcileWebhook(namespace, name string) error {
 		return err
 	}
 
+	ready := true
 	if err := m.updateWebhookConfig(webhooks); err != nil {
-		return errors.Wrapf(err, "failed to update webhook configurations for policy %s/%s", namespace, name)
+		ready = false
+		logger.Error(err, "failed to update webhook configurations for policy")
 	}
 
 	// DELETION of the policy
@@ -382,11 +384,13 @@ func (m *webhookConfigManager) reconcileWebhook(namespace, name string) error {
 		return nil
 	}
 
-	if err := m.updateStatus(policy); err != nil {
+	if err := m.updateStatus(policy, ready); err != nil {
 		return errors.Wrapf(err, "failed to update policy status %s/%s", namespace, name)
 	}
 
-	logger.Info("policy is ready to serve admission requests")
+	if ready {
+		logger.Info("policy is ready to serve admission requests")
+	}
 	return nil
 }
 
@@ -469,17 +473,17 @@ func (m *webhookConfigManager) buildWebhooks(namespace string) (res []*webhook,
 	for _, p := range policies {
 		if p.HasValidate() || p.HasGenerate() {
 			if p.Spec.FailurePolicy != nil && *p.Spec.FailurePolicy == kyverno.Ignore {
-				m.mergeWebhook(validateIgnore, p)
+				m.mergeWebhook(validateIgnore, p, true)
 			} else {
-				m.mergeWebhook(validateFail, p)
+				m.mergeWebhook(validateFail, p, true)
 			}
 		}
 
 		if p.HasMutate() || p.HasGenerate() {
 			if p.Spec.FailurePolicy != nil && *p.Spec.FailurePolicy == kyverno.Ignore {
-				m.mergeWebhook(mutateIgnore, p)
+				m.mergeWebhook(mutateIgnore, p, false)
 			} else {
-				m.mergeWebhook(mutateFail, p)
+				m.mergeWebhook(mutateFail, p, false)
 			}
 		}
 	}
@@ -635,9 +639,9 @@ func (m *webhookConfigManager) compareAndUpdateWebhook(webhookKind, webhookName
 	return nil
 }
 
-func (m *webhookConfigManager) updateStatus(policy *kyverno.ClusterPolicy) error {
+func (m *webhookConfigManager) updateStatus(policy *kyverno.ClusterPolicy, status bool) error {
 	policyCopy := policy.DeepCopy()
-	policyCopy.Status.Ready = true
+	policyCopy.Status.Ready = status
 	if policy.GetNamespace() == "" {
 		_, err := m.kyvernoClient.KyvernoV1().ClusterPolicies().UpdateStatus(context.TODO(), policyCopy, v1.UpdateOptions{})
 		return err
@@ -648,12 +652,19 @@ func (m *webhookConfigManager) updateStatus(policy *kyverno.ClusterPolicy) error
 }
 
 // mergeWebhook merges the matching kinds of the policy to webhook.rule
-func (m *webhookConfigManager) mergeWebhook(dst *webhook, policy *kyverno.ClusterPolicy) {
+func (m *webhookConfigManager) mergeWebhook(dst *webhook, policy *kyverno.ClusterPolicy, updateValidate bool) {
 	matchedGVK := make([]string, 0)
 	for _, rule := range policy.Spec.Rules {
-		matchedGVK = append(matchedGVK, rule.MatchKinds()...)
+		// matching kinds in generate policies need to be added to both webhook
 		if rule.HasGenerate() {
+			matchedGVK = append(matchedGVK, rule.MatchKinds()...)
 			matchedGVK = append(matchedGVK, rule.Generation.ResourceSpec.Kind)
+			continue
+		}
+
+		if (updateValidate && rule.HasValidate()) ||
+			(!updateValidate && rule.HasMutate()) {
+			matchedGVK = append(matchedGVK, rule.MatchKinds()...)
 		}
 	}
 

test/e2e/generate/generate_test.go

@@ -1102,8 +1102,15 @@ func Test_Generate_Policy_Deletion_for_Clone(t *testing.T) {
 
 		// ======= Check Generated Resources =======
 		By(fmt.Sprintf("Checking the generated resource (Configmap) in namespace : %s", tests.ResourceNamespace))
-		_, err = e2eClient.GetNamespacedResource(cmGVR, tests.ResourceNamespace, tests.ConfigMapName)
+		err = e2e.GetWithRetry(1*time.Second, 15, func() error {
+			_, err := e2eClient.GetNamespacedResource(cmGVR, tests.ResourceNamespace, tests.ConfigMapName)
+			if err != nil {
+				return err
+			}
+			return nil
+		})
 		Expect(err).NotTo(HaveOccurred())
+
 		// ===========================================
 
 		// test: the generated resource is not updated if the source resource is updated