From 4260fc1336f0cc69fef20bf50bdd5db74721f4c2 Mon Sep 17 00:00:00 2001 From: Ben Bettridge Date: Thu, 12 Apr 2018 23:26:40 +1200 Subject: [PATCH 1/3] Add support for annotation 'certmanager.k8s.io/ingress-class' to toggle the use of ingressClass: Add annotation to the ingress-shim documentation Remove debug output. Update documentation errors. Implement suggestions of using edit-in-place annotation to control behaviour. Fix reference to editInPlaceAnnotation Remove the presence of editInPlaceAnnotation from returning true to shouldSync() and relevant test. Update comment reference to correct annotation name. Remove tests that relied on annotation impacting result from shouldSync() Only edit in-place when explicitly requested to do so. Don't return error if unable to determine Ingress class, continue without setting either ingress or ingressClass. Update annotation to certmanager.k8s.io/acme-http01-edit-in-place in order to make use case more obvious and have consistent naming. Update docs to reflect possible values more accurately --- cmd/ingress-shim/controller/sync.go | 14 +++++++++++++- docs/reference/ingress-shim.rst | 7 +++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/cmd/ingress-shim/controller/sync.go b/cmd/ingress-shim/controller/sync.go index 4cd4e0cc7..15b43ff48 100644 --- a/cmd/ingress-shim/controller/sync.go +++ b/cmd/ingress-shim/controller/sync.go @@ -20,6 +20,9 @@ const ( // the default configuration provided to ingress-annotation should be // created. tlsACMEAnnotation = "kubernetes.io/tls-acme" + // editInPlaceAnnotation is used to toggle the use of ingressClass instead + // of ingress on the created Certificate resource + editInPlaceAnnotation = "certmanager.k8s.io/acme-http01-edit-in-place" // issuerNameAnnotation can be used to override the issuer specified on the // created Certificate resource. issuerNameAnnotation = "certmanager.k8s.io/issuer" @@ -182,7 +185,16 @@ func (c *Controller) setIssuerSpecificConfig(crt *v1alpha1.Certificate, issuer v } switch challengeType { case "http01": - domainCfg.HTTP01 = &v1alpha1.ACMECertificateHTTP01Config{Ingress: ing.Name} + editInPlace, ok := ingAnnotations[editInPlaceAnnotation] + // If annotation isn't present, or it's set to true, edit the existing ingress + if ok && editInPlace == "true" { + domainCfg.HTTP01 = &v1alpha1.ACMECertificateHTTP01Config{Ingress: ing.Name} + } else { + ingressClass, ok := ingAnnotations["kubernetes.io/ingress.class"] + if ok { + domainCfg.HTTP01 = &v1alpha1.ACMECertificateHTTP01Config{IngressClass: &ingressClass} + } + } case "dns01": dnsProvider, ok := ingAnnotations[acmeIssuerDNS01ProviderNameAnnotation] if !ok { diff --git a/docs/reference/ingress-shim.rst b/docs/reference/ingress-shim.rst index ce1f928a0..a9358d38c 100644 --- a/docs/reference/ingress-shim.rst +++ b/docs/reference/ingress-shim.rst @@ -68,5 +68,12 @@ Certificate resources to be automatically created: configuration of the ingress-shim (see above). Namely, a default issuer must be specified as arguments to the ingress-shim container. +* ``certmanager.k8s.io/acme-http01-edit-in-place""`` - if the ACME challenge type + has been set to http01, and the ingress has the 'kubernetes.io/tls-acme: true' + annotation, this controls whether the ingress is modified 'in-place', or a new + one created specifically for the http01 challenge. If present, and set to "true" + the existing ingress will be modified. Any other value, or the absence of the + annotation assumes "false". + .. _kube-lego: https://github.com/jetstack/kube-lego .. _ingress-shim: https://github.com/jetstack/cert-manager/tree/master/cmd/ingress-shim From 4b072e2ba38c114c3ca57f399ba58a99ecdf355e Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Thu, 19 Apr 2018 10:13:54 +0100 Subject: [PATCH 2/3] Update unit tests for edit-in-place annotation. --- cmd/ingress-shim/controller/sync.go | 5 +- cmd/ingress-shim/controller/sync_test.go | 112 ++++++++++++++++++++++- 2 files changed, 115 insertions(+), 2 deletions(-) diff --git a/cmd/ingress-shim/controller/sync.go b/cmd/ingress-shim/controller/sync.go index 15b43ff48..7a4bf822a 100644 --- a/cmd/ingress-shim/controller/sync.go +++ b/cmd/ingress-shim/controller/sync.go @@ -10,6 +10,7 @@ import ( extv1beta1 "k8s.io/api/extensions/v1beta1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/ingress/core/pkg/ingress/annotations/class" "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1" ) @@ -36,6 +37,8 @@ const ( // acmeIssuerDNS01ProviderNameAnnotation can be used to override the default dns01 provider // configured on the issuer if the challenge type is set to dns01 acmeIssuerDNS01ProviderNameAnnotation = "certmanager.k8s.io/acme-dns01-provider" + + ingressClassAnnotation = class.IngressKey ) var ingressGVK = extv1beta1.SchemeGroupVersion.WithKind("Ingress") @@ -190,7 +193,7 @@ func (c *Controller) setIssuerSpecificConfig(crt *v1alpha1.Certificate, issuer v if ok && editInPlace == "true" { domainCfg.HTTP01 = &v1alpha1.ACMECertificateHTTP01Config{Ingress: ing.Name} } else { - ingressClass, ok := ingAnnotations["kubernetes.io/ingress.class"] + ingressClass, ok := ingAnnotations[ingressClassAnnotation] if ok { domainCfg.HTTP01 = &v1alpha1.ACMECertificateHTTP01Config{IngressClass: &ingressClass} } diff --git a/cmd/ingress-shim/controller/sync_test.go b/cmd/ingress-shim/controller/sync_test.go index 460e7442c..c208c968a 100644 --- a/cmd/ingress-shim/controller/sync_test.go +++ b/cmd/ingress-shim/controller/sync_test.go @@ -13,6 +13,10 @@ import ( cminformers "github.com/jetstack/cert-manager/pkg/client/informers/externalversions" ) +func strPtr(s string) *string { + return &s +} + func TestShouldSync(t *testing.T) { type testT struct { Annotations map[string]string @@ -74,7 +78,7 @@ func TestBuildCertificates(t *testing.T) { } tests := []testT{ { - Name: "return a single HTTP01 Certificate for an ingress with a single valid TLS entry and HTTP01 annotations", + Name: "return a single HTTP01 Certificate for an ingress with a single valid TLS entry and HTTP01 annotations using edit-in-place", Ingress: &extv1beta1.Ingress{ ObjectMeta: metav1.ObjectMeta{ Name: "ingress-name", @@ -82,6 +86,7 @@ func TestBuildCertificates(t *testing.T) { Annotations: map[string]string{ clusterIssuerNameAnnotation: "issuer-name", acmeIssuerChallengeTypeAnnotation: "http01", + editInPlaceAnnotation: "true", }, }, Spec: extv1beta1.IngressSpec{ @@ -124,6 +129,111 @@ func TestBuildCertificates(t *testing.T) { }, }, }, + { + Name: "return a single HTTP01 Certificate for an ingress with a single valid TLS entry and HTTP01 annotations with a custom ingress class", + Ingress: &extv1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ingress-name", + Namespace: "ingress-namespace", + Annotations: map[string]string{ + clusterIssuerNameAnnotation: "issuer-name", + acmeIssuerChallengeTypeAnnotation: "http01", + ingressClassAnnotation: "nginx-ing", + }, + }, + Spec: extv1beta1.IngressSpec{ + TLS: []extv1beta1.IngressTLS{ + { + Hosts: []string{"example.com", "www.example.com"}, + SecretName: "example-com-tls", + }, + }, + }, + }, + ClusterIssuerLister: []*v1alpha1.ClusterIssuer{buildACMEClusterIssuer("issuer-name")}, + ExpectedCreate: []*v1alpha1.Certificate{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "example-com-tls", + Namespace: "ingress-namespace", + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(buildIngress("ingress-name", "ingress-namespace", nil), ingressGVK)}, + }, + Spec: v1alpha1.CertificateSpec{ + DNSNames: []string{"example.com", "www.example.com"}, + SecretName: "example-com-tls", + IssuerRef: v1alpha1.ObjectReference{ + Name: "issuer-name", + Kind: "ClusterIssuer", + }, + ACME: &v1alpha1.ACMECertificateConfig{ + Config: []v1alpha1.ACMECertificateDomainConfig{ + { + Domains: []string{"example.com", "www.example.com"}, + ACMESolverConfig: v1alpha1.ACMESolverConfig{ + HTTP01: &v1alpha1.ACMECertificateHTTP01Config{ + IngressClass: strPtr("nginx-ing"), + }, + }, + }, + }, + }, + }, + }, + }, + }, + { + Name: "edit-in-place set to false should not trigger editing the ingress in-place", + Ingress: &extv1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ingress-name", + Namespace: "ingress-namespace", + Annotations: map[string]string{ + clusterIssuerNameAnnotation: "issuer-name", + acmeIssuerChallengeTypeAnnotation: "http01", + ingressClassAnnotation: "nginx-ing", + editInPlaceAnnotation: "false", + }, + }, + Spec: extv1beta1.IngressSpec{ + TLS: []extv1beta1.IngressTLS{ + { + Hosts: []string{"example.com", "www.example.com"}, + SecretName: "example-com-tls", + }, + }, + }, + }, + ClusterIssuerLister: []*v1alpha1.ClusterIssuer{buildACMEClusterIssuer("issuer-name")}, + ExpectedCreate: []*v1alpha1.Certificate{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "example-com-tls", + Namespace: "ingress-namespace", + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(buildIngress("ingress-name", "ingress-namespace", nil), ingressGVK)}, + }, + Spec: v1alpha1.CertificateSpec{ + DNSNames: []string{"example.com", "www.example.com"}, + SecretName: "example-com-tls", + IssuerRef: v1alpha1.ObjectReference{ + Name: "issuer-name", + Kind: "ClusterIssuer", + }, + ACME: &v1alpha1.ACMECertificateConfig{ + Config: []v1alpha1.ACMECertificateDomainConfig{ + { + Domains: []string{"example.com", "www.example.com"}, + ACMESolverConfig: v1alpha1.ACMESolverConfig{ + HTTP01: &v1alpha1.ACMECertificateHTTP01Config{ + IngressClass: strPtr("nginx-ing"), + }, + }, + }, + }, + }, + }, + }, + }, + }, { Name: "should error when an ingress specifies dns01 challenge type but no challenge provider", Err: true, From 464cde00bf3daae29acfeb42eaa1ccca5ac743ef Mon Sep 17 00:00:00 2001 From: James Munnelly Date: Thu, 19 Apr 2018 11:50:34 +0100 Subject: [PATCH 3/3] Fix case where no ingress class is set. Add test case to verify. --- cmd/ingress-shim/controller/sync.go | 5 ++- cmd/ingress-shim/controller/sync_test.go | 49 ++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/cmd/ingress-shim/controller/sync.go b/cmd/ingress-shim/controller/sync.go index 7a4bf822a..0b7794226 100644 --- a/cmd/ingress-shim/controller/sync.go +++ b/cmd/ingress-shim/controller/sync.go @@ -188,14 +188,15 @@ func (c *Controller) setIssuerSpecificConfig(crt *v1alpha1.Certificate, issuer v } switch challengeType { case "http01": + domainCfg.HTTP01 = &v1alpha1.ACMECertificateHTTP01Config{} editInPlace, ok := ingAnnotations[editInPlaceAnnotation] // If annotation isn't present, or it's set to true, edit the existing ingress if ok && editInPlace == "true" { - domainCfg.HTTP01 = &v1alpha1.ACMECertificateHTTP01Config{Ingress: ing.Name} + domainCfg.HTTP01.Ingress = ing.Name } else { ingressClass, ok := ingAnnotations[ingressClassAnnotation] if ok { - domainCfg.HTTP01 = &v1alpha1.ACMECertificateHTTP01Config{IngressClass: &ingressClass} + domainCfg.HTTP01.IngressClass = &ingressClass } } case "dns01": diff --git a/cmd/ingress-shim/controller/sync_test.go b/cmd/ingress-shim/controller/sync_test.go index c208c968a..a23b2ce35 100644 --- a/cmd/ingress-shim/controller/sync_test.go +++ b/cmd/ingress-shim/controller/sync_test.go @@ -129,6 +129,55 @@ func TestBuildCertificates(t *testing.T) { }, }, }, + { + Name: "return a single HTTP01 Certificate for an ingress with a single valid TLS entry and HTTP01 annotations with no ingress class set", + Ingress: &extv1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ingress-name", + Namespace: "ingress-namespace", + Annotations: map[string]string{ + clusterIssuerNameAnnotation: "issuer-name", + acmeIssuerChallengeTypeAnnotation: "http01", + }, + }, + Spec: extv1beta1.IngressSpec{ + TLS: []extv1beta1.IngressTLS{ + { + Hosts: []string{"example.com", "www.example.com"}, + SecretName: "example-com-tls", + }, + }, + }, + }, + ClusterIssuerLister: []*v1alpha1.ClusterIssuer{buildACMEClusterIssuer("issuer-name")}, + ExpectedCreate: []*v1alpha1.Certificate{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "example-com-tls", + Namespace: "ingress-namespace", + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(buildIngress("ingress-name", "ingress-namespace", nil), ingressGVK)}, + }, + Spec: v1alpha1.CertificateSpec{ + DNSNames: []string{"example.com", "www.example.com"}, + SecretName: "example-com-tls", + IssuerRef: v1alpha1.ObjectReference{ + Name: "issuer-name", + Kind: "ClusterIssuer", + }, + ACME: &v1alpha1.ACMECertificateConfig{ + Config: []v1alpha1.ACMECertificateDomainConfig{ + { + Domains: []string{"example.com", "www.example.com"}, + ACMESolverConfig: v1alpha1.ACMESolverConfig{ + HTTP01: &v1alpha1.ACMECertificateHTTP01Config{}, + }, + }, + }, + }, + }, + }, + }, + }, { Name: "return a single HTTP01 Certificate for an ingress with a single valid TLS entry and HTTP01 annotations with a custom ingress class", Ingress: &extv1beta1.Ingress{