diff --git a/pkg/internal/controllers/certificaterequestpolicies.go b/pkg/internal/controllers/certificaterequestpolicies.go index d758a5b8..9cfbed19 100644 --- a/pkg/internal/controllers/certificaterequestpolicies.go +++ b/pkg/internal/controllers/certificaterequestpolicies.go @@ -142,7 +142,7 @@ func addCertificateRequestPolicyController(_ context.Context, opts Options) erro func (c *certificaterequestpolicies) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { result, patch, resultErr := c.reconcileStatusPatch(ctx, req) if patch != nil { - crp, patch, err := ssa_client.GenerateCertificateRequestPolicyStatusPatch(req.Name, req.Namespace, patch) + crp, patch, err := ssa_client.GenerateCertificateRequestPolicyStatusPatch(req.Name, patch) if err != nil { err = fmt.Errorf("failed to generate CertificateRequestPolicy.Status patch: %w", err) return ctrl.Result{}, utilerrors.NewAggregate([]error{resultErr, err}) @@ -214,6 +214,7 @@ func (c *certificaterequestpolicies) reconcileStatusPatch(ctx context.Context, r c.recorder.Event(policy, corev1.EventTypeWarning, "NotReady", message) c.setCertificateRequestPolicyCondition( + policy.Status.Conditions, &policyPatch.Conditions, policy.Generation, policyapi.CertificateRequestPolicyCondition{ @@ -233,6 +234,7 @@ func (c *certificaterequestpolicies) reconcileStatusPatch(ctx context.Context, r c.recorder.Event(policy, corev1.EventTypeNormal, "Ready", message) c.setCertificateRequestPolicyCondition( + policy.Status.Conditions, &policyPatch.Conditions, policy.Generation, policyapi.CertificateRequestPolicyCondition{ @@ -256,30 +258,47 @@ func (c *certificaterequestpolicies) reconcileStatusPatch(ctx context.Context, r // Returns true if the condition has been updated or an existing condition has // been updated. Returns false otherwise. func (c *certificaterequestpolicies) setCertificateRequestPolicyCondition( - conditions *[]policyapi.CertificateRequestPolicyCondition, + existingConditions []policyapi.CertificateRequestPolicyCondition, + patchConditions *[]policyapi.CertificateRequestPolicyCondition, generation int64, - condition policyapi.CertificateRequestPolicyCondition, + newCondition policyapi.CertificateRequestPolicyCondition, ) { - condition.LastTransitionTime = &metav1.Time{Time: c.clock.Now()} - condition.ObservedGeneration = generation + newCondition.LastTransitionTime = &metav1.Time{Time: c.clock.Now()} + newCondition.ObservedGeneration = generation - for idx, existingCondition := range *conditions { + for _, existingCondition := range existingConditions { // Skip unrelated conditions - if existingCondition.Type != condition.Type { + if existingCondition.Type != newCondition.Type { continue } // If this update doesn't contain a state transition, we don't update // the conditions LastTransitionTime to Now() - if existingCondition.Status == condition.Status { - condition.LastTransitionTime = existingCondition.LastTransitionTime + if existingCondition.Status == newCondition.Status { + newCondition.LastTransitionTime = existingCondition.LastTransitionTime } + } + + // Search through existing conditions + for idx, patchCondition := range *patchConditions { + // Skip unrelated conditions + if patchCondition.Type != newCondition.Type { + continue + } + + // If this update doesn't contain a state transition, we don't update + // the conditions LastTransitionTime to Now() + if patchCondition.Status == newCondition.Status { + newCondition.LastTransitionTime = patchCondition.LastTransitionTime + } + + // Overwrite the existing condition + (*patchConditions)[idx] = newCondition - (*conditions)[idx] = condition return } // If we've not found an existing condition of this type, we simply insert // the new condition into the slice. - *conditions = append(*conditions, condition) + *patchConditions = append(*patchConditions, newCondition) } diff --git a/pkg/internal/controllers/certificaterequestpolicies_test.go b/pkg/internal/controllers/certificaterequestpolicies_test.go index e16839ff..2d53d4f9 100644 --- a/pkg/internal/controllers/certificaterequestpolicies_test.go +++ b/pkg/internal/controllers/certificaterequestpolicies_test.go @@ -483,6 +483,7 @@ func Test_certificaterequestpolicies_setCertificateRequestPolicyCondition(t *tes tests := map[string]struct { existingConditions []policyapi.CertificateRequestPolicyCondition + patchConditions []policyapi.CertificateRequestPolicyCondition newCondition policyapi.CertificateRequestPolicyCondition expectedConditions []policyapi.CertificateRequestPolicyCondition }{ @@ -503,8 +504,8 @@ func Test_certificaterequestpolicies_setCertificateRequestPolicyCondition(t *tes ObservedGeneration: policyGeneration, }}, }, - "an existing condition of different type should add a different condition with time and gen to the policy": { - existingConditions: []policyapi.CertificateRequestPolicyCondition{{Type: "B"}}, + "an existing patch condition of different type should add a different condition with time and gen to the policy": { + patchConditions: []policyapi.CertificateRequestPolicyCondition{{Type: "B"}}, newCondition: policyapi.CertificateRequestPolicyCondition{ Type: "A", Status: corev1.ConditionTrue, @@ -523,8 +524,8 @@ func Test_certificaterequestpolicies_setCertificateRequestPolicyCondition(t *tes }, }, }, - "an existing condition of the same type but different status should be replaced with new time if it has a different status": { - existingConditions: []policyapi.CertificateRequestPolicyCondition{ + "an existing patch condition of the same type but different status should be replaced with new time if it has a different status": { + patchConditions: []policyapi.CertificateRequestPolicyCondition{ {Type: "B"}, { Type: "A", @@ -553,8 +554,8 @@ func Test_certificaterequestpolicies_setCertificateRequestPolicyCondition(t *tes }, }, }, - "an existing condition of the same type and status should be replaced with same time": { - existingConditions: []policyapi.CertificateRequestPolicyCondition{ + "an existing patch condition of the same type and status should be replaced with same time": { + patchConditions: []policyapi.CertificateRequestPolicyCondition{ {Type: "B"}, { Type: "A", @@ -589,13 +590,14 @@ func Test_certificaterequestpolicies_setCertificateRequestPolicyCondition(t *tes t.Run(name, func(t *testing.T) { c := &certificaterequestpolicies{clock: fixedclock} c.setCertificateRequestPolicyCondition( - &test.existingConditions, + test.existingConditions, + &test.patchConditions, policyGeneration, test.newCondition, ) - if !apiequality.Semantic.DeepEqual(test.existingConditions, test.expectedConditions) { - t.Errorf("unexpected resulting conditions, exp=%v got=%v", test.expectedConditions, test.existingConditions) + if !apiequality.Semantic.DeepEqual(test.patchConditions, test.expectedConditions) { + t.Errorf("unexpected resulting conditions, exp=%v got=%v", test.expectedConditions, test.patchConditions) } }) } diff --git a/pkg/internal/controllers/ssa_client/certificaterequestpolicy_status.go b/pkg/internal/controllers/ssa_client/certificaterequestpolicy_status.go index 1cba7458..3c637e6c 100644 --- a/pkg/internal/controllers/ssa_client/certificaterequestpolicy_status.go +++ b/pkg/internal/controllers/ssa_client/certificaterequestpolicy_status.go @@ -32,12 +32,12 @@ type certificateRequestPolicyStatusStatusApplyConfiguration struct { } func GenerateCertificateRequestPolicyStatusPatch( - name, namespace string, + name string, status *policyapi.CertificateRequestPolicyStatus, ) (*policyapi.CertificateRequestPolicy, client.Patch, error) { - // This object is used to deduce the name & namespace + unmarshall the return value in + // This object is used to deduce the name + unmarshall the return value in crp := &policyapi.CertificateRequestPolicy{ - ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, + ObjectMeta: metav1.ObjectMeta{Name: name}, } // This object is used to render the patch @@ -45,7 +45,6 @@ func GenerateCertificateRequestPolicyStatusPatch( ObjectMetaApplyConfiguration: &v1.ObjectMetaApplyConfiguration{}, } b.WithName(name) - b.WithNamespace(namespace) b.WithKind(policyapi.CertificateRequestPolicyKind) b.WithAPIVersion(policyapi.SchemeGroupVersion.Identifier()) b.Status = status diff --git a/pkg/internal/controllers/test/review.go b/pkg/internal/controllers/test/review.go index 4f70368d..ec415cd6 100644 --- a/pkg/internal/controllers/test/review.go +++ b/pkg/internal/controllers/test/review.go @@ -28,6 +28,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest/komega" policyapi "github.com/cert-manager/approver-policy/pkg/apis/policy/v1alpha1" "github.com/cert-manager/approver-policy/pkg/approver" @@ -589,4 +590,36 @@ var _ = Context("Review", func() { deleteRoleAndRoleBindings(ctx, env.AdminClient, namespace.Name, userUsePolicyRoleName, userCreateCRRoleName) }) + + Context("Reconcile consistency", func() { + It("If the policy is not ready, should have stable resource version", func() { + plugin.FakeReconciler = fake.NewFakeReconciler().WithReady(func(_ context.Context, policy *policyapi.CertificateRequestPolicy) (approver.ReconcilerReadyResponse, error) { + return approver.ReconcilerReadyResponse{Ready: false}, nil + }) + + policyNotReady := policyapi.CertificateRequestPolicy{ObjectMeta: metav1.ObjectMeta{GenerateName: "not-ready-"}} + var policy policyapi.CertificateRequestPolicy + + komega.SetClient(env.AdminClient) + Expect(env.AdminClient.Create(ctx, &policyNotReady)).ToNot(HaveOccurred()) + waitForNotReady(ctx, env.AdminClient, policyNotReady.Name) + Expect(env.AdminClient.Get(ctx, client.ObjectKey{Name: policyNotReady.Name}, &policy)).To(Succeed()) + + resourceVersion := policy.ResourceVersion + Consistently(komega.Object(&policy)).Should(HaveField("ObjectMeta.ResourceVersion", Equal(resourceVersion))) + }) + + It("If the policy is ready, should have stable resource version", func() { + policyReady := policyapi.CertificateRequestPolicy{ObjectMeta: metav1.ObjectMeta{GenerateName: "ready-"}} + var policy policyapi.CertificateRequestPolicy + + komega.SetClient(env.AdminClient) + Expect(env.AdminClient.Create(ctx, &policyReady)).ToNot(HaveOccurred()) + waitForReady(ctx, env.AdminClient, policyReady.Name) + Expect(env.AdminClient.Get(ctx, client.ObjectKey{Name: policyReady.Name}, &policy)).To(Succeed()) + + resourceVersion := policy.ResourceVersion + Consistently(komega.Object(&policy)).Should(HaveField("ObjectMeta.ResourceVersion", Equal(resourceVersion))) + }) + }) })