Merge pull request #353 from hrbasic/341_fix

fix: should reconcile certificate request policies consistently

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)
 }

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)
 			}
 		})
 	}

pkg/internal/controllers/ssa_client/certificaterequestpolicy_status.go

@@ -32,20 +32,19 @@ 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
 	b := &certificateRequestPolicyStatusStatusApplyConfiguration{
 		ObjectMetaApplyConfiguration: &v1.ObjectMetaApplyConfiguration{},
 	}
 	b.WithName(name)
-	b.WithNamespace(namespace)
 	b.WithKind(policyapi.CertificateRequestPolicyKind)
 	b.WithAPIVersion(policyapi.SchemeGroupVersion.Identifier())
 	b.Status = status

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)))
+		})
+	})
 })