From e39c7b79bb45e212490c65deaa185c951cee78e2 Mon Sep 17 00:00:00 2001 From: Jaydip Gabani Date: Sat, 11 Jan 2025 02:05:42 +0000 Subject: [PATCH 1/3] adding annotation to generate VAPB right away once the waiting window is over to protect against clock skews Signed-off-by: Jaydip Gabani --- .../constraint/constraint_controller.go | 27 ++++++++++-------- .../constrainttemplate_controller.go | 28 +++++++++++++------ .../constrainttemplate_controller_test.go | 17 +++++++++-- 3 files changed, 49 insertions(+), 23 deletions(-) diff --git a/pkg/controller/constraint/constraint_controller.go b/pkg/controller/constraint/constraint_controller.go index 731bb556d1b..53df813d1a6 100644 --- a/pkg/controller/constraint/constraint_controller.go +++ b/pkg/controller/constraint/constraint_controller.go @@ -65,9 +65,12 @@ import ( const ( BlockVAPBGenerationUntilAnnotation = "gatekeeper.sh/block-vapb-generation-until" + VAPBGenerationAnnotation = "gatekeeper.sh/vapb-generation" ErrGenerateVAPBState = "error" GeneratedVAPBState = "generated" WaitVAPBState = "waiting" + VAPBGenerationBlocked = "blocked" + VAPBGenerationUnblocked = "unblocked" ) var ( @@ -528,20 +531,22 @@ func (r *ReconcileConstraint) manageVAPB(ctx context.Context, enforcementAction shouldGenerateVAPB = false default: // reconcile for vapb generation if annotation is not set - if ct.Annotations == nil || ct.Annotations[BlockVAPBGenerationUntilAnnotation] == "" { + if ct.Annotations == nil || (ct.Annotations[BlockVAPBGenerationUntilAnnotation] == "" && ct.Annotations[VAPBGenerationAnnotation] != "unblocked") { return noDelay, r.reportErrorOnConstraintStatus(ctx, status, errors.New("annotation to wait for ValidatingAdmissionPolicyBinding generation not found"), "could not find annotation to wait for ValidatingAdmissionPolicyBinding generation") } - // waiting for sometime before generating vapbinding, gives api-server time to cache CRDs - timestamp := ct.Annotations[BlockVAPBGenerationUntilAnnotation] - t, err := time.Parse(time.RFC3339, timestamp) - if err != nil { - return noDelay, r.reportErrorOnConstraintStatus(ctx, status, err, "could not parse timestamp") - } - if t.After(time.Now()) { - wait := time.Until(t) - updateEnforcementPointStatus(status, util.VAPEnforcementPoint, WaitVAPBState, fmt.Sprintf("waiting for %s before generating ValidatingAdmissionPolicyBinding to make sure api-server has cached constraint CRD", wait), instance.GetGeneration()) - return wait, r.writer.Update(ctx, status) + if ct.Annotations[VAPBGenerationAnnotation] == "" || ct.Annotations[VAPBGenerationAnnotation] == VAPBGenerationBlocked { + // waiting for sometime before generating vapbinding, gives api-server time to cache CRDs + timestamp := ct.Annotations[BlockVAPBGenerationUntilAnnotation] + t, err := time.Parse(time.RFC3339, timestamp) + if err != nil { + return noDelay, r.reportErrorOnConstraintStatus(ctx, status, err, "could not parse timestamp") + } + if t.After(time.Now()) { + wait := time.Until(t) + updateEnforcementPointStatus(status, util.VAPEnforcementPoint, WaitVAPBState, fmt.Sprintf("waiting for %s before generating ValidatingAdmissionPolicyBinding to make sure api-server has cached constraint CRD", wait), instance.GetGeneration()) + return wait, r.writer.Update(ctx, status) + } } } } diff --git a/pkg/controller/constrainttemplate/constrainttemplate_controller.go b/pkg/controller/constrainttemplate/constrainttemplate_controller.go index 2871e135dd9..c4473cc8583 100644 --- a/pkg/controller/constrainttemplate/constrainttemplate_controller.go +++ b/pkg/controller/constrainttemplate/constrainttemplate_controller.go @@ -440,7 +440,8 @@ func (r *ReconcileConstraintTemplate) handleUpdate( status.Status.VAPGenerationStatus = &statusv1beta1.VAPGenerationStatus{State: ErrGenerateVAPState, ObservedGeneration: ct.GetGeneration(), Warning: fmt.Sprintf("ValidatingAdmissionPolicy is not generated: %s", err.Error())} } - if err := r.generateCRD(ctx, ct, proposedCRD, currentCRD, status, logger, generateVap); err != nil { + var requeueAfter time.Duration + if err := r.generateCRD(ctx, ct, proposedCRD, currentCRD, status, logger, generateVap, &requeueAfter); err != nil { return reconcile.Result{}, err } @@ -459,7 +460,7 @@ func (r *ReconcileConstraintTemplate) handleUpdate( logger.Error(err, "update ct pod status error") return reconcile.Result{Requeue: true}, nil } - return reconcile.Result{}, nil + return reconcile.Result{RequeueAfter: requeueAfter}, nil } func (r *ReconcileConstraintTemplate) handleDelete( @@ -737,7 +738,7 @@ func v1beta1ToV1(v1beta1Obj *admissionregistrationv1beta1.ValidatingAdmissionPol return obj, nil } -func (r *ReconcileConstraintTemplate) generateCRD(ctx context.Context, ct *v1beta1.ConstraintTemplate, proposedCRD, currentCRD *apiextensionsv1.CustomResourceDefinition, status *statusv1beta1.ConstraintTemplatePodStatus, logger logr.Logger, generateVAP bool) error { +func (r *ReconcileConstraintTemplate) generateCRD(ctx context.Context, ct *v1beta1.ConstraintTemplate, proposedCRD, currentCRD *apiextensionsv1.CustomResourceDefinition, status *statusv1beta1.ConstraintTemplatePodStatus, logger logr.Logger, generateVAP bool, requeueAfter *time.Duration) error { if !operations.IsAssigned(operations.Generate) { return nil } @@ -769,9 +770,9 @@ func (r *ReconcileConstraintTemplate) generateCRD(ctx context.Context, ct *v1bet if !generateVAP { return nil } - + var err error // We add the annotation as a follow-on update to be sure the timestamp is set relative to a time after the CRD is successfully created. Creating the CRD with a delay timestamp already set would not account for request latency. - err := r.updateTemplateWithBlockVAPBGenerationAnnotations(ctx, ct) + *requeueAfter, err = r.updateTemplateWithBlockVAPBGenerationAnnotations(ctx, ct) if err != nil { err = r.reportErrorOnCTStatus(ctx, ErrCreateCode, "Could not annotate with timestamp to block VAPB generation", status, err) } @@ -887,23 +888,32 @@ func (r *ReconcileConstraintTemplate) manageVAP(ctx context.Context, ct *v1beta1 // updateTemplateWithBlockVAPBGenerationAnnotations updates the ConstraintTemplate with an annotation to block VAPB generation until specific time // This is to avoid the issue where the VAPB is generated before the CRD is cached in the API server. -func (r *ReconcileConstraintTemplate) updateTemplateWithBlockVAPBGenerationAnnotations(ctx context.Context, ct *v1beta1.ConstraintTemplate) error { +func (r *ReconcileConstraintTemplate) updateTemplateWithBlockVAPBGenerationAnnotations(ctx context.Context, ct *v1beta1.ConstraintTemplate) (time.Duration, error) { + noRequeue := time.Duration(0) + if ct.Annotations != nil && ct.Annotations[constraint.VAPBGenerationAnnotation] == constraint.VAPBGenerationUnblocked { + return noRequeue, nil + } currentTime := time.Now() if ct.Annotations != nil && ct.Annotations[constraint.BlockVAPBGenerationUntilAnnotation] != "" { until := ct.Annotations[constraint.BlockVAPBGenerationUntilAnnotation] t, err := time.Parse(time.RFC3339, until) if err != nil { - return err + return noRequeue, err } // if wait time is within the time window to generate vap binding, do not update the annotation // otherwise update the annotation with the current time + wait time. This prevents clock skew from preventing generation on task reschedule. if t.Before(currentTime.Add(time.Duration(*constraint.DefaultWaitForVAPBGeneration) * time.Second)) { - return nil + if t.Before(currentTime) { + ct.Annotations[constraint.VAPBGenerationAnnotation] = constraint.VAPBGenerationUnblocked + return noRequeue, r.Update(ctx, ct) + } + return t.Sub(currentTime), nil } } if ct.Annotations == nil { ct.Annotations = make(map[string]string) } ct.Annotations[constraint.BlockVAPBGenerationUntilAnnotation] = currentTime.Add(time.Duration(*constraint.DefaultWaitForVAPBGeneration) * time.Second).Format(time.RFC3339) - return r.Update(ctx, ct) + ct.Annotations[constraint.VAPBGenerationAnnotation] = constraint.VAPBGenerationBlocked + return time.Duration(*constraint.DefaultWaitForVAPBGeneration) * time.Second, r.Update(ctx, ct) } diff --git a/pkg/controller/constrainttemplate/constrainttemplate_controller_test.go b/pkg/controller/constrainttemplate/constrainttemplate_controller_test.go index 5ab10810734..1144f066932 100644 --- a/pkg/controller/constrainttemplate/constrainttemplate_controller_test.go +++ b/pkg/controller/constrainttemplate/constrainttemplate_controller_test.go @@ -304,7 +304,7 @@ func TestReconcile(t *testing.T) { t.Fatal(err) } - logger.Info("Running Test: block-vapb-generation-until annotation should not be present") + logger.Info("Running Test: vapb annotation should not be present on constraint template") err = retry.OnError(testutils.ConstantRetry, func(_ error) bool { return true }, func() error { @@ -315,6 +315,9 @@ func TestReconcile(t *testing.T) { if _, ok := ct.GetAnnotations()[constraint.BlockVAPBGenerationUntilAnnotation]; ok { return fmt.Errorf("unexpected %s annotations on CT", constraint.BlockVAPBGenerationUntilAnnotation) } + if _, ok := ct.GetAnnotations()[constraint.VAPBGenerationAnnotation]; ok { + return fmt.Errorf("unexpected %s annotations on CT", constraint.VAPBGenerationAnnotation) + } return nil }) }) @@ -726,7 +729,6 @@ func TestReconcile(t *testing.T) { if timestamp == "" { return fmt.Errorf("expected %s annotations on CT", constraint.BlockVAPBGenerationUntilAnnotation) } - // check if vapbinding resource exists now if err := c.Get(ctx, types.NamespacedName{Name: cstr.GetName()}, cstr); err != nil { return err } @@ -743,6 +745,9 @@ func TestReconcile(t *testing.T) { if vapBindingCreationTime.Before(blockTime) { return fmt.Errorf("VAPBinding should be created after default wait") } + if ct.GetAnnotations()[constraint.VAPBGenerationAnnotation] == constraint.VAPBGenerationUnblocked { + return fmt.Errorf("expected %s annotations on CT to be unblocked", constraint.VAPBGenerationAnnotation) + } return nil }) if err != nil { @@ -810,7 +815,7 @@ func TestReconcile(t *testing.T) { t.Fatal(err) } - logger.Info("Running test: VAP ConstraintTemplate should have block-VAPB-generation-until annotation") + logger.Info("Running test: VAP ConstraintTemplate should have VAPB annotation") err = retry.OnError(testutils.ConstantRetry, func(_ error) bool { return true }, func() error { @@ -821,6 +826,9 @@ func TestReconcile(t *testing.T) { if ct.GetAnnotations()[constraint.BlockVAPBGenerationUntilAnnotation] == "" { return fmt.Errorf("expected %s annotations on CT", constraint.BlockVAPBGenerationUntilAnnotation) } + if ct.GetAnnotations()[constraint.VAPBGenerationAnnotation] == "" { + return fmt.Errorf("expected %s annotations on CT", constraint.VAPBGenerationAnnotation) + } return nil }) if err != nil { @@ -871,6 +879,9 @@ func TestReconcile(t *testing.T) { if vapBindingCreationTime.Before(blockTime) { return fmt.Errorf("VAPBinding should not be created before the timestamp") } + if ct.GetAnnotations()[constraint.VAPBGenerationAnnotation] == constraint.VAPBGenerationUnblocked { + return fmt.Errorf("expected %s annotations on CT to be unblocked", constraint.VAPBGenerationAnnotation) + } if err := c.Delete(ctx, cstr); err != nil { return err } From 6a17e692844478667fde13cd784b895d5e5efcf5 Mon Sep 17 00:00:00 2001 From: Jaydip Gabani Date: Sat, 18 Jan 2025 05:24:35 +0000 Subject: [PATCH 2/3] updating annotation name Signed-off-by: Jaydip Gabani --- pkg/controller/constraint/constraint_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/constraint/constraint_controller.go b/pkg/controller/constraint/constraint_controller.go index 53df813d1a6..d37f27f05d5 100644 --- a/pkg/controller/constraint/constraint_controller.go +++ b/pkg/controller/constraint/constraint_controller.go @@ -65,7 +65,7 @@ import ( const ( BlockVAPBGenerationUntilAnnotation = "gatekeeper.sh/block-vapb-generation-until" - VAPBGenerationAnnotation = "gatekeeper.sh/vapb-generation" + VAPBGenerationAnnotation = "gatekeeper.sh/vapb-generation-state" ErrGenerateVAPBState = "error" GeneratedVAPBState = "generated" WaitVAPBState = "waiting" From 7ac275c624dc75e5d27fa9b30fe074f326bb88df Mon Sep 17 00:00:00 2001 From: Jaydip Gabani Date: Mon, 27 Jan 2025 23:57:00 +0000 Subject: [PATCH 3/3] fixing test Signed-off-by: Jaydip Gabani --- .../constrainttemplate/constrainttemplate_controller_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controller/constrainttemplate/constrainttemplate_controller_test.go b/pkg/controller/constrainttemplate/constrainttemplate_controller_test.go index 1144f066932..b422c95fca9 100644 --- a/pkg/controller/constrainttemplate/constrainttemplate_controller_test.go +++ b/pkg/controller/constrainttemplate/constrainttemplate_controller_test.go @@ -745,7 +745,7 @@ func TestReconcile(t *testing.T) { if vapBindingCreationTime.Before(blockTime) { return fmt.Errorf("VAPBinding should be created after default wait") } - if ct.GetAnnotations()[constraint.VAPBGenerationAnnotation] == constraint.VAPBGenerationUnblocked { + if ct.GetAnnotations()[constraint.VAPBGenerationAnnotation] != constraint.VAPBGenerationUnblocked { return fmt.Errorf("expected %s annotations on CT to be unblocked", constraint.VAPBGenerationAnnotation) } return nil @@ -879,7 +879,7 @@ func TestReconcile(t *testing.T) { if vapBindingCreationTime.Before(blockTime) { return fmt.Errorf("VAPBinding should not be created before the timestamp") } - if ct.GetAnnotations()[constraint.VAPBGenerationAnnotation] == constraint.VAPBGenerationUnblocked { + if ct.GetAnnotations()[constraint.VAPBGenerationAnnotation] != constraint.VAPBGenerationUnblocked { return fmt.Errorf("expected %s annotations on CT to be unblocked", constraint.VAPBGenerationAnnotation) } if err := c.Delete(ctx, cstr); err != nil {