From 1936d86623445538d6f0b6561a9ee004d547bb10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Charles-Edouard=20Br=C3=A9t=C3=A9ch=C3=A9?= Date: Thu, 19 May 2022 18:06:56 +0200 Subject: [PATCH] fix: move ur controller filtering in reconciler (#3964) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: move ur controller filtering in reconciler Signed-off-by: Charles-Edouard Brétéché * fix: mark ur retry on conflict Signed-off-by: Charles-Edouard Brétéché * fix: test data Signed-off-by: Charles-Edouard Brétéché * fix: add filter back in update ur handler Signed-off-by: Charles-Edouard Brétéché * fix: added some logs about attempts and increased backoff Signed-off-by: Charles-Edouard Brétéché * fix: reconciliation logic Signed-off-by: Charles-Edouard Brétéché * fix: Test_Generate_Synchronize_Flag Signed-off-by: Charles-Edouard Brétéché * fix: small nits Signed-off-by: Charles-Edouard Brétéché --- cmd/initContainer/main.go | 9 +- cmd/kyverno/main.go | 1 - pkg/background/common/status.go | 95 ++----- pkg/background/common/util.go | 68 ++++- pkg/background/generate/generate.go | 17 +- pkg/background/log.go | 5 + pkg/background/mutate/mutate.go | 13 +- pkg/background/request_process.go | 74 ------ pkg/background/update_request_controller.go | 263 ++++++++++---------- pkg/policy/policy_controller.go | 32 ++- pkg/policy/updaterequest.go | 10 +- pkg/webhooks/resource/generation.go | 42 +--- pkg/webhooks/updaterequest/generator.go | 22 +- test/e2e/generate/resources.go | 3 + 14 files changed, 284 insertions(+), 370 deletions(-) create mode 100644 pkg/background/log.go delete mode 100644 pkg/background/request_process.go diff --git a/cmd/initContainer/main.go b/cmd/initContainer/main.go index 9d70592fd0..e3fe385de4 100644 --- a/cmd/initContainer/main.go +++ b/cmd/initContainer/main.go @@ -470,7 +470,7 @@ func convertGR(pclient kyvernoclient.Interface) error { }, } - new, err := pclient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).Create(context.TODO(), ur, metav1.CreateOptions{}) + _, err := pclient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).Create(context.TODO(), ur, metav1.CreateOptions{}) if err != nil { logger.Info("failed to create UpdateRequest", "GR namespace", gr.GetNamespace(), "GR name", gr.GetName(), "err", err.Error()) errors = append(errors, err) @@ -479,13 +479,6 @@ func convertGR(pclient kyvernoclient.Interface) error { logger.Info("successfully created UpdateRequest", "GR namespace", gr.GetNamespace(), "GR name", gr.GetName()) } - new.Status.State = kyvernov1beta1.Pending - if _, err := pclient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).UpdateStatus(context.TODO(), new, metav1.UpdateOptions{}); err != nil { - logger.Error(err, "failed to set UpdateRequest state to Pending") - errors = append(errors, err) - continue - } - if err := pclient.KyvernoV1().GenerateRequests(config.KyvernoNamespace()).Delete(context.TODO(), gr.GetName(), metav1.DeleteOptions{}); err != nil { errors = append(errors, err) logger.Error(err, "failed to delete GR") diff --git a/cmd/kyverno/main.go b/cmd/kyverno/main.go index f748e8d81d..ebe2af8e75 100644 --- a/cmd/kyverno/main.go +++ b/cmd/kyverno/main.go @@ -293,7 +293,6 @@ func main() { kyvernoInformer.Kyverno().V1beta1().UpdateRequests(), eventGenerator, kubeInformer.Core().V1().Namespaces(), - log.Log.WithName("BackgroundController"), configuration, ) diff --git a/pkg/background/common/status.go b/pkg/background/common/status.go index 9d1852e5bc..c96a79c9c5 100644 --- a/pkg/background/common/status.go +++ b/pkg/background/common/status.go @@ -4,93 +4,40 @@ import ( kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1" kyvernov1beta1 "github.com/kyverno/kyverno/api/kyverno/v1beta1" kyvernoclient "github.com/kyverno/kyverno/pkg/client/clientset/versioned" - jsonutils "github.com/kyverno/kyverno/pkg/utils/json" - "k8s.io/apimachinery/pkg/api/errors" - "sigs.k8s.io/controller-runtime/pkg/log" + kyvernov1beta1listers "github.com/kyverno/kyverno/pkg/client/listers/kyverno/v1beta1" ) // StatusControlInterface provides interface to update status subresource type StatusControlInterface interface { - Failed(ur kyvernov1beta1.UpdateRequest, message string, genResources []kyvernov1.ResourceSpec) error - Success(ur kyvernov1beta1.UpdateRequest, genResources []kyvernov1.ResourceSpec) error - Skip(ur kyvernov1beta1.UpdateRequest, genResources []kyvernov1.ResourceSpec) error + Failed(name string, message string, genResources []kyvernov1.ResourceSpec) (*kyvernov1beta1.UpdateRequest, error) + Success(name string, genResources []kyvernov1.ResourceSpec) (*kyvernov1beta1.UpdateRequest, error) + Skip(name string, genResources []kyvernov1.ResourceSpec) (*kyvernov1beta1.UpdateRequest, error) } -// StatusControl is default implementaation of GRStatusControlInterface -type StatusControl struct { - Client kyvernoclient.Interface +// statusControl is default implementaation of GRStatusControlInterface +type statusControl struct { + client kyvernoclient.Interface + urLister kyvernov1beta1listers.UpdateRequestNamespaceLister +} + +func NewStatusControl(client kyvernoclient.Interface, urLister kyvernov1beta1listers.UpdateRequestNamespaceLister) StatusControlInterface { + return &statusControl{ + client: client, + urLister: urLister, + } } // Failed sets ur status.state to failed with message -func (sc StatusControl) Failed(ur kyvernov1beta1.UpdateRequest, message string, genResources []kyvernov1.ResourceSpec) error { - genR := &kyvernov1beta1.UpdateRequestStatus{ - State: kyvernov1beta1.Failed, - Message: message, - } - if genResources != nil { - genR.GeneratedResources = genResources - } - - patch := jsonutils.NewPatch( - "/status", - "replace", - genR, - ) - _, err := PatchUpdateRequest(&ur, patch, sc.Client, "status") - if err != nil && !errors.IsNotFound(err) { - log.Log.Error(err, "failed to patch update request status", "name", ur.Name) - return err - } - log.Log.V(3).Info("updated update request status", "name", ur.Name, "status", string(kyvernov1.Failed)) - return nil +func (sc *statusControl) Failed(name, message string, genResources []kyvernov1.ResourceSpec) (*kyvernov1beta1.UpdateRequest, error) { + return UpdateStatus(sc.client, sc.urLister, name, kyvernov1beta1.Failed, message, genResources) } // Success sets the ur status.state to completed and clears message -func (sc StatusControl) Success(ur kyvernov1beta1.UpdateRequest, genResources []kyvernov1.ResourceSpec) error { - genR := &kyvernov1beta1.UpdateRequestStatus{ - State: kyvernov1beta1.Completed, - Message: "", - } - - if genResources != nil { - genR.GeneratedResources = genResources - } - - patch := jsonutils.NewPatch( - "/status", - "replace", - genR, - ) - _, err := PatchUpdateRequest(&ur, patch, sc.Client, "status") - if err != nil && !errors.IsNotFound(err) { - log.Log.Error(err, "failed to patch update request status", "name", ur.Name) - return err - } - log.Log.V(3).Info("updated update request status", "name", ur.Name, "status", string(kyvernov1beta1.Completed)) - return nil +func (sc *statusControl) Success(name string, genResources []kyvernov1.ResourceSpec) (*kyvernov1beta1.UpdateRequest, error) { + return UpdateStatus(sc.client, sc.urLister, name, kyvernov1beta1.Completed, "", genResources) } // Success sets the ur status.state to completed and clears message -func (sc StatusControl) Skip(ur kyvernov1beta1.UpdateRequest, genResources []kyvernov1.ResourceSpec) error { - genR := &kyvernov1beta1.UpdateRequestStatus{ - State: kyvernov1beta1.Skip, - Message: "", - } - - if genResources != nil { - genR.GeneratedResources = genResources - } - - patch := jsonutils.NewPatch( - "/status", - "replace", - genR, - ) - _, err := PatchUpdateRequest(&ur, patch, sc.Client, "status") - if err != nil && !errors.IsNotFound(err) { - log.Log.Error(err, "failed to update UR status", "name", ur.Name) - return err - } - log.Log.V(3).Info("updated UR status", "name", ur.Name, "status", string(kyvernov1.Skip)) - return nil +func (sc *statusControl) Skip(name string, genResources []kyvernov1.ResourceSpec) (*kyvernov1beta1.UpdateRequest, error) { + return UpdateStatus(sc.client, sc.urLister, name, kyvernov1beta1.Skip, "", genResources) } diff --git a/pkg/background/common/util.go b/pkg/background/common/util.go index 2874e5d418..133d677835 100644 --- a/pkg/background/common/util.go +++ b/pkg/background/common/util.go @@ -4,31 +4,73 @@ import ( "context" "time" + kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1" kyvernov1beta1 "github.com/kyverno/kyverno/api/kyverno/v1beta1" kyvernoclient "github.com/kyverno/kyverno/pkg/client/clientset/versioned" + kyvernov1beta1listers "github.com/kyverno/kyverno/pkg/client/listers/kyverno/v1beta1" "github.com/kyverno/kyverno/pkg/config" - jsonutils "github.com/kyverno/kyverno/pkg/utils/json" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/util/retry" + "sigs.k8s.io/controller-runtime/pkg/log" ) var DefaultRetry = wait.Backoff{ - Steps: 5, - Duration: 30 * time.Millisecond, + Steps: 15, + Duration: 100 * time.Millisecond, Factor: 1.0, Jitter: 0.1, } -// PatchUpdateRequest patches a update request object -func PatchUpdateRequest(ur *kyvernov1beta1.UpdateRequest, patch jsonutils.Patch, client kyvernoclient.Interface, subresources ...string) (*kyvernov1beta1.UpdateRequest, error) { - data, err := patch.ToPatchBytes() - if nil != err { - return ur, err - } - newUR, err := client.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).Patch(context.TODO(), ur.Name, types.JSONPatchType, data, metav1.PatchOptions{}, subresources...) +func Update(client kyvernoclient.Interface, urLister kyvernov1beta1listers.UpdateRequestNamespaceLister, name string, mutator func(*kyvernov1beta1.UpdateRequest)) (*kyvernov1beta1.UpdateRequest, error) { + var ur *kyvernov1beta1.UpdateRequest + err := retry.RetryOnConflict(DefaultRetry, func() error { + ur, err := urLister.Get(name) + if err != nil { + log.Log.Error(err, "[ATTEMPT] failed to fetch update request", "name", name) + return err + } + ur = ur.DeepCopy() + mutator(ur) + _, err = client.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).Update(context.TODO(), ur, metav1.UpdateOptions{}) + if err != nil { + log.Log.Error(err, "[ATTEMPT] failed to update update request", "name", name) + } + return err + }) if err != nil { - return ur, err + log.Log.Error(err, "failed to update update request", "name", name) + } else { + log.Log.V(3).Info("updated update request", "name", name, "status") } - return newUR, nil + return ur, err +} + +func UpdateStatus(client kyvernoclient.Interface, urLister kyvernov1beta1listers.UpdateRequestNamespaceLister, name string, state kyvernov1beta1.UpdateRequestState, message string, genResources []kyvernov1.ResourceSpec) (*kyvernov1beta1.UpdateRequest, error) { + var ur *kyvernov1beta1.UpdateRequest + err := retry.RetryOnConflict(DefaultRetry, func() error { + ur, err := urLister.Get(name) + if err != nil { + log.Log.Error(err, "[ATTEMPT] failed to fetch update request", "name", name) + return err + } + ur = ur.DeepCopy() + ur.Status.State = state + ur.Status.Message = message + if genResources != nil { + ur.Status.GeneratedResources = genResources + } + _, err = client.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).UpdateStatus(context.TODO(), ur, metav1.UpdateOptions{}) + if err != nil { + log.Log.Error(err, "[ATTEMPT] failed to update update request status", "name", name) + return err + } + return err + }) + if err != nil { + log.Log.Error(err, "failed to update update request status", "name", name) + } else { + log.Log.V(3).Info("updated update request status", "name", name, "status", string(state)) + } + return ur, err } diff --git a/pkg/background/generate/generate.go b/pkg/background/generate/generate.go index 649c5abea5..cd68aabc4a 100644 --- a/pkg/background/generate/generate.go +++ b/pkg/background/generate/generate.go @@ -89,7 +89,7 @@ func NewGenerateController( urLister: urLister, } - c.statusControl = common.StatusControl{Client: kyvernoClient} + c.statusControl = common.NewStatusControl(kyvernoClient, urLister) c.nsLister = nsLister return &c, nil @@ -299,12 +299,19 @@ func (c *GenerateController) getPolicySpec(ur kyvernov1beta1.UpdateRequest) (kyv func updateStatus(statusControl common.StatusControlInterface, ur kyvernov1beta1.UpdateRequest, err error, genResources []kyvernov1.ResourceSpec, precreatedResource bool) error { if err != nil { - return statusControl.Failed(ur, err.Error(), genResources) + if _, err := statusControl.Failed(ur.GetName(), err.Error(), genResources); err != nil { + return err + } } else if precreatedResource { - return statusControl.Skip(ur, genResources) + if _, err := statusControl.Skip(ur.GetName(), genResources); err != nil { + return err + } + } else { + if _, err := statusControl.Success(ur.GetName(), genResources); err != nil { + return err + } } - - return statusControl.Success(ur, genResources) + return nil } func (c *GenerateController) applyGeneratePolicy(log logr.Logger, policyContext *engine.PolicyContext, ur kyvernov1beta1.UpdateRequest, applicableRules []string) (genResources []kyvernov1.ResourceSpec, processExisting bool, err error) { diff --git a/pkg/background/log.go b/pkg/background/log.go new file mode 100644 index 0000000000..c8d0aa7c8e --- /dev/null +++ b/pkg/background/log.go @@ -0,0 +1,5 @@ +package background + +import "sigs.k8s.io/controller-runtime/pkg/log" + +var logger = log.Log.WithName("background") diff --git a/pkg/background/mutate/mutate.go b/pkg/background/mutate/mutate.go index 732f4029e5..03035f61cd 100644 --- a/pkg/background/mutate/mutate.go +++ b/pkg/background/mutate/mutate.go @@ -73,7 +73,7 @@ func NewMutateExistingController( Config: dynamicConfig, } - c.statusControl = common.StatusControl{Client: kyvernoClient} + c.statusControl = common.NewStatusControl(kyvernoClient, urLister) return &c, nil } @@ -183,10 +183,15 @@ func (c *MutateExistingController) report(err error, policy, rule string, target func updateURStatus(statusControl common.StatusControlInterface, ur kyvernov1beta1.UpdateRequest, err error) error { if err != nil { - return statusControl.Failed(ur, err.Error(), nil) + if _, err := statusControl.Failed(ur.GetName(), err.Error(), nil); err != nil { + return err + } + } else { + if _, err := statusControl.Success(ur.GetName(), nil); err != nil { + return err + } } - - return statusControl.Success(ur, nil) + return nil } func addAnnotation(policy kyvernov1.PolicyInterface, patched *unstructured.Unstructured, r response.RuleResponse) (patchedNew *unstructured.Unstructured, err error) { diff --git a/pkg/background/request_process.go b/pkg/background/request_process.go deleted file mode 100644 index 50debe8b79..0000000000 --- a/pkg/background/request_process.go +++ /dev/null @@ -1,74 +0,0 @@ -package background - -import ( - "context" - - kyvernov1beta1 "github.com/kyverno/kyverno/api/kyverno/v1beta1" - "github.com/kyverno/kyverno/pkg/background/common" - "github.com/kyverno/kyverno/pkg/background/generate" - "github.com/kyverno/kyverno/pkg/background/mutate" - "github.com/kyverno/kyverno/pkg/config" - jsonutils "github.com/kyverno/kyverno/pkg/utils/json" - "github.com/pkg/errors" - apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/util/retry" -) - -func (c *Controller) processUR(ur *kyvernov1beta1.UpdateRequest) error { - switch ur.Spec.Type { - case kyvernov1beta1.Mutate: - ctrl, _ := mutate.NewMutateExistingController(c.kyvernoClient, c.client, - c.policyLister, c.npolicyLister, c.urLister, c.eventGen, c.log, c.Config) - return ctrl.ProcessUR(ur) - - case kyvernov1beta1.Generate: - ctrl, _ := generate.NewGenerateController(c.kyvernoClient, c.client, - c.policyLister, c.npolicyLister, c.urLister, c.eventGen, c.nsLister, c.log, c.Config, - ) - return ctrl.ProcessUR(ur) - } - return nil -} - -func (c *Controller) markUR(ur *kyvernov1beta1.UpdateRequest) (*kyvernov1beta1.UpdateRequest, bool, error) { - ur = ur.DeepCopy() - if ur.Status.Handler != "" { - return ur, ur.Status.Handler == config.KyvernoPodName(), nil - } - err := retry.RetryOnConflict(common.DefaultRetry, func() error { - var retryError error - ur.Status.Handler = config.KyvernoPodName() - ur, retryError = c.kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).UpdateStatus(context.TODO(), ur, metav1.UpdateOptions{}) - return retryError - }) - return ur, true, err -} - -func (c *Controller) unmarkUR(ur *kyvernov1beta1.UpdateRequest) error { - if _, err := c.patchHandler(ur, ""); err != nil { - return err - } - if ur.Spec.Type == kyvernov1beta1.Mutate && ur.Status.State == kyvernov1beta1.Completed { - return c.kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).Delete(context.TODO(), ur.GetName(), metav1.DeleteOptions{}) - } - return nil -} - -func (c *Controller) patchHandler(ur *kyvernov1beta1.UpdateRequest, val string) (*kyvernov1beta1.UpdateRequest, error) { - patch := jsonutils.NewPatch( - "/status/handler", - "replace", - val, - ) - - updateUR, err := common.PatchUpdateRequest(ur, patch, c.kyvernoClient, "status") - if err != nil && !apierrors.IsNotFound(err) { - c.log.Error(err, "failed to patch UpdateRequest: %v", patch) - if val == "" { - return nil, errors.Wrapf(err, "failed to patch UpdateRequest to clear /status/handler") - } - return nil, errors.Wrapf(err, "failed to patch UpdateRequest to update /status/handler to %s", val) - } - return updateUR, nil -} diff --git a/pkg/background/update_request_controller.go b/pkg/background/update_request_controller.go index e06155113b..0eb4eba11d 100644 --- a/pkg/background/update_request_controller.go +++ b/pkg/background/update_request_controller.go @@ -1,15 +1,15 @@ package background import ( + "context" "fmt" - "reflect" "time" - "github.com/go-logr/logr" kyvernov1 "github.com/kyverno/kyverno/api/kyverno/v1" kyvernov1beta1 "github.com/kyverno/kyverno/api/kyverno/v1beta1" - "github.com/kyverno/kyverno/pkg/autogen" common "github.com/kyverno/kyverno/pkg/background/common" + "github.com/kyverno/kyverno/pkg/background/generate" + "github.com/kyverno/kyverno/pkg/background/mutate" kyvernoclient "github.com/kyverno/kyverno/pkg/client/clientset/versioned" kyvernov1informers "github.com/kyverno/kyverno/pkg/client/informers/externalversions/kyverno/v1" kyvernov1beta1informers "github.com/kyverno/kyverno/pkg/client/informers/externalversions/kyverno/v1beta1" @@ -20,12 +20,14 @@ import ( "github.com/kyverno/kyverno/pkg/event" admissionv1 "k8s.io/api/admission/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" - utilruntime "k8s.io/apimachinery/pkg/util/runtime" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes" corev1listers "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" + "k8s.io/client-go/util/retry" "k8s.io/client-go/util/workqueue" ) @@ -33,40 +35,27 @@ const ( maxRetries = 10 ) -// Controller manages the life-cycle for Generate-Requests and applies generate rule -type Controller struct { - // dynamic client implementation - client dclient.Interface +type Controller interface { + Run(int, <-chan struct{}) +} - // typed client for Kyverno CRDs +// controller manages the life-cycle for Generate-Requests and applies generate rule +type controller struct { + // clients + client dclient.Interface kyvernoClient kyvernoclient.Interface - policyInformer kyvernov1informers.ClusterPolicyInformer + // listers + policyLister kyvernov1listers.ClusterPolicyLister + npolicyLister kyvernov1listers.PolicyLister + urLister kyvernov1beta1listers.UpdateRequestNamespaceLister + nsLister corev1listers.NamespaceLister - // event generator interface - eventGen event.Interface - - // urStatusControl is used to update UR status - statusControl common.StatusControlInterface - - // UR that need to be synced + // queue queue workqueue.RateLimitingInterface - // policyLister can list/get cluster policy from the shared informer's store - policyLister kyvernov1listers.ClusterPolicyLister - - // policyLister can list/get Namespace policy from the shared informer's store - npolicyLister kyvernov1listers.PolicyLister - - // urLister can list/get update request from the shared informer's store - urLister kyvernov1beta1listers.UpdateRequestNamespaceLister - - // nsLister can list/get namespaces from the shared informer's store - nsLister corev1listers.NamespaceLister - - log logr.Logger - - Config config.Configuration + eventGen event.Interface + configuration config.Configuration } // NewController returns an instance of the Generate-Request Controller @@ -79,45 +68,40 @@ func NewController( urInformer kyvernov1beta1informers.UpdateRequestInformer, eventGen event.Interface, namespaceInformer corev1informers.NamespaceInformer, - log logr.Logger, dynamicConfig config.Configuration, -) *Controller { - c := Controller{ - client: client, - kyvernoClient: kyvernoClient, - policyInformer: policyInformer, - eventGen: eventGen, - queue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "generate-request"), - log: log, - Config: dynamicConfig, - statusControl: common.StatusControl{Client: kyvernoClient}, - policyLister: policyInformer.Lister(), - npolicyLister: npolicyInformer.Lister(), - urLister: urInformer.Lister().UpdateRequests(config.KyvernoNamespace()), - nsLister: namespaceInformer.Lister(), +) Controller { + urLister := urInformer.Lister().UpdateRequests(config.KyvernoNamespace()) + c := controller{ + client: client, + kyvernoClient: kyvernoClient, + policyLister: policyInformer.Lister(), + npolicyLister: npolicyInformer.Lister(), + urLister: urLister, + nsLister: namespaceInformer.Lister(), + queue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "generate-request"), + eventGen: eventGen, + configuration: dynamicConfig, } urInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: c.addUR, UpdateFunc: c.updateUR, DeleteFunc: c.deleteUR, }) + policyInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ + UpdateFunc: c.updatePolicy, // We only handle updates to policy + // Deletion of policy will be handled by cleanup controller + }) return &c } // Run starts workers -func (c *Controller) Run(workers int, stopCh <-chan struct{}) { - logger := c.log - defer utilruntime.HandleCrash() +func (c *controller) Run(workers int, stopCh <-chan struct{}) { + defer runtime.HandleCrash() defer c.queue.ShutDown() logger.Info("starting") defer logger.Info("shutting down") - c.policyInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ - UpdateFunc: c.updatePolicy, // We only handle updates to policy - // Deletion of policy will be handled by cleanup controller - }) - for i := 0; i < workers; i++ { go wait.Until(c.worker, time.Second, stopCh) } @@ -127,12 +111,12 @@ func (c *Controller) Run(workers int, stopCh <-chan struct{}) { // worker runs a worker thread that just dequeues items, processes them, and marks them done. // It enforces that the syncHandler is never invoked concurrently with the same key. -func (c *Controller) worker() { +func (c *controller) worker() { for c.processNextWorkItem() { } } -func (c *Controller) processNextWorkItem() bool { +func (c *controller) processNextWorkItem() bool { key, quit := c.queue.Get() if quit { return false @@ -144,8 +128,7 @@ func (c *Controller) processNextWorkItem() bool { return true } -func (c *Controller) handleErr(err error, key interface{}) { - logger := c.log +func (c *controller) handleErr(err error, key interface{}) { if err == nil { c.queue.Forget(key) return @@ -167,9 +150,7 @@ func (c *Controller) handleErr(err error, key interface{}) { c.queue.Forget(key) } -func (c *Controller) syncUpdateRequest(key string) error { - logger := c.log - var err error +func (c *controller) syncUpdateRequest(key string) error { startTime := time.Now() logger.V(4).Info("started sync", "key", key, "startTime", startTime) defer func() { @@ -181,62 +162,55 @@ func (c *Controller) syncUpdateRequest(key string) error { } ur, err := c.urLister.Get(urName) if err != nil { - if apierrors.IsNotFound(err) { + return err + } + // if not in any state, try to set it to pending + if ur.Status.State == "" { + ur = ur.DeepCopy() + ur.Status.State = kyvernov1beta1.Pending + _, err := c.kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).UpdateStatus(context.TODO(), ur, metav1.UpdateOptions{}) + return err + } + // if in pending state, try to acquire ur and eventually process it + if ur.Status.State == kyvernov1beta1.Pending { + ur, ok, err := c.acquireUR(ur) + if err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return fmt.Errorf("failed to mark handler for UR %s: %v", key, err) + } + if !ok { + logger.V(3).Info("another instance is handling the UR", "handler", ur.Status.Handler) return nil } - return fmt.Errorf("failed to fetch update request %s: %v", key, err) + logger.V(3).Info("UR is marked successfully", "ur", ur.GetName(), "resourceVersion", ur.GetResourceVersion()) + if err := c.processUR(ur); err != nil { + return fmt.Errorf("failed to process UR %s: %v", key, err) + } } - ur, ok, err := c.markUR(ur) + ur, err = c.releaseUR(ur) if err != nil { - return fmt.Errorf("failed to mark handler for UR %s: %v", key, err) - } - if !ok { - logger.V(3).Info("another instance is handling the UR", "handler", ur.Status.Handler) - return nil - } - logger.V(3).Info("UR is marked successfully", "ur", ur.GetName(), "resourceVersion", ur.GetResourceVersion()) - if err := c.processUR(ur); err != nil { - return fmt.Errorf("failed to process UR %s: %v", key, err) - } - if err = c.unmarkUR(ur); err != nil { return fmt.Errorf("failed to unmark UR %s: %v", key, err) } - return nil + err = c.cleanUR(ur) + return err } -func (c *Controller) enqueueUpdateRequest(obj interface{}) { +func (c *controller) enqueueUpdateRequest(obj interface{}) { key, err := cache.MetaNamespaceKeyFunc(obj) if err != nil { - c.log.Error(err, "failed to extract name") + logger.Error(err, "failed to extract name") return } - - c.log.V(5).Info("enqueued update request", "ur", key) + logger.V(5).Info("enqueued update request", "ur", key) c.queue.Add(key) } -func (c *Controller) updatePolicy(old, cur interface{}) { - logger := c.log +func (c *controller) updatePolicy(old, cur interface{}) { oldP := old.(*kyvernov1.ClusterPolicy) curP := cur.(*kyvernov1.ClusterPolicy) if oldP.ResourceVersion == curP.ResourceVersion { - // Periodic resync will send update events for all known Namespace. - // Two different versions of the same replica set will always have different RVs. - return - } - - var policyHasGenerate bool - for _, rule := range autogen.ComputeRules(curP) { - if rule.HasGenerate() { - policyHasGenerate = true - } - } - - if reflect.DeepEqual(curP.Spec, oldP.Spec) { - policyHasGenerate = false - } - - if !policyHasGenerate { return } @@ -255,36 +229,17 @@ func (c *Controller) updatePolicy(old, cur interface{}) { } } -func (c *Controller) addUR(obj interface{}) { +func (c *controller) addUR(obj interface{}) { ur := obj.(*kyvernov1beta1.UpdateRequest) - if ur.Status.Handler != "" { - return - } c.enqueueUpdateRequest(ur) } -func (c *Controller) updateUR(old, cur interface{}) { - oldUr := old.(*kyvernov1beta1.UpdateRequest) +func (c *controller) updateUR(_, cur interface{}) { curUr := cur.(*kyvernov1beta1.UpdateRequest) - if oldUr.ResourceVersion == curUr.ResourceVersion { - // Periodic resync will send update events for all known Namespace. - // Two different versions of the same replica set will always have different RVs. - return - } - // only process the ones that are in "Pending"/"Completed" state - // if the UPDATE Request fails due to incorrect policy, it will be requeued during policy update - if curUr.Status.State != kyvernov1beta1.Pending { - return - } - - if curUr.Status.Handler != "" { - return - } c.enqueueUpdateRequest(curUr) } -func (c *Controller) deleteUR(obj interface{}) { - logger := c.log +func (c *controller) deleteUR(obj interface{}) { ur, ok := obj.(*kyvernov1beta1.UpdateRequest) if !ok { tombstone, ok := obj.(cache.DeletedFinalStateUnknown) @@ -298,11 +253,65 @@ func (c *Controller) deleteUR(obj interface{}) { return } } - - if ur.Status.Handler != "" { - return - } - // sync Handler will remove it from the queue c.enqueueUpdateRequest(ur) } + +func (c *controller) processUR(ur *kyvernov1beta1.UpdateRequest) error { + switch ur.Spec.Type { + case kyvernov1beta1.Mutate: + ctrl, _ := mutate.NewMutateExistingController(c.kyvernoClient, c.client, + c.policyLister, c.npolicyLister, c.urLister, c.eventGen, logger, c.configuration) + return ctrl.ProcessUR(ur) + + case kyvernov1beta1.Generate: + ctrl, _ := generate.NewGenerateController(c.kyvernoClient, c.client, + c.policyLister, c.npolicyLister, c.urLister, c.eventGen, c.nsLister, logger, c.configuration, + ) + return ctrl.ProcessUR(ur) + } + return nil +} + +func (c *controller) acquireUR(ur *kyvernov1beta1.UpdateRequest) (*kyvernov1beta1.UpdateRequest, bool, error) { + err := retry.RetryOnConflict(common.DefaultRetry, func() error { + var err error + ur, err = c.urLister.Get(ur.GetName()) + if err != nil { + return err + } + if ur.Status.Handler != "" { + return nil + } + ur = ur.DeepCopy() + ur.Status.Handler = config.KyvernoPodName() + ur, err = c.kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).UpdateStatus(context.TODO(), ur, metav1.UpdateOptions{}) + return err + }) + return ur, ur.Status.Handler == config.KyvernoPodName(), err +} + +func (c *controller) releaseUR(ur *kyvernov1beta1.UpdateRequest) (*kyvernov1beta1.UpdateRequest, error) { + err := retry.RetryOnConflict(common.DefaultRetry, func() error { + var err error + ur, err = c.urLister.Get(ur.GetName()) + if err != nil { + return err + } + if ur.Status.Handler != config.KyvernoPodName() { + return nil + } + ur = ur.DeepCopy() + ur.Status.Handler = "" + ur, err = c.kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).UpdateStatus(context.TODO(), ur, metav1.UpdateOptions{}) + return err + }) + return ur, err +} + +func (c *controller) cleanUR(ur *kyvernov1beta1.UpdateRequest) error { + if ur.Spec.Type == kyvernov1beta1.Mutate && ur.Status.State == kyvernov1beta1.Completed { + return c.kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).Delete(context.TODO(), ur.GetName(), metav1.DeleteOptions{}) + } + return nil +} diff --git a/pkg/policy/policy_controller.go b/pkg/policy/policy_controller.go index 07b3ae9c51..0956326678 100644 --- a/pkg/policy/policy_controller.go +++ b/pkg/policy/policy_controller.go @@ -14,6 +14,7 @@ import ( kyvernov1beta1 "github.com/kyverno/kyverno/api/kyverno/v1beta1" utilscommon "github.com/kyverno/kyverno/cmd/cli/kubectl-kyverno/utils/common" "github.com/kyverno/kyverno/pkg/autogen" + common "github.com/kyverno/kyverno/pkg/background/common" kyvernoclient "github.com/kyverno/kyverno/pkg/client/clientset/versioned" "github.com/kyverno/kyverno/pkg/client/clientset/versioned/scheme" kyvernov1informers "github.com/kyverno/kyverno/pkg/client/informers/externalversions/kyverno/v1" @@ -530,29 +531,26 @@ func deleteUR(kyvernoClient kyvernoclient.Interface, policyKey string, grList [] } } -func updateUR(kyvernoClient kyvernoclient.Interface, policyKey string, urList []*kyvernov1beta1.UpdateRequest, logger logr.Logger) { +func updateUR(kyvernoClient kyvernoclient.Interface, urLister kyvernov1beta1listers.UpdateRequestNamespaceLister, policyKey string, urList []*kyvernov1beta1.UpdateRequest, logger logr.Logger) { for _, ur := range urList { if policyKey == ur.Spec.Policy { - urLabels := ur.Labels - if len(urLabels) == 0 { - urLabels = make(map[string]string) - } - - nBig, err := rand.Int(rand.Reader, big.NewInt(100000)) - if err != nil { - logger.Error(err, "failed to generate random interger") - } - urLabels["policy-update"] = fmt.Sprintf("revision-count-%d", nBig.Int64()) - ur.SetLabels(urLabels) - - new, err := kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).Update(context.TODO(), ur, metav1.UpdateOptions{}) + _, err := common.Update(kyvernoClient, urLister, ur.GetName(), func(ur *kyvernov1beta1.UpdateRequest) { + urLabels := ur.Labels + if len(urLabels) == 0 { + urLabels = make(map[string]string) + } + nBig, err := rand.Int(rand.Reader, big.NewInt(100000)) + if err != nil { + logger.Error(err, "failed to generate random interger") + } + urLabels["policy-update"] = fmt.Sprintf("revision-count-%d", nBig.Int64()) + ur.SetLabels(urLabels) + }) if err != nil { logger.Error(err, "failed to update gr", "name", ur.GetName()) continue } - - new.Status.State = kyvernov1beta1.Pending - if _, err := kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).UpdateStatus(context.TODO(), new, metav1.UpdateOptions{}); err != nil { + if _, err := common.UpdateStatus(kyvernoClient, urLister, ur.GetName(), kyvernov1beta1.Pending, "", nil); err != nil { logger.Error(err, "failed to set UpdateRequest state to Pending") } } diff --git a/pkg/policy/updaterequest.go b/pkg/policy/updaterequest.go index 83374a5513..d069dcecd9 100644 --- a/pkg/policy/updaterequest.go +++ b/pkg/policy/updaterequest.go @@ -31,7 +31,7 @@ func (pc *PolicyController) updateUR(policyKey string, policy kyvernov1.PolicyIn var errors []error mutateURs := pc.listMutateURs(policyKey, nil) generateURs := pc.listGenerateURs(policyKey, nil) - updateUR(pc.kyvernoClient, policyKey, append(mutateURs, generateURs...), pc.log.WithName("updateUR")) + updateUR(pc.kyvernoClient, pc.urLister.UpdateRequests(config.KyvernoNamespace()), policyKey, append(mutateURs, generateURs...), pc.log.WithName("updateUR")) for _, rule := range policy.GetSpec().Rules { var ruleType kyvernov1beta1.RequestType @@ -115,16 +115,10 @@ func (pc *PolicyController) handleUpdateRequest(ur *kyvernov1beta1.UpdateRequest } pc.log.Info("creating new UR for generate") - new, err := pc.kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).Create(context.TODO(), ur, metav1.CreateOptions{}) + _, err := pc.kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).Create(context.TODO(), ur, metav1.CreateOptions{}) if err != nil { return false, err } - - new.Status.State = kyvernov1beta1.Pending - if _, err := pc.kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).UpdateStatus(context.TODO(), new, metav1.UpdateOptions{}); err != nil { - pc.log.Error(err, "failed to set UpdateRequest state to Pending") - return false, err - } } return false, err } diff --git a/pkg/webhooks/resource/generation.go b/pkg/webhooks/resource/generation.go index 967743ed66..438caba8f4 100644 --- a/pkg/webhooks/resource/generation.go +++ b/pkg/webhooks/resource/generation.go @@ -1,7 +1,7 @@ package resource import ( - contextdefault "context" + "context" "fmt" "strings" "time" @@ -18,7 +18,6 @@ import ( "github.com/kyverno/kyverno/pkg/engine/response" enginutils "github.com/kyverno/kyverno/pkg/engine/utils" "github.com/kyverno/kyverno/pkg/event" - jsonutils "github.com/kyverno/kyverno/pkg/utils/json" admissionv1 "k8s.io/api/admission/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -113,7 +112,7 @@ func (h *handlers) handleUpdateGenerateSourceResource(resLabels map[string]strin policyNames := strings.Split(resLabels["generate.kyverno.io/clone-policy-name"], ",") for _, policyName := range policyNames { // check if the policy exists - _, err := h.kyvernoClient.KyvernoV1().ClusterPolicies().Get(contextdefault.TODO(), policyName, metav1.GetOptions{}) + _, err := h.kyvernoClient.KyvernoV1().ClusterPolicies().Get(context.TODO(), policyName, metav1.GetOptions{}) if err != nil { if strings.Contains(err.Error(), "not found") { logger.V(4).Info("skipping update of update request as policy is deleted") @@ -141,28 +140,18 @@ func (h *handlers) handleUpdateGenerateSourceResource(resLabels map[string]strin // updateAnnotationInUR - function used to update UR annotation // updating UR will trigger reprocessing of UR and recreation/updation of generated resource func (h *handlers) updateAnnotationInUR(ur *kyvernov1beta1.UpdateRequest, logger logr.Logger) { - urAnnotations := ur.Annotations - if len(urAnnotations) == 0 { - urAnnotations = make(map[string]string) - } - h.mu.Lock() - urAnnotations["generate.kyverno.io/updation-time"] = time.Now().String() - ur.SetAnnotations(urAnnotations) - h.mu.Unlock() - - patch := jsonutils.NewPatch( - "/metadata/annotations", - "replace", - ur.Annotations, - ) - - new, err := gencommon.PatchUpdateRequest(ur, patch, h.kyvernoClient) - if err != nil { + if _, err := gencommon.Update(h.kyvernoClient, h.urLister, ur.GetName(), func(ur *kyvernov1beta1.UpdateRequest) { + urAnnotations := ur.Annotations + if len(urAnnotations) == 0 { + urAnnotations = make(map[string]string) + } + urAnnotations["generate.kyverno.io/updation-time"] = time.Now().String() + ur.SetAnnotations(urAnnotations) + }); err != nil { logger.Error(err, "failed to update update request update-time annotations for the resource", "update request", ur.Name) return } - new.Status.State = kyvernov1beta1.Pending - if _, err := h.kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).UpdateStatus(contextdefault.TODO(), new, metav1.UpdateOptions{}); err != nil { + if _, err := gencommon.UpdateStatus(h.kyvernoClient, h.urLister, ur.GetName(), kyvernov1beta1.Pending, "", nil); err != nil { logger.Error(err, "failed to set UpdateRequest state to Pending", "update request", ur.Name) } } @@ -179,7 +168,7 @@ func (h *handlers) handleUpdateGenerateTargetResource(request *admissionv1.Admis targetSourceName := newRes.GetName() targetSourceKind := newRes.GetKind() - policy, err := h.kyvernoClient.KyvernoV1().ClusterPolicies().Get(contextdefault.TODO(), policyName, metav1.GetOptions{}) + policy, err := h.kyvernoClient.KyvernoV1().ClusterPolicies().Get(context.TODO(), policyName, metav1.GetOptions{}) if err != nil { logger.Error(err, "failed to get policy from kyverno client.", "policy name", policyName) return @@ -245,14 +234,9 @@ func (h *handlers) deleteGR(logger logr.Logger, engineResponse *response.EngineR } for _, v := range urList { - err := h.kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).Delete(contextdefault.TODO(), v.GetName(), metav1.DeleteOptions{}) + err := h.kyvernoClient.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).Delete(context.TODO(), v.GetName(), metav1.DeleteOptions{}) if err != nil { logger.Error(err, "failed to update ur") } } } - -// type updateRequestResponse struct { -// ur urkyverno.UpdateRequestSpec -// err error -// } diff --git a/pkg/webhooks/updaterequest/generator.go b/pkg/webhooks/updaterequest/generator.go index 6c89d2eb0e..0c870ba92a 100644 --- a/pkg/webhooks/updaterequest/generator.go +++ b/pkg/webhooks/updaterequest/generator.go @@ -8,6 +8,7 @@ import ( "github.com/gardener/controller-manager-library/pkg/logger" "github.com/go-logr/logr" kyvernov1beta1 "github.com/kyverno/kyverno/api/kyverno/v1beta1" + "github.com/kyverno/kyverno/pkg/background/common" kyvernoclient "github.com/kyverno/kyverno/pkg/client/clientset/versioned" kyvernov1beta1informers "github.com/kyverno/kyverno/pkg/client/informers/externalversions/kyverno/v1beta1" kyvernov1beta1listers "github.com/kyverno/kyverno/pkg/client/listers/kyverno/v1beta1" @@ -17,6 +18,7 @@ import ( "k8s.io/apimachinery/pkg/labels" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/client-go/tools/cache" + "k8s.io/client-go/util/retry" ) // UpdateRequest provides interface to manage update requests @@ -163,13 +165,19 @@ func retryApplyResource( } else { log.V(4).Info("successfully updated UpdateRequest", "retryCount", i, "name", ur.GetName(), "namespace", ur.GetNamespace()) } - - new.Status.State = kyvernov1beta1.Pending - if _, err := client.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).UpdateStatus(context.TODO(), new, metav1.UpdateOptions{}); err != nil { + err = retry.RetryOnConflict(common.DefaultRetry, func() error { + ur, err := client.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).Get(context.TODO(), new.GetName(), metav1.GetOptions{}) + if err != nil { + return err + } + ur.Status.State = kyvernov1beta1.Pending + _, err = client.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).UpdateStatus(context.TODO(), ur, metav1.UpdateOptions{}) + return err + }) + if err != nil { log.Error(err, "failed to set UpdateRequest state to Pending") return err } - isExist = true } @@ -187,12 +195,6 @@ func retryApplyResource( } else { log.V(4).Info("successfully created UpdateRequest", "retryCount", i, "name", new.GetName(), "namespace", ur.GetNamespace()) } - - new.Status.State = kyvernov1beta1.Pending - if _, err := client.KyvernoV1beta1().UpdateRequests(config.KyvernoNamespace()).UpdateStatus(context.TODO(), new, metav1.UpdateOptions{}); err != nil { - log.Error(err, "failed to set UpdateRequest state to Pending") - return err - } } return nil diff --git a/test/e2e/generate/resources.go b/test/e2e/generate/resources.go index ff1f2d4296..7764f39cb7 100644 --- a/test/e2e/generate/resources.go +++ b/test/e2e/generate/resources.go @@ -255,6 +255,7 @@ metadata: name: add-networkpolicy spec: background: true + generateExistingOnPolicyUpdate: true rules: - name: allow-dns match: @@ -294,6 +295,7 @@ metadata: name: add-networkpolicy spec: background: true + generateExistingOnPolicyUpdate: true rules: - name: allow-dns match: @@ -333,6 +335,7 @@ metadata: name: add-networkpolicy spec: background: true + generateExistingOnPolicyUpdate: true rules: - name: allow-dns match: