From 30af2057772cf570c27cac6c14161ff8ca6edd74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Tue, 3 Aug 2021 15:05:43 +0200 Subject: [PATCH] nil pointer: the Gateway API is full of pointers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maël Valais --- pkg/controller/certificate-shim/sync.go | 50 +++---- pkg/controller/certificate-shim/sync_test.go | 145 +++++++++++++++++++ 2 files changed, 169 insertions(+), 26 deletions(-) diff --git a/pkg/controller/certificate-shim/sync.go b/pkg/controller/certificate-shim/sync.go index c3163438b..a37293b8d 100644 --- a/pkg/controller/certificate-shim/sync.go +++ b/pkg/controller/certificate-shim/sync.go @@ -136,17 +136,18 @@ func SyncFnFor( rec.Eventf(ingLikeObj, corev1.EventTypeNormal, reasonUpdateCertificate, "Successfully updated Certificate %q", crt.Name) } - unrequiredCrts, err := findUnrequiredCertificates(cmLister, ingLike) + certs, err := cmLister.Certificates(ingLike.GetNamespace()).List(labels.Everything()) if err != nil { return err } + unrequiredCertNames := findCertificatesToBeRemoved(certs, ingLike) - for _, crt := range unrequiredCrts { - err = cmClient.CertmanagerV1().Certificates(crt.Namespace).Delete(ctx, crt.Name, metav1.DeleteOptions{}) + for _, certName := range unrequiredCertNames { + err = cmClient.CertmanagerV1().Certificates(ingLike.GetNamespace()).Delete(ctx, certName, metav1.DeleteOptions{}) if err != nil { return err } - rec.Eventf(ingLikeObj, corev1.EventTypeNormal, reasonDeleteCertificate, "Successfully deleted unrequired Certificate %q", crt.Name) + rec.Eventf(ingLikeObj, corev1.EventTypeNormal, reasonDeleteCertificate, "Successfully deleted unrequired Certificate %q", certName) } return nil @@ -395,42 +396,39 @@ func buildCertificates( return newCrts, updateCrts, nil } -func findUnrequiredCertificates(list cmlisters.CertificateLister, ingLike metav1.Object) ([]*cmapi.Certificate, error) { - crts, err := list.Certificates(ingLike.GetNamespace()).List(labels.Everything()) - if err != nil { - return nil, err - } - - var unrequired []*cmapi.Certificate - for _, crt := range crts { - if isUnrequiredCertificate(crt, ingLike) { - unrequired = append(unrequired, crt) +func findCertificatesToBeRemoved(certs []*cmapi.Certificate, ingLike metav1.Object) []string { + var toBeRemoved []string + for _, crt := range certs { + if !metav1.IsControlledBy(crt, ingLike) { + continue + } + if !secretNameUsedIn(crt.Spec.SecretName, ingLike) { + toBeRemoved = append(toBeRemoved, crt.Name) } } - - return unrequired, nil + return toBeRemoved } -func isUnrequiredCertificate(crt *cmapi.Certificate, ingLike metav1.Object) bool { - if !metav1.IsControlledBy(crt, ingLike) { - return false - } - +func secretNameUsedIn(secretName string, ingLike metav1.Object) bool { switch o := ingLike.(type) { case *networkingv1.Ingress: for _, tls := range o.Spec.TLS { - if crt.Spec.SecretName == tls.SecretName { - return false + if secretName == tls.SecretName { + return true } } case *gwapi.Gateway: for _, l := range o.Spec.Listeners { - if crt.Spec.SecretName == l.TLS.CertificateRef.Name { - return false + if l.TLS == nil || l.TLS.CertificateRef == nil { + continue + } + if secretName == l.TLS.CertificateRef.Name { + return true } } } - return true + + return false } // certNeedsUpdate checks and returns true if two Certificates differ. diff --git a/pkg/controller/certificate-shim/sync_test.go b/pkg/controller/certificate-shim/sync_test.go index 7973ccef2..8aa774e00 100644 --- a/pkg/controller/certificate-shim/sync_test.go +++ b/pkg/controller/certificate-shim/sync_test.go @@ -2610,6 +2610,7 @@ func buildIngressOwnerReferences(name, namespace string) []metav1.OwnerReference } } +// The Gateway name and UID are set to the same. func buildGatewayOwnerReferences(name, namespace string) []metav1.OwnerReference { return []metav1.OwnerReference{ *metav1.NewControllerRef(buildIngress(name, namespace, nil), gatewayGVK), @@ -2720,3 +2721,147 @@ func Test_validateGatewayListenerBlock(t *testing.T) { }) } } + +func Test_findCertificatesToBeRemoved(t *testing.T) { + tests := []struct { + name string + givenCerts []*cmapi.Certificate + ingLike metav1.Object + wantToBeRemoved []string + }{ + { + name: "should not remove Certificate when not owned by the Ingress", + givenCerts: []*cmapi.Certificate{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cert-1", + Namespace: "default", + OwnerReferences: buildGatewayOwnerReferences("ingress-1", "default"), + }, Spec: cmapi.CertificateSpec{ + SecretName: "secret-name", + }}, + }, + ingLike: &networkingv1.Ingress{ + ObjectMeta: metav1.ObjectMeta{Name: "ingress-2", Namespace: "default", UID: "ingress-2"}, + Spec: networkingv1.IngressSpec{TLS: []networkingv1.IngressTLS{{SecretName: "secret-name"}}}, + }, + wantToBeRemoved: nil, + }, + { + name: "should not remove Certificate when Ingress references the secretName of the Certificate", + givenCerts: []*cmapi.Certificate{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cert-1", + Namespace: "default", + OwnerReferences: buildGatewayOwnerReferences("ingress-1", "default"), + }, Spec: cmapi.CertificateSpec{ + SecretName: "secret-name", + }}, + }, + ingLike: &networkingv1.Ingress{ + ObjectMeta: metav1.ObjectMeta{Name: "ingress-1", Namespace: "default", UID: "ingress-1"}, + Spec: networkingv1.IngressSpec{TLS: []networkingv1.IngressTLS{{SecretName: "secret-name"}}}, + }, + wantToBeRemoved: nil, + }, + { + name: "should remove Certificate when Ingress does not reference the secretName of the Certificate", + givenCerts: []*cmapi.Certificate{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cert-1", + Namespace: "default", + OwnerReferences: buildGatewayOwnerReferences("ingress-1", "default"), + }, Spec: cmapi.CertificateSpec{ + SecretName: "secret-name", + }}, + }, + ingLike: &networkingv1.Ingress{ + ObjectMeta: metav1.ObjectMeta{Name: "ingress-1", Namespace: "default", UID: "ingress-1"}, + }, + wantToBeRemoved: []string{"cert-1"}, + }, + { + name: "should not remove Certificate when not owned by the Gateway", + givenCerts: []*cmapi.Certificate{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cert-1", + Namespace: "default", + OwnerReferences: buildGatewayOwnerReferences("gw-1", "default"), + }, Spec: cmapi.CertificateSpec{ + SecretName: "secret-name", + }}, + }, + ingLike: &gwapi.Gateway{ + ObjectMeta: metav1.ObjectMeta{Name: "gw-2", Namespace: "default", UID: "gw-2"}, + Spec: gwapi.GatewaySpec{Listeners: []gwapi.Listener{{ + TLS: &gwapi.GatewayTLSConfig{CertificateRef: &gwapi.LocalObjectReference{Name: "secret-name"}}, + }}}, + }, + wantToBeRemoved: nil, + }, + { + name: "should remove Certificate when Gateway does not reference the secretName of the Certificate in one of its listers", + givenCerts: []*cmapi.Certificate{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cert-1", + Namespace: "default", + OwnerReferences: buildGatewayOwnerReferences("gw-1", "default"), + }, Spec: cmapi.CertificateSpec{ + SecretName: "secret-name", + }}, + }, + ingLike: &gwapi.Gateway{ + ObjectMeta: metav1.ObjectMeta{Name: "gw-1", Namespace: "default", UID: "gw-1"}, + Spec: gwapi.GatewaySpec{Listeners: []gwapi.Listener{ + {TLS: &gwapi.GatewayTLSConfig{CertificateRef: &gwapi.LocalObjectReference{Name: "not-secret-name"}}}, + }}, + }, + wantToBeRemoved: []string{"cert-1"}, + }, + { + name: "should not remove Certificate when the Gateway references the secretName of the Certificate in one of its listers", + givenCerts: []*cmapi.Certificate{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cert-1", + Namespace: "default", + OwnerReferences: buildGatewayOwnerReferences("gw-1", "default"), + }, Spec: cmapi.CertificateSpec{ + SecretName: "secret-name", + }}, + }, + ingLike: &gwapi.Gateway{ + ObjectMeta: metav1.ObjectMeta{Name: "gw-1", Namespace: "default", UID: "gw-1"}, + Spec: gwapi.GatewaySpec{Listeners: []gwapi.Listener{ + {TLS: &gwapi.GatewayTLSConfig{CertificateRef: &gwapi.LocalObjectReference{Name: "secret-name"}}}, + }}, + }, + wantToBeRemoved: nil, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + gotCerts := findCertificatesToBeRemoved(test.givenCerts, test.ingLike) + assert.Equal(t, test.wantToBeRemoved, gotCerts) + }) + } +} + +func Test_secretNameUsedIn_nilPointerGateway(t *testing.T) { + got := secretNameUsedIn("secret-name", &gwapi.Gateway{ + ObjectMeta: metav1.ObjectMeta{Name: "gw-1", Namespace: "default", UID: "gw-1"}, + Spec: gwapi.GatewaySpec{Listeners: []gwapi.Listener{ + {TLS: nil}, + {TLS: &gwapi.GatewayTLSConfig{CertificateRef: nil}}, + {TLS: &gwapi.GatewayTLSConfig{CertificateRef: &gwapi.LocalObjectReference{Name: "secret-name"}}}, + }}, + }) + assert.Equal(t, true, got) + + got = secretNameUsedIn("secret-name", &gwapi.Gateway{ + ObjectMeta: metav1.ObjectMeta{Name: "gw-1", Namespace: "default", UID: "gw-1"}, + Spec: gwapi.GatewaySpec{Listeners: []gwapi.Listener{ + {TLS: nil}, + {TLS: &gwapi.GatewayTLSConfig{CertificateRef: nil}}, + }}, + }) + assert.Equal(t, false, got) +}