Skip to content

Commit

Permalink
refactor: integration tests for orphaned tests & address some comments
Browse files Browse the repository at this point in the history 
Signed-off-by: KevFan <chfan@redhat.com>
  • Loading branch information
@KevFan
KevFan committed Oct 17, 2024
1 parent 58e608c commit 2c1093b
Show file tree
Hide file tree
Showing 4 changed files with 131 additions and 28 deletions.
8 changes: 6 additions & 2 deletions controllers/effective_tls_policies_reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/client-go/dynamic"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
Expand Down Expand Up @@ -162,7 +163,10 @@ func (t *EffectiveTLSPoliciesReconciler) Reconcile(ctx context.Context, events [
}

// Clean up orphaned certs
orphanedCerts, _ := lo.Difference(certs, expectedCerts)
uniqueExpectedCerts := lo.UniqBy(expectedCerts, func(item *certmanv1.Certificate) types.UID {
return item.GetUID()
})
orphanedCerts, _ := lo.Difference(certs, uniqueExpectedCerts)
for _, orphanedCert := range orphanedCerts {
resource := t.client.Resource(CertManagerCertificatesResource).Namespace(orphanedCert.GetNamespace())
if err := resource.Delete(ctx, orphanedCert.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
Expand Down Expand Up @@ -192,7 +196,7 @@ func (t *EffectiveTLSPoliciesReconciler) deleteCertificatesForPolicy(ctx context
for _, cert := range certs {
resource := t.client.Resource(CertManagerCertificatesResource).Namespace(cert.GetNamespace())

if err := resource.Delete(ctx, cert.Name, metav1.DeleteOptions{}); err != nil {
if err := resource.Delete(ctx, cert.Name, metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
return err
}
}
Expand Down
4 changes: 3 additions & 1 deletion controllers/tls_workflow.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -245,5 +245,7 @@ func GetTLSPoliciesByEvents(topology *machinery.Topology, events []controller.Re
}

// Return only unique policies as there can be duplicates from multiple events
return lo.Uniq(affectedPolicies)
return lo.UniqBy(affectedPolicies, func(item machinery.Policy) string {
return item.GetLocator()
})
}
9 changes: 7 additions & 2 deletions controllers/tlspolicy_status_updater.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/kuadrant/policy-machinery/machinery"
"github.com/samber/lo"
"k8s.io/apimachinery/pkg/api/equality"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/validation/field"
Expand Down Expand Up @@ -92,7 +93,7 @@ func (t *TLSPolicyStatusUpdater) UpdateStatus(ctx context.Context, events []cont
}

_, err = resource.UpdateStatus(ctx, un, metav1.UpdateOptions{})
if err != nil {
if err != nil && !apierrors.IsConflict(err) {
logger.Error(err, "unable to update status for TLSPolicy", "name", policy.GetName(), "namespace", policy.GetNamespace(), "uid", p.GetUID())
}
}
Expand Down Expand Up @@ -142,7 +143,11 @@ func (t *TLSPolicyStatusUpdater) isIssuerReady(ctx context.Context, policy *kuad
return o.GroupVersionKind().GroupKind() == CertManagerIssuerKind && o.GetNamespace() == policy.GetNamespace() && o.GetName() == policy.Spec.IssuerRef.Name
})
if !ok {
err := fmt.Errorf("%s \"%s\" not found", policy.Spec.IssuerRef.Kind, policy.Spec.IssuerRef.Name)
issuerRef := policy.Spec.IssuerRef.Kind
if issuerRef == "" {
issuerRef = certmanagerv1.IssuerKind
}
err := fmt.Errorf("%s \"%s\" not found", issuerRef, policy.Spec.IssuerRef.Name)
logger.Error(err, "error finding object in topology")
return err
}
Expand Down
138 changes: 115 additions & 23 deletions tests/common/tlspolicy/tlspolicy_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ var _ = Describe("TLSPolicy controller", func() {
"Type": Equal(string(kuadrant.PolicyConditionEnforced)),
})),
)
}, tests.TimeoutMedium, time.Second).Should(Succeed())
}, tests.TimeoutLong, time.Second).Should(Succeed())
}, testTimeOut)

It("should have accepted condition with status true", func(ctx SpecContext) {
Expand All @@ -112,7 +112,7 @@ var _ = Describe("TLSPolicy controller", func() {
"Type": Equal(string(kuadrant.PolicyConditionEnforced)),
})),
)
}, tests.TimeoutMedium, time.Second).Should(Succeed())
}, tests.TimeoutLong, time.Second).Should(Succeed())

By("creating a valid Gateway")
gateway = tests.NewGatewayBuilder("test-gateway", gatewayClass.Name, testNamespace).
Expand All @@ -139,7 +139,7 @@ var _ = Describe("TLSPolicy controller", func() {
"Message": Equal("TLSPolicy has been successfully enforced"),
})),
)
}, tests.TimeoutMedium, time.Second).Should(Succeed())
}, tests.TimeoutLong, time.Second).Should(Succeed())
}, testTimeOut)

})
Expand Down Expand Up @@ -173,7 +173,7 @@ var _ = Describe("TLSPolicy controller", func() {
"Type": Equal(string(kuadrant.PolicyConditionEnforced)),
})),
)
}, tests.TimeoutMedium, time.Second).Should(Succeed())
}, tests.TimeoutLong, time.Second).Should(Succeed())
}, testTimeOut)

It("unable to find issuer - should have accepted condition with status false and correct reason", func(ctx SpecContext) {
Expand All @@ -198,7 +198,7 @@ var _ = Describe("TLSPolicy controller", func() {
"Type": Equal(string(kuadrant.PolicyConditionEnforced)),
})),
)
}, tests.TimeoutMedium, time.Second).Should(Succeed())
}, tests.TimeoutLong, time.Second).Should(Succeed())
}, testTimeOut)
})

Expand Down Expand Up @@ -233,7 +233,7 @@ var _ = Describe("TLSPolicy controller", func() {
"Message": Equal("TLSPolicy has been successfully enforced"),
})),
)
}, tests.TimeoutMedium, time.Second).Should(Succeed())
}, tests.TimeoutLong, time.Second).Should(Succeed())
}, testTimeOut)
})

Expand Down Expand Up @@ -278,7 +278,7 @@ var _ = Describe("TLSPolicy controller", func() {
"Message": Equal("TLSPolicy has been successfully enforced"),
})),
)
}, tests.TimeoutMedium, time.Second).Should(Succeed())
}, tests.TimeoutLong, time.Second).Should(Succeed())
}, testTimeOut)
})

Expand Down Expand Up @@ -326,7 +326,7 @@ var _ = Describe("TLSPolicy controller", func() {
ContainElements(
HaveField("Name", "test-tls-secret"),
))
}, tests.TimeoutMedium, time.Second, ctx).Should(Succeed())
}, tests.TimeoutLong, time.Second, ctx).Should(Succeed())
}, testTimeOut)

})
Expand All @@ -352,7 +352,7 @@ var _ = Describe("TLSPolicy controller", func() {
ContainElements(
HaveField("Name", "test-tls-secret"),
))
}, tests.TimeoutMedium, time.Second, ctx).Should(Succeed())
}, tests.TimeoutLong, time.Second, ctx).Should(Succeed())
}, testTimeOut)
})

Expand Down Expand Up @@ -391,7 +391,7 @@ var _ = Describe("TLSPolicy controller", func() {

Expect(cert1.Spec.DNSNames).To(ConsistOf("test1.example.com", "test2.example.com"))
Expect(cert2.Spec.DNSNames).To(ConsistOf("test3.example.com"))
}, tests.TimeoutMedium, time.Second, ctx).Should(Succeed())
}, tests.TimeoutLong, time.Second, ctx).Should(Succeed())
}, testTimeOut)
})

Expand Down Expand Up @@ -436,11 +436,38 @@ var _ = Describe("TLSPolicy controller", func() {
Expect(cert1.Spec.DNSNames).To(ConsistOf("test1.example.com"))
Expect(cert2.Spec.DNSNames).To(ConsistOf("test2.example.com"))
Expect(cert3.Spec.DNSNames).To(ConsistOf("test3.example.com"))
}, tests.TimeoutMedium, time.Second, ctx).Should(Succeed())
}, tests.TimeoutLong, time.Second, ctx).Should(Succeed())
}, testTimeOut)

It("should delete all tls certificates when policy is deleted", func(ctx SpecContext) {
// confirm all expected certificates are present
Eventually(func() error {
certificateList := &certmanv1.CertificateList{}
Expect(k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace})).To(BeNil())
if len(certificateList.Items) != 3 {
return fmt.Errorf("expected 3 certificates, found: %v", len(certificateList.Items))
}
return nil
}, time.Second*60, time.Second).Should(BeNil())

// delete the tls policy
Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, tlsPolicy))).ToNot(HaveOccurred())

// confirm all certificates have been deleted
Eventually(func() error {
certificateList := &certmanv1.CertificateList{}
if err := k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace}); err != nil {
return err
}
if len(certificateList.Items) != 0 {
return fmt.Errorf("expected 0 certificates, found: %v", len(certificateList.Items))
}
return nil
}, tests.TimeoutLong, time.Second).Should(BeNil())
}, testTimeOut)

It("should delete tls certificate when listener is removed", func(ctx SpecContext) {
//confirm all expected certificates are present
// confirm all expected certificates are present
Eventually(func() error {
certificateList := &certmanv1.CertificateList{}
Expect(k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace})).To(BeNil())
Expand All @@ -450,12 +477,12 @@ var _ = Describe("TLSPolicy controller", func() {
return nil
}, time.Second*60, time.Second).Should(BeNil())

//remove a listener
// remove a listener
patch := client.MergeFrom(gateway.DeepCopy())
gateway.Spec.Listeners = gateway.Spec.Listeners[1:]
Expect(k8sClient.Patch(ctx, gateway, patch)).To(BeNil())

//confirm a certificate has been deleted
// confirm a certificate has been deleted
Eventually(func() error {
certificateList := &certmanv1.CertificateList{}
if err := k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace}); err != nil {
Expand All @@ -465,35 +492,100 @@ var _ = Describe("TLSPolicy controller", func() {
return fmt.Errorf("expected 2 certificates, found: %v", len(certificateList.Items))
}
return nil
}, tests.TimeoutMedium, time.Second).Should(BeNil())
}, tests.TimeoutLong, time.Second).Should(BeNil())
}, testTimeOut)

It("should delete all tls certificates when tls policy is removed even if gateway is already removed", func(ctx SpecContext) {
//confirm all expected certificates are present
It("should delete all tls certificates when gateway is deleted", func(ctx SpecContext) {
// confirm all expected certificates are present
Eventually(func() error {
certificateList := &certmanv1.CertificateList{}
Expect(k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace})).To(BeNil())
if len(certificateList.Items) != 3 {
return fmt.Errorf("expected 3 certificates, found: %v", len(certificateList.Items))
}
return nil
}, time.Second*10, time.Second).Should(BeNil())
}, time.Second*60, time.Second).Should(BeNil())

// delete the gateway
Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, gateway))).ToNot(HaveOccurred())

//delete the tls policy
Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, tlsPolicy))).ToNot(HaveOccurred())

//confirm all certificates have been deleted
// confirm all certificates have been deleted
Eventually(func() error {
certificateList := &certmanv1.CertificateList{}
Expect(k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace})).To(BeNil())
if len(certificateList.Items) != 0 {
return fmt.Errorf("expected 0 certificates, found: %v", len(certificateList.Items))
}
return nil
}, tests.TimeoutLong, time.Second).Should(BeNil())
}, testTimeOut)

It("Should delete orphaned tls certificates when changing to valid target ref", func(ctx SpecContext) {
// confirm all expected certificates are present
Eventually(func() error {
certificateList := &certmanv1.CertificateList{}
Expect(k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace})).To(BeNil())
if len(certificateList.Items) != 3 {
return fmt.Errorf("expected 3 certificates, found: %v", len(certificateList.Items))
}
return nil
}, time.Second*60, time.Second).Should(BeNil())

// new gateway with one listener
gateway2 := tests.NewGatewayBuilder("test-gateway-2", gatewayClass.Name, testNamespace).
WithHTTPSListener("gateway2.example.com", "gateway2-tls-secret").Gateway
Expect(k8sClient.Create(ctx, gateway2)).To(BeNil())

// update tls policy target ref to new gateway
Eventually(func(g Gomega) {
g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(tlsPolicy), tlsPolicy)).To(Succeed())
tlsPolicy.Spec.TargetRef.Name = gatewayapiv1.ObjectName(gateway2.Name)
g.Expect(k8sClient.Update(ctx, tlsPolicy)).To(Succeed())
}).WithContext(ctx).Should(Succeed())

// confirm orphaned certs are deleted
Eventually(func() error {
certificateList := &certmanv1.CertificateList{}
Expect(k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace})).To(BeNil())
if len(certificateList.Items) != 1 {
return fmt.Errorf("expected 1 certificates, found: %v", len(certificateList.Items))
}

if certificateList.Items[0].Name != "gateway2-tls-secret" {
return fmt.Errorf("expected certificate to be 'gateway2-tls-secret', found: %s", certificateList.Items[0].Name)

}
return nil
}, tests.TimeoutLong, time.Second).Should(BeNil())
}, testTimeOut)

It("Should delete orphaned tls certificates when changing to invalid target ref", func(ctx SpecContext) {
// confirm all expected certificates are present
Eventually(func() error {
certificateList := &certmanv1.CertificateList{}
Expect(k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace})).To(BeNil())
if len(certificateList.Items) != 3 {
return fmt.Errorf("expected 3 certificates, found: %v", len(certificateList.Items))
}
return nil
}, time.Second*60, time.Second).Should(BeNil())

// update tlspolicy target ref to invalid reference
Eventually(func(g Gomega) {
g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(tlsPolicy), tlsPolicy)).To(Succeed())
tlsPolicy.Spec.TargetRef.Name = "does-not-exist"
g.Expect(k8sClient.Update(ctx, tlsPolicy)).To(Succeed())
}).WithContext(ctx).Should(Succeed())

// confirm orphaned certs are deleted
Eventually(func() error {
certificateList := &certmanv1.CertificateList{}
Expect(k8sClient.List(ctx, certificateList, &client.ListOptions{Namespace: testNamespace})).To(BeNil())
if len(certificateList.Items) != 0 {
return fmt.Errorf("expected 0 certificates, found: %v", len(certificateList.Items))
}
return nil
}, tests.TimeoutLong, time.Second).Should(BeNil())
}, testTimeOut)
})

Expand Down Expand Up @@ -553,7 +645,7 @@ var _ = Describe("TLSPolicy controller", func() {
certmanv1.UsageCertSign,
))
Expect(cert1.Spec.RevisionHistoryLimit).To(PointTo(Equal(int32(1))))
}, tests.TimeoutMedium, time.Second, ctx).Should(Succeed())
}, tests.TimeoutLong, time.Second, ctx).Should(Succeed())
}, testTimeOut)

})
Expand Down

0 comments on commit 2c1093b

Please sign in to comment.