From 59f8fcd6fe1088726747d4707545bd4e0d256cb5 Mon Sep 17 00:00:00 2001 From: Craig Trought Date: Sun, 11 Jun 2023 16:31:25 -0400 Subject: [PATCH 1/6] feat: support subject annotations on route Signed-off-by: Craig Trought --- README.md | 9 ++ internal/controller/sync.go | 197 ++++++++++++++++++++++++++++-------- 2 files changed, 165 insertions(+), 41 deletions(-) diff --git a/README.md b/README.md index efc851c..5177dd7 100644 --- a/README.md +++ b/README.md @@ -105,6 +105,15 @@ metadata: cert-manager.io/uri-sans: "spiffe://trustdomain/workload" # Optional, no default cert-manager.io/private-key-algorithm: "ECDSA" # Optional, defaults to RSA cert-manager.io/private-key-size: "384" # Optional, defaults to 265 for ECDSA and 2048 for RSA + cert-manager.io/email-sans: "me@example.com,you@example.com" # Optional, no default + cert-manager.io/subject-organizations: "company" # Optional, no default + cert-manager.io/subject-organizationalunits: "company division" # Optional, no default + cert-manager.io/subject-countries: "My Country" # Optional, no default + cert-manager.io/subject-provinces: "My Province" # Optional, no default + cert-manager.io/subject-localities: "My City" # Optional, no default + cert-manager.io/subject-postalcodes: "123ABC" # Optional, no default + cert-manager.io/subject-streetaddresses: "1 Example St" # Optional, no default + cert-manager.io/subject-serialnumber: "123456" # Optional, no default spec: host: app.service.clustername.domain.com # will be added to the Subject Alternative Names of the CertificateRequest port: diff --git a/internal/controller/sync.go b/internal/controller/sync.go index 431d720..745e0de 100644 --- a/internal/controller/sync.go +++ b/internal/controller/sync.go @@ -33,6 +33,7 @@ import ( cmapiutil "github.com/cert-manager/cert-manager/pkg/api/util" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" + cmutil "github.com/cert-manager/cert-manager/pkg/util" utilpki "github.com/cert-manager/cert-manager/pkg/util/pki" routev1 "github.com/openshift/api/route/v1" corev1 "k8s.io/api/core/v1" @@ -42,6 +43,7 @@ import ( const ( ReasonIssuing = `Issuing` + ReasonIssued = `Issued` ReasonInvalidKey = `InvalidKey` ReasonInvalidPrivateKeyAlgorithm = `InvalidPrivateKeyAlgorithm` ReasonInvalidPrivateKeySize = `InvalidPrivateKeySize` @@ -121,7 +123,12 @@ func (r *Route) sync(ctx context.Context, req reconcile.Request, route *routev1. } // Cert is ready. Populate the route. err = r.populateRoute(ctx, route, cr, revision) + if err != nil { + log.V(1).Error(err, "failed to populate route certificate") + return result, err + } log.V(5).Info("populated route cert") + r.eventRecorder.Event(route, corev1.EventTypeNormal, ReasonIssued, "Route updated with issued certificate") return result, err } @@ -352,43 +359,6 @@ func (r *Route) buildNextCR(ctx context.Context, route *routev1.Route, revision return nil, fmt.Errorf("Invalid duration annotation on Route %s/%s", route.Namespace, route.Name) } - var dnsNames []string - // Get the canonical hostname(s) of the Route (from .spec.host or .spec.subdomain) - dnsNames = getRouteHostnames(route) - if len(dnsNames) == 0 { - err := fmt.Errorf("Route is not yet initialized with a hostname") - r.eventRecorder.Event(route, corev1.EventTypeWarning, ReasonMissingHostname, fmt.Sprint(err)) - return nil, err - } - - // Parse out SANs - if metav1.HasAnnotation(route.ObjectMeta, cmapi.AltNamesAnnotationKey) { - altNames := strings.Split(route.Annotations[cmapi.AltNamesAnnotationKey], ",") - dnsNames = append(dnsNames, altNames...) - } - var ipSans []net.IP - if metav1.HasAnnotation(route.ObjectMeta, cmapi.IPSANAnnotationKey) { - ipAddresses := strings.Split(route.Annotations[cmapi.IPSANAnnotationKey], ",") - for _, i := range ipAddresses { - ip := net.ParseIP(i) - if ip != nil { - ipSans = append(ipSans, ip) - } - } - } - var uriSans []*url.URL - if metav1.HasAnnotation(route.ObjectMeta, cmapi.URISANAnnotationKey) { - urls := strings.Split(route.Annotations[cmapi.URISANAnnotationKey], ",") - for _, u := range urls { - ur, err := url.Parse(u) - if err != nil { - r.eventRecorder.Event(route, corev1.EventTypeWarning, ReasonInvalidValue, "Ignoring malformed URI SAN "+u) - continue - } - uriSans = append(uriSans, ur) - } - } - privateKeyAlgorithm, found := route.Annotations[cmapi.PrivateKeyAlgorithmAnnotationKey] if !found { privateKeyAlgorithm = string(cmapi.RSAKeyAlgorithm) @@ -437,6 +407,142 @@ func (r *Route) buildNextCR(ctx context.Context, route *routev1.Route, revision return nil, fmt.Errorf("invalid private key algorithm, %s", privateKeyAlgorithm) } + var dnsNames []string + // Get the canonical hostname(s) of the Route (from .spec.host or .spec.subdomain) + dnsNames = getRouteHostnames(route) + if len(dnsNames) == 0 { + err := fmt.Errorf("Route is not yet initialized with a hostname") + r.eventRecorder.Event(route, corev1.EventTypeWarning, ReasonMissingHostname, fmt.Sprint(err)) + return nil, err + } + + // Parse out SANs + if metav1.HasAnnotation(route.ObjectMeta, cmapi.AltNamesAnnotationKey) { + altNames := strings.Split(route.Annotations[cmapi.AltNamesAnnotationKey], ",") + dnsNames = append(dnsNames, altNames...) + } + var ipSans []net.IP + if metav1.HasAnnotation(route.ObjectMeta, cmapi.IPSANAnnotationKey) { + ipAddresses := strings.Split(route.Annotations[cmapi.IPSANAnnotationKey], ",") + for _, i := range ipAddresses { + ip := net.ParseIP(i) + if ip != nil { + ipSans = append(ipSans, ip) + } + } + } + var uriSans []*url.URL + if metav1.HasAnnotation(route.ObjectMeta, cmapi.URISANAnnotationKey) { + urls := strings.Split(route.Annotations[cmapi.URISANAnnotationKey], ",") + for _, u := range urls { + ur, err := url.Parse(u) + if err != nil { + r.eventRecorder.Event(route, corev1.EventTypeWarning, ReasonInvalidValue, "Ignoring malformed URI SAN "+u) + continue + } + uriSans = append(uriSans, ur) + } + } + var emailAddresses []string + if metav1.HasAnnotation(route.ObjectMeta, cmapi.EmailsAnnotationKey) { + emailAddresses = strings.Split(route.Annotations[cmapi.EmailsAnnotationKey], ",") + } + var organizations []string + if metav1.HasAnnotation(route.ObjectMeta, cmapi.SubjectOrganizationsAnnotationKey) { + subjectOrganizations, err := cmutil.SplitWithEscapeCSV(route.Annotations[cmapi.SubjectOrganizationsAnnotationKey]) + organizations = subjectOrganizations + + if err != nil { + r.log.V(1).Error(err, "the organizations annotation is invalid", + "object", route.Namespace+"/"+route.Name, cmapi.SubjectOrganizationsAnnotationKey, + route.Annotations[cmapi.SubjectOrganizationsAnnotationKey]) + r.eventRecorder.Event(route, corev1.EventTypeWarning, ReasonInvalidValue, "annotation "+cmapi.SubjectOrganizationsAnnotationKey+": "+route.Annotations[cmapi.SubjectOrganizationsAnnotationKey]+" value is malformed") + return nil, err + } + } + var organizationalUnits []string + if metav1.HasAnnotation(route.ObjectMeta, cmapi.SubjectOrganizationalUnitsAnnotationKey) { + subjectOrganizationalUnits, err := cmutil.SplitWithEscapeCSV(route.Annotations[cmapi.SubjectOrganizationalUnitsAnnotationKey]) + organizationalUnits = subjectOrganizationalUnits + + if err != nil { + r.log.V(1).Error(err, "the organizational units annotation is invalid", + "object", route.Namespace+"/"+route.Name, cmapi.SubjectOrganizationalUnitsAnnotationKey, + route.Annotations[cmapi.SubjectOrganizationalUnitsAnnotationKey]) + r.eventRecorder.Event(route, corev1.EventTypeWarning, ReasonInvalidValue, "annotation "+cmapi.SubjectOrganizationalUnitsAnnotationKey+": "+route.Annotations[cmapi.SubjectOrganizationalUnitsAnnotationKey]+" value is malformed") + return nil, err + } + + } + var countries []string + if metav1.HasAnnotation(route.ObjectMeta, cmapi.SubjectCountriesAnnotationKey) { + subjectCountries, err := cmutil.SplitWithEscapeCSV(route.Annotations[cmapi.SubjectCountriesAnnotationKey]) + countries = subjectCountries + + if err != nil { + r.log.V(1).Error(err, "the countries annotation is invalid", + "object", route.Namespace+"/"+route.Name, cmapi.SubjectCountriesAnnotationKey, + route.Annotations[cmapi.SubjectCountriesAnnotationKey]) + r.eventRecorder.Event(route, corev1.EventTypeWarning, ReasonInvalidValue, "annotation "+cmapi.SubjectCountriesAnnotationKey+": "+route.Annotations[cmapi.SubjectCountriesAnnotationKey]+" value is malformed") + return nil, err + } + } + var provinces []string + if metav1.HasAnnotation(route.ObjectMeta, cmapi.SubjectProvincesAnnotationKey) { + subjectProvinces, err := cmutil.SplitWithEscapeCSV(route.Annotations[cmapi.SubjectProvincesAnnotationKey]) + provinces = subjectProvinces + + if err != nil { + r.log.V(1).Error(err, "the provinces annotation is invalid", + "object", route.Namespace+"/"+route.Name, cmapi.SubjectProvincesAnnotationKey, + route.Annotations[cmapi.SubjectProvincesAnnotationKey]) + r.eventRecorder.Event(route, corev1.EventTypeWarning, ReasonInvalidValue, "annotation "+cmapi.SubjectProvincesAnnotationKey+": "+route.Annotations[cmapi.SubjectProvincesAnnotationKey]+" value is malformed") + return nil, err + } + } + var localities []string + if metav1.HasAnnotation(route.ObjectMeta, cmapi.SubjectLocalitiesAnnotationKey) { + subjectLocalities, err := cmutil.SplitWithEscapeCSV(route.Annotations[cmapi.SubjectLocalitiesAnnotationKey]) + localities = subjectLocalities + + if err != nil { + r.log.V(1).Error(err, "the localities annotation is invalid", + "object", route.Namespace+"/"+route.Name, cmapi.SubjectLocalitiesAnnotationKey, + route.Annotations[cmapi.SubjectLocalitiesAnnotationKey]) + r.eventRecorder.Event(route, corev1.EventTypeWarning, ReasonInvalidValue, "annotation "+cmapi.SubjectLocalitiesAnnotationKey+": "+route.Annotations[cmapi.SubjectLocalitiesAnnotationKey]+" value is malformed") + return nil, err + } + } + var postalCodes []string + if metav1.HasAnnotation(route.ObjectMeta, cmapi.SubjectPostalCodesAnnotationKey) { + subjectPostalCodes, err := cmutil.SplitWithEscapeCSV(route.Annotations[cmapi.SubjectPostalCodesAnnotationKey]) + postalCodes = subjectPostalCodes + + if err != nil { + r.log.V(1).Error(err, "the postal codes annotation is invalid", + "object", route.Namespace+"/"+route.Name, cmapi.SubjectPostalCodesAnnotationKey, + route.Annotations[cmapi.SubjectPostalCodesAnnotationKey]) + r.eventRecorder.Event(route, corev1.EventTypeWarning, ReasonInvalidValue, "annotation "+cmapi.SubjectPostalCodesAnnotationKey+": "+route.Annotations[cmapi.SubjectPostalCodesAnnotationKey]+" value is malformed") + return nil, err + } + } + var streetAddresses []string + if metav1.HasAnnotation(route.ObjectMeta, cmapi.SubjectStreetAddressesAnnotationKey) { + subjectStreetAddresses, err := cmutil.SplitWithEscapeCSV(route.Annotations[cmapi.SubjectStreetAddressesAnnotationKey]) + streetAddresses = subjectStreetAddresses + + if err != nil { + r.log.V(1).Error(err, "the street addresses annotation is invalid", + "object", route.Namespace+"/"+route.Name, cmapi.SubjectStreetAddressesAnnotationKey, + route.Annotations[cmapi.SubjectStreetAddressesAnnotationKey]) + r.eventRecorder.Event(route, corev1.EventTypeWarning, ReasonInvalidValue, "annotation "+cmapi.SubjectStreetAddressesAnnotationKey+": "+route.Annotations[cmapi.SubjectStreetAddressesAnnotationKey]+" value is malformed") + return nil, err + } + } + var serialNumber string + if metav1.HasAnnotation(route.ObjectMeta, cmapi.SubjectSerialNumberAnnotationKey) { + serialNumber = route.Annotations[cmapi.SubjectStreetAddressesAnnotationKey] + } csr, err := x509.CreateCertificateRequest( rand.Reader, &x509.CertificateRequest{ @@ -444,11 +550,20 @@ func (r *Route) buildNextCR(ctx context.Context, route *routev1.Route, revision SignatureAlgorithm: signatureAlgorithm, PublicKeyAlgorithm: publicKeyAlgorithm, Subject: pkix.Name{ - CommonName: route.Annotations[cmapi.CommonNameAnnotationKey], + CommonName: route.Annotations[cmapi.CommonNameAnnotationKey], + Country: countries, + Locality: localities, + Organization: organizations, + OrganizationalUnit: organizationalUnits, + PostalCode: postalCodes, + Province: provinces, + SerialNumber: serialNumber, + StreetAddress: streetAddresses, }, - DNSNames: dnsNames, - IPAddresses: ipSans, - URIs: uriSans, + EmailAddresses: emailAddresses, + DNSNames: dnsNames, + IPAddresses: ipSans, + URIs: uriSans, }, key, ) From edea400ac0bb1132dc7387067bf68cf095ae42b9 Mon Sep 17 00:00:00 2001 From: Craig Trought Date: Sun, 11 Jun 2023 16:33:57 -0400 Subject: [PATCH 2/6] fix: support ingress issuer name annotation Signed-off-by: Craig Trought --- internal/controller/controller.go | 3 +++ internal/controller/sync.go | 8 +++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/internal/controller/controller.go b/internal/controller/controller.go index 66119cd..66983c7 100644 --- a/internal/controller/controller.go +++ b/internal/controller/controller.go @@ -57,6 +57,9 @@ func (r *Route) Reconcile(ctx context.Context, req reconcile.Request) (reconcile if metav1.HasAnnotation(route.ObjectMeta, cmapi.IssuerNameAnnotationKey) { log.V(5).Info("route has cert-manager annotation, reconciling", cmapi.IssuerNameAnnotationKey, route.Annotations[cmapi.IssuerNameAnnotationKey]) return r.sync(ctx, req, route.DeepCopy()) + } else if metav1.HasAnnotation(route.ObjectMeta, cmapi.IngressIssuerNameAnnotationKey) { + log.V(5).Info("route has cert-manager annotation, reconciling", cmapi.IngressIssuerNameAnnotationKey, route.Annotations[cmapi.IngressIssuerNameAnnotationKey]) + return r.sync(ctx, req, route.DeepCopy()) } log.V(5).Info("ignoring route without cert-manager issuer name annotation") return reconcile.Result{}, nil diff --git a/internal/controller/sync.go b/internal/controller/sync.go index 745e0de..8d3b07b 100644 --- a/internal/controller/sync.go +++ b/internal/controller/sync.go @@ -575,6 +575,12 @@ func (r *Route) buildNextCR(ctx context.Context, route *routev1.Route, revision Bytes: csr, }) + var issuerName string + if metav1.HasAnnotation(route.ObjectMeta, cmapi.IngressIssuerNameAnnotationKey) { + issuerName = route.Annotations[cmapi.IssuerNameAnnotationKey] + } else { + issuerName = route.Annotations[cmapi.IngressIssuerNameAnnotationKey] + } cr := &cmapi.CertificateRequest{ ObjectMeta: metav1.ObjectMeta{ GenerateName: route.Name + "-", @@ -590,7 +596,7 @@ func (r *Route) buildNextCR(ctx context.Context, route *routev1.Route, revision Spec: cmapi.CertificateRequestSpec{ Duration: &metav1.Duration{Duration: duration}, IssuerRef: cmmeta.ObjectReference{ - Name: route.Annotations[cmapi.IssuerNameAnnotationKey], + Name: issuerName, Kind: route.Annotations[cmapi.IssuerKindAnnotationKey], Group: route.Annotations[cmapi.IssuerGroupAnnotationKey], }, From c621c0e92e99a5ac916ced62e698ab4c835d95e0 Mon Sep 17 00:00:00 2001 From: Craig Trought Date: Sun, 11 Jun 2023 16:37:20 -0400 Subject: [PATCH 3/6] fix: ignore routes owned by ingress Signed-off-by: Craig Trought --- internal/controller/controller.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/controller/controller.go b/internal/controller/controller.go index 66983c7..66b56e6 100644 --- a/internal/controller/controller.go +++ b/internal/controller/controller.go @@ -53,6 +53,14 @@ func (r *Route) Reconcile(ctx context.Context, req reconcile.Request) (reconcile if err != nil { return reconcile.Result{}, err } + if len(route.ObjectMeta.OwnerReferences) > 0 { + for _, o := range route.ObjectMeta.OwnerReferences { + if o.Kind == "Ingress" { + log.V(5).Info("ignoring route owned by ingress", o.Name) + return reconcile.Result{}, nil + } + } + } log.V(5).Info("retrieved route") if metav1.HasAnnotation(route.ObjectMeta, cmapi.IssuerNameAnnotationKey) { log.V(5).Info("route has cert-manager annotation, reconciling", cmapi.IssuerNameAnnotationKey, route.Annotations[cmapi.IssuerNameAnnotationKey]) From c84ea353d8c17a0509c8e05ed98ec6782203b36a Mon Sep 17 00:00:00 2001 From: Craig Trought Date: Wed, 13 Sep 2023 21:25:25 -0400 Subject: [PATCH 4/6] fix: annotation key copy paste error Signed-off-by: Craig Trought --- internal/controller/sync.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/internal/controller/sync.go b/internal/controller/sync.go index 8d3b07b..d55904d 100644 --- a/internal/controller/sync.go +++ b/internal/controller/sync.go @@ -102,7 +102,6 @@ func (r *Route) sync(ctx context.Context, req reconcile.Request, route *routev1. // Not a reconcile error, so don't retry this revision return result, nil } - // create CR and return. We own the CR so it will cause a re-reconcile _, err = r.certClient.CertmanagerV1().CertificateRequests(route.Namespace).Create(ctx, cr, metav1.CreateOptions{}) if err != nil { @@ -110,7 +109,6 @@ func (r *Route) sync(ctx context.Context, req reconcile.Request, route *routev1. } r.eventRecorder.Event(route, corev1.EventTypeNormal, ReasonIssuing, "Created new CertificateRequest for Route %s") return result, nil - } // is the CR Ready and Approved? ready, cr, err := r.certificateRequestReadyAndApproved(ctx, route, revision) @@ -541,7 +539,7 @@ func (r *Route) buildNextCR(ctx context.Context, route *routev1.Route, revision } var serialNumber string if metav1.HasAnnotation(route.ObjectMeta, cmapi.SubjectSerialNumberAnnotationKey) { - serialNumber = route.Annotations[cmapi.SubjectStreetAddressesAnnotationKey] + serialNumber = route.Annotations[cmapi.SubjectSerialNumberAnnotationKey] } csr, err := x509.CreateCertificateRequest( rand.Reader, @@ -577,10 +575,11 @@ func (r *Route) buildNextCR(ctx context.Context, route *routev1.Route, revision var issuerName string if metav1.HasAnnotation(route.ObjectMeta, cmapi.IngressIssuerNameAnnotationKey) { - issuerName = route.Annotations[cmapi.IssuerNameAnnotationKey] - } else { issuerName = route.Annotations[cmapi.IngressIssuerNameAnnotationKey] + } else { + issuerName = route.Annotations[cmapi.IssuerNameAnnotationKey] } + cr := &cmapi.CertificateRequest{ ObjectMeta: metav1.ObjectMeta{ GenerateName: route.Name + "-", From 084400743060f700bff26f21983fe592a1055f49 Mon Sep 17 00:00:00 2001 From: Craig Trought Date: Wed, 13 Sep 2023 21:25:58 -0400 Subject: [PATCH 5/6] test: additional buildNextCR tests including subject annotations Signed-off-by: Craig Trought --- internal/controller/sync_test.go | 307 +++++++++++++++++++++++++++++++ 1 file changed, 307 insertions(+) diff --git a/internal/controller/sync_test.go b/internal/controller/sync_test.go index 6c4b311..641bb6a 100644 --- a/internal/controller/sync_test.go +++ b/internal/controller/sync_test.go @@ -35,6 +35,7 @@ import ( "time" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" utilpki "github.com/cert-manager/cert-manager/pkg/util/pki" routev1 "github.com/openshift/api/route/v1" fakeroutev1client "github.com/openshift/client-go/route/clientset/versioned/fake" @@ -731,6 +732,166 @@ func TestRoute_buildNextCR(t *testing.T) { }, wantErr: nil, }, + { + name: "Basic test with issuer", + revision: 1337, + route: generateRouteStatus(&routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-route", + Namespace: "some-namespace", + Annotations: map[string]string{ + cmapi.DurationAnnotationKey: "42m", + cmapi.IsNextPrivateKeySecretLabelKey: string(rsaPEM), + cmapi.IssuerNameAnnotationKey: "self-signed-issuer", + cmapi.IssuerKindAnnotationKey: "Issuer", + }, + }, + Spec: routev1.RouteSpec{ + Host: "some-host.some-domain.tld", + }, + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + Host: "some-host.some-domain.tld", + Conditions: []routev1.RouteIngressCondition{ + { + Type: "Admitted", + Status: "True", + }, + }, + }, + }, + }, + }, + true), + want: &cmapi.CertificateRequest{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "some-route-", + Namespace: "some-namespace", + Annotations: map[string]string{ + cmapi.CertificateRequestRevisionAnnotationKey: "1338", + }, + }, + Spec: cmapi.CertificateRequestSpec{ + Duration: &metav1.Duration{Duration: 42 * time.Minute}, + IsCA: false, + Usages: []cmapi.KeyUsage{cmapi.UsageServerAuth, cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment}, + IssuerRef: cmmeta.ObjectReference{ + Name: "self-signed-issuer", + Kind: "Issuer", + }, + }, + }, + wantErr: nil, + }, + { + name: "Basic test with external issuer", + revision: 1337, + route: generateRouteStatus(&routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-route", + Namespace: "some-namespace", + Annotations: map[string]string{ + cmapi.DurationAnnotationKey: "42m", + cmapi.IsNextPrivateKeySecretLabelKey: string(rsaPEM), + cmapi.IssuerKindAnnotationKey: "Issuer", + cmapi.IssuerNameAnnotationKey: "self-signed-issuer", + cmapi.IssuerGroupAnnotationKey: "external-issuer.io", + }, + }, + Spec: routev1.RouteSpec{ + Host: "some-host.some-domain.tld", + }, + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + Host: "some-host.some-domain.tld", + Conditions: []routev1.RouteIngressCondition{ + { + Type: "Admitted", + Status: "True", + }, + }, + }, + }, + }, + }, + true), + want: &cmapi.CertificateRequest{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "some-route-", + Namespace: "some-namespace", + Annotations: map[string]string{ + cmapi.CertificateRequestRevisionAnnotationKey: "1338", + }, + }, + Spec: cmapi.CertificateRequestSpec{ + Duration: &metav1.Duration{Duration: 42 * time.Minute}, + IsCA: false, + Usages: []cmapi.KeyUsage{cmapi.UsageServerAuth, cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment}, + IssuerRef: cmmeta.ObjectReference{ + Name: "self-signed-issuer", + Kind: "Issuer", + Group: "external-issuer.io", + }, + }, + }, + wantErr: nil, + }, + { + name: "Basic test with alternate ingress issuer name annotation", + revision: 1337, + route: generateRouteStatus(&routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-route", + Namespace: "some-namespace", + Annotations: map[string]string{ + cmapi.DurationAnnotationKey: "42m", + cmapi.IsNextPrivateKeySecretLabelKey: string(rsaPEM), + cmapi.IssuerKindAnnotationKey: "Issuer", + cmapi.IngressIssuerNameAnnotationKey: "self-signed-issuer", + cmapi.IssuerGroupAnnotationKey: "external-issuer.io", + }, + }, + Spec: routev1.RouteSpec{ + Host: "some-host.some-domain.tld", + }, + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + Host: "some-host.some-domain.tld", + Conditions: []routev1.RouteIngressCondition{ + { + Type: "Admitted", + Status: "True", + }, + }, + }, + }, + }, + }, + true), + want: &cmapi.CertificateRequest{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "some-route-", + Namespace: "some-namespace", + Annotations: map[string]string{ + cmapi.CertificateRequestRevisionAnnotationKey: "1338", + }, + }, + Spec: cmapi.CertificateRequestSpec{ + Duration: &metav1.Duration{Duration: 42 * time.Minute}, + IsCA: false, + Usages: []cmapi.KeyUsage{cmapi.UsageServerAuth, cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment}, + IssuerRef: cmmeta.ObjectReference{ + Name: "self-signed-issuer", + Kind: "Issuer", + Group: "external-issuer.io", + }, + }, + }, + wantErr: nil, + }, { name: "With subdomain and multiple ICs", revision: 1337, @@ -1136,6 +1297,151 @@ func TestRoute_buildNextCR(t *testing.T) { }, wantErr: nil, }, + { + name: "With subject annotations", + revision: 1337, + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-route-with-subject-annotations", + Namespace: "some-namespace", + Annotations: map[string]string{ + cmapi.IsNextPrivateKeySecretLabelKey: string(rsaPEM), + cmapi.SubjectOrganizationsAnnotationKey: "Company 1,Company 2", + cmapi.SubjectOrganizationalUnitsAnnotationKey: "Tech Division,Other Division", + cmapi.SubjectCountriesAnnotationKey: "Country 1,Country 2", + cmapi.SubjectProvincesAnnotationKey: "Province 1,Province 2", + cmapi.SubjectStreetAddressesAnnotationKey: "123 Example St,456 Example Ave", + cmapi.SubjectLocalitiesAnnotationKey: "City 1,City 2", + cmapi.SubjectPostalCodesAnnotationKey: "123ABC,456DEF", + cmapi.SubjectSerialNumberAnnotationKey: "10978342379280287615", + }, + }, + Spec: routev1.RouteSpec{ + Host: "example-route.example.com", + }, + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + Host: "example-route.example.com", + Conditions: []routev1.RouteIngressCondition{ + { + Type: "Admitted", + Status: "True", + }, + }, + }, + }, + }, + }, + want: &cmapi.CertificateRequest{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "some-route-with-subject-annotations-", + Namespace: "some-namespace", + Annotations: map[string]string{ + cmapi.CertificateRequestRevisionAnnotationKey: "1338", + }, + }, + Spec: cmapi.CertificateRequestSpec{ + Duration: &metav1.Duration{Duration: DefaultCertificateDuration}, + Usages: []cmapi.KeyUsage{cmapi.UsageServerAuth, cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment}, + }, + }, + wantCSR: &x509.CertificateRequest{ + SignatureAlgorithm: x509.SHA256WithRSA, + PublicKeyAlgorithm: x509.RSA, + Subject: pkix.Name{ + CommonName: "", + Organization: []string{"Company 1", "Company 2"}, + OrganizationalUnit: []string{"Tech Division", "Other Division"}, + Country: []string{"Country 1", "Country 2"}, + Province: []string{"Province 1", "Province 2"}, + Locality: []string{"City 1", "City 2"}, + PostalCode: []string{"123ABC", "456DEF"}, + StreetAddress: []string{"123 Example St", "456 Example Ave"}, + SerialNumber: "10978342379280287615", + }, + DNSNames: []string{"example-route.example.com"}, + IPAddresses: []net.IP{}, + URIs: []*url.URL{}, + }, + wantErr: nil, + }, + { + name: "With all annotations", + revision: 1337, + route: &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-route-with-all-annotations", + Namespace: "some-namespace", + Annotations: map[string]string{ + cmapi.IsNextPrivateKeySecretLabelKey: string(rsaPEM), + cmapi.DurationAnnotationKey: "720h", + cmapi.IPSANAnnotationKey: "10.20.30.40,192.168.192.168", + cmapi.AltNamesAnnotationKey: "mycooldomain.com,mysecondarydomain.com", + cmapi.URISANAnnotationKey: "spiffe://trustdomain/workload", + cmapi.CommonNameAnnotationKey: "mycommonname.com", + cmapi.EmailsAnnotationKey: "email@example.com", + cmapi.SubjectOrganizationsAnnotationKey: "Company 1,Company 2", + cmapi.SubjectOrganizationalUnitsAnnotationKey: "Tech Division,Other Division", + cmapi.SubjectCountriesAnnotationKey: "Country 1,Country 2", + cmapi.SubjectProvincesAnnotationKey: "Province 1,Province 2", + cmapi.SubjectStreetAddressesAnnotationKey: "123 Example St,456 Example Ave", + cmapi.SubjectLocalitiesAnnotationKey: "City 1,City 2", + cmapi.SubjectPostalCodesAnnotationKey: "123ABC,456DEF", + cmapi.SubjectSerialNumberAnnotationKey: "10978342379280287615", + }, + }, + Spec: routev1.RouteSpec{ + Host: "example-route.example.com", + }, + Status: routev1.RouteStatus{ + Ingress: []routev1.RouteIngress{ + { + Host: "example-route.example.com", + Conditions: []routev1.RouteIngressCondition{ + { + Type: "Admitted", + Status: "True", + }, + }, + }, + }, + }, + }, + want: &cmapi.CertificateRequest{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "some-route-with-all-annotations-", + Namespace: "some-namespace", + Annotations: map[string]string{ + cmapi.CertificateRequestRevisionAnnotationKey: "1338", + }, + }, + Spec: cmapi.CertificateRequestSpec{ + Duration: &metav1.Duration{Duration: time.Hour * 24 * 30}, + Usages: []cmapi.KeyUsage{cmapi.UsageServerAuth, cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment}, + }, + }, + wantCSR: &x509.CertificateRequest{ + SignatureAlgorithm: x509.SHA256WithRSA, + PublicKeyAlgorithm: x509.RSA, + Subject: pkix.Name{ + CommonName: "mycommonname.com", + Organization: []string{"Company 1", "Company 2"}, + OrganizationalUnit: []string{"Tech Division", "Other Division"}, + Country: []string{"Country 1", "Country 2"}, + Province: []string{"Province 1", "Province 2"}, + Locality: []string{"City 1", "City 2"}, + PostalCode: []string{"123ABC", "456DEF"}, + StreetAddress: []string{"123 Example St", "456 Example Ave"}, + SerialNumber: "10978342379280287615", + }, + DNSNames: []string{"example-route.example.com", "mycooldomain.com", "mysecondarydomain.com"}, + IPAddresses: []net.IP{net.IPv4(10, 20, 30, 40), net.IPv4(192, 168, 192, 168)}, + URIs: []*url.URL{{Scheme: "spiffe", Host: "trustdomain", Path: "workload"}}, + EmailAddresses: []string{"email@example.com"}, + }, + wantErr: nil, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1157,6 +1463,7 @@ func TestRoute_buildNextCR(t *testing.T) { assert.Equal(t, tt.want.Spec.Duration, cr.Spec.Duration) assert.Equal(t, tt.want.Spec.IsCA, cr.Spec.IsCA) assert.Equal(t, tt.want.Spec.Usages, cr.Spec.Usages) + assert.Equal(t, tt.want.Spec.IssuerRef, cr.Spec.IssuerRef) // check the CSR if tt.wantCSR != nil { From e1cc7e6b71b5e4063944ca76c507a21204e2438b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Fri, 15 Dec 2023 11:37:30 +0100 Subject: [PATCH 6/6] add tests around whether or not a sync should happen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maël Valais --- internal/controller/controller.go | 45 +++++++++----- internal/controller/controller_test.go | 85 ++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 16 deletions(-) create mode 100644 internal/controller/controller_test.go diff --git a/internal/controller/controller.go b/internal/controller/controller.go index 66b56e6..005a80b 100644 --- a/internal/controller/controller.go +++ b/internal/controller/controller.go @@ -43,6 +43,30 @@ type Route struct { log logr.Logger } +func shouldSync(log logr.Logger, route *routev1.Route) bool { + if len(route.ObjectMeta.OwnerReferences) > 0 { + for _, o := range route.ObjectMeta.OwnerReferences { + if o.Kind == "Ingress" { + log.V(5).Info("Route is owned by an Ingress") + return false + } + } + } + + if metav1.HasAnnotation(route.ObjectMeta, cmapi.IssuerNameAnnotationKey) { + log.V(5).Info("Route has the annotation %s=%s", cmapi.IssuerNameAnnotationKey, route.Annotations[cmapi.IssuerNameAnnotationKey]) + return true + } + + if metav1.HasAnnotation(route.ObjectMeta, cmapi.IngressIssuerNameAnnotationKey) { + log.V(5).Info("Route has the annotation %s=%s", cmapi.IngressIssuerNameAnnotationKey, route.Annotations[cmapi.IngressIssuerNameAnnotationKey]) + return true + } + + log.V(5).Info("Route does not have the cert-manager issuer annotation") + return false +} + func (r *Route) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { log := r.log.WithValues("object", req.NamespacedName) log.V(5).Info("started reconciling") @@ -53,24 +77,13 @@ func (r *Route) Reconcile(ctx context.Context, req reconcile.Request) (reconcile if err != nil { return reconcile.Result{}, err } - if len(route.ObjectMeta.OwnerReferences) > 0 { - for _, o := range route.ObjectMeta.OwnerReferences { - if o.Kind == "Ingress" { - log.V(5).Info("ignoring route owned by ingress", o.Name) - return reconcile.Result{}, nil - } - } - } log.V(5).Info("retrieved route") - if metav1.HasAnnotation(route.ObjectMeta, cmapi.IssuerNameAnnotationKey) { - log.V(5).Info("route has cert-manager annotation, reconciling", cmapi.IssuerNameAnnotationKey, route.Annotations[cmapi.IssuerNameAnnotationKey]) - return r.sync(ctx, req, route.DeepCopy()) - } else if metav1.HasAnnotation(route.ObjectMeta, cmapi.IngressIssuerNameAnnotationKey) { - log.V(5).Info("route has cert-manager annotation, reconciling", cmapi.IngressIssuerNameAnnotationKey, route.Annotations[cmapi.IngressIssuerNameAnnotationKey]) - return r.sync(ctx, req, route.DeepCopy()) + + if !shouldSync(log, route) { + return reconcile.Result{}, nil } - log.V(5).Info("ignoring route without cert-manager issuer name annotation") - return reconcile.Result{}, nil + + return r.sync(ctx, req, route.DeepCopy()) } func New(base logr.Logger, config *rest.Config, recorder record.EventRecorder) (*Route, error) { diff --git a/internal/controller/controller_test.go b/internal/controller/controller_test.go new file mode 100644 index 0000000..0e709a2 --- /dev/null +++ b/internal/controller/controller_test.go @@ -0,0 +1,85 @@ +/* +Copyright 2022 The cert-manager Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "testing" + + "github.com/go-logr/logr" + routev1 "github.com/openshift/api/route/v1" + "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func Test_shouldReconcile(t *testing.T) { + tests := []struct { + name string + given *routev1.Route + want bool + }{ + { + name: "should reconcile with cert-manager.io/issuer-name annotation", + given: &routev1.Route{ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "cert-manager.io/issuer-name": "test", + }}, + }, + want: true, + }, + { + name: "should sync with cert-manager.io/issuer annotation", + given: &routev1.Route{ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "cert-manager.io/issuer": "test", + }}, + }, + want: true, + }, + { + name: "should not sync when Route owned by Ingress", + given: &routev1.Route{ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Ingress", + }, + }}, + }, + want: false, + }, + { + name: "should not sync when Route owned by Ingress", + given: &routev1.Route{ObjectMeta: metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "Ingress", + }, + }}, + }, + want: false, + }, + { + name: "should not sync when no annotation is found", + given: &routev1.Route{ObjectMeta: metav1.ObjectMeta{}}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := shouldSync(logr.Discard(), tt.given) + assert.Equal(t, tt.want, got) + }) + } +}