[ACME] Add RFC2136 DNS Provider #245

simonfuhrer · 2018-01-13T22:43:22Z

What this PR does / why we need it:
In our datacenter we do not use any of the currently supported DNS providers. With this pull request I added support for the DNS RFC2136 provider. An example configuration is also included.

Special notes for your reviewer:
What do you think of that? Is there anything missing? It would be great if this could be merged.

Release note:

Add RFC2136 DNS Provider

jetstack-ci-bot · 2018-01-13T22:43:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: munnerz

Assign the PR to them by writing /assign @munnerz in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

munnerz

Thanks for this! Absolutely will accept if we can get a few things cleaned up.

It looks like this code was also taken from the xenolf lego package? That's the source I've used for other provides 😄

munnerz · 2018-01-13T22:57:03Z

pkg/issuer/acme/dns/dns.go

+		tsigSecret, err := s.secretLister.Secrets(s.resourceNamespace).Get(providerConfig.RFC2136.TSIGSecret.Name)
+		if err != nil {
+			return nil, fmt.Errorf("error getting rfc2136 service account: %s", err.Error())
+		}


It looks like if this is left blank, authentication can be disabled. If the secret specified doesn't exist we should attempt without auth.

munnerz · 2018-01-13T22:58:04Z

pkg/apis/certmanager/v1alpha1/types.go

+	TSIGKey       string            `json:"TSIGKey"`
+	TSIGAlgorithm string            `json:"TSIGAlgorithm"`
+	Timeout       string            `json:"timeout"`
+}


You'll need to run ./hack/update-codegen.sh as you've updated API types.

Hi munnerz,
I have been able to execute this command successfully.

whereisaaron · 2018-01-13T23:52:26Z

Great. Can you add and Issuer example config to the docs, so people will know it is available.

simonfuhrer

The referenced secret is now also optional.
Documentation (Issuer and ClusterIssuer) enhanced with RFC2136 Provider.

Anything else missing?

simonfuhrer · 2018-01-14T12:12:40Z

pkg/apis/certmanager/v1alpha1/types.go

+	TSIGKey       string            `json:"TSIGKey"`
+	TSIGAlgorithm string            `json:"TSIGAlgorithm"`
+	Timeout       string            `json:"timeout"`
+}


Hi munnerz,
I have been able to execute this command successfully.

munnerz · 2018-01-15T18:24:30Z

/ok-to-test

munnerz · 2018-01-15T19:17:37Z

/test e2e

jetstack-ci-bot · 2018-01-15T23:15:24Z

@simonfuhrer PR needs rebase

simonfuhrer · 2018-01-19T17:34:36Z

@munnerz when do you plan to accept this pull request? Is there anything else I can do?

munnerz

Sorry for the delay here - i've had a pending review sitting for a few days!

With the addition of the Azure provider #246, I mentioned potentially moving the providers configuration into a single secret (with fixed value keys, e.g. tsigKey needs to be set as a value in the Secret) to save having so many fields that a user need configure on these manifests.

I'd like to hold this PR until we can decide what is best to do, and merge both this and #246 together after potentially adjusting for this new convention. What are your thoughts? I'm not sure if there's somewhere else in Kubernetes we can use as a reference for this perhaps.

munnerz · 2018-01-18T17:26:44Z

docs/api-types/issuer/spec.md

+rfc2136:
+  nameserver: 192.168.0.1
+  TSIGKey: tsig-key
+  TSIGSecretSecretRef:


Please camelCase this, so tsigSecretSecretRef and tsigKey

munnerz · 2018-01-18T17:27:27Z

docs/api-types/issuer/spec.md

+    name: tsig-secret
+    key: secret
+  TSIGAlgorithm: HmacMD5 
+  timeout: 60


Probably makes sense to just have the one example of this, explaining each field? If some fields are optional, add a comment to it to indicate so.

munnerz · 2018-01-18T17:29:12Z

pkg/apis/certmanager/v1alpha1/types.go

+	Nameserver    string            `json:"nameserver"`
+	TSIGSecret    SecretKeySelector `json:"TSIGSecretSecretRef"`
+	TSIGKey       string            `json:"TSIGKey"`
+	TSIGAlgorithm string            `json:"TSIGAlgorithm"`


How many algorithm types are there in the spec? Can we pull this into its own type?

type TSIGAlgorithm string const ( HmacMD5Algorithm TSIGAlgorithm = "HmacMD5" )

munnerz · 2018-01-23T17:59:44Z

docs/examples/acme-issuer.yaml

@@ -46,3 +46,20 @@ spec:
          # This field is optional for overriding the Route53 hosted zone ID
          # It is required to use it if the cert-manager cannot disambiguate between two different hosted zones for the same zone name
          hostedZoneID: DIKER8JPL21PSA
+      - name: rfc2136


dns providers can have friendly names here, e.g. 'corp-dns'. It might be better to use that kind of name here, so that users can realise this? rfc2136 is explicitly called out below in the struct configuration.

jetstack-bot · 2018-01-24T14:25:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: munnerz

Assign the PR to them by writing /assign @munnerz in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

simonfuhrer · 2018-01-26T10:47:38Z

/retest

jetstack-bot · 2018-01-26T10:48:39Z

@simonfuhrer: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
cert-manager-quick-verify	`4552188`	link	`/test verify`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

simonfuhrer · 2018-01-28T16:47:11Z

In my opinion we should avoid any secrets and use environment variables instead.
These can then also be made available in the pods via secrets.
However, we leave this choice to the users. This would simplify the entire deployment process.

What are your though?

munnerz · 2018-01-28T16:49:54Z

If environment variables were used, it would not be possible to talk to two different RFC2136 servers for different issuers, effectively breaking the multi tenant use case for DNS providers. By having this configuration (somehow) referenced/associated with each Issuer, we're able to support a wide variety of certificate issuing sources without having to deploy multiple instances of cert-manager.

…

On Sun, 28 Jan 2018 at 16:47, Simon Fuhrer ***@***.***> wrote: In my opinion we should avoid any secrets and use environment variables instead. These can then also be made available in the pods via secrets. However, we leave this choice to the users. This would simplify the entire deployment process. What are your though? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#245 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMbP_2fbAVSryuK8WUIWM5pi4ppuImRks5tPKSQgaJpZM4RdZYH> .

whereisaaron · 2018-01-28T18:32:52Z

I agree Secrets should be used in this case because:

It enables support for multiple RFC2136 servers, whether just because you have more that one RFC2136 DNS Issuer/ClusterIssuer or for multi-tenant clusters - as @munnerz noted. Using environment variables has been a big limitation on previous implementations like kube-lego and kube-cert-manager, where you have to run separate instances of the controller for every DNS provider for a given protocol. We have this issue, and cert-manager use of Secrets over environment variables is a fantastic improvement for operations.
It secures credentials in the a multi-tenant situation. If environment variables are used then one tenant's RFC2136 credentials become available other tenants to use.
It enables run-time updates to RFC2136 settings and credentials. Environment variable in k8s can only be set at Pod creation. They are not updated when the Secret is updated. Using environment variables necessitates redeploying the cert-manager application because the credentials for one Issuer/ClusterIssuer, of potentially a large number, has changed.

simonfuhrer · 2018-01-28T21:30:08Z

you convinced me. I see the benefits.

So it would be best to put the whole provider configuration in a secret with predefined keys. The Issuer object then only refers to the corresponding secret.

whereisaaron · 2018-01-28T22:31:58Z

@simonfuhrer that's a far less clear-cut question or more matter of taste or convention. I would suggest consistency is the aim here, ie. use the same approach other parts of cert-manager.

Looking around the configuration is usually to put only the actual secrets in the Secret, and refer to the secret values (or whole config file) by name and key name. Which is exactly what you already did in your PR spec and sample issuer for the tsig secret I think?

@munnerz talks above about having fixed keys to keep the Issuer manifest smaller. But I would personally prefer not to so that, as the correct fixed, and case-specific key in these cases is not obvious for users to guess, and the key would then become 'magic' value for each protocol that users can only find in the documentation. You'd get confused new users trying to create Secrets to match the implicit 'magic' key value. Having the key specified explicitly in the Issuer takes away all doubt for the user or trouble-shooter. It also give the user more flexibility, e.g. to stack all their tsig keys in one Secret under different keys. So I prefer to keep consistent with the current approach used by the clouddns/cloudflare examples below. What do you and @munnerz think?

      providers:
      - name: prod-dns
        clouddns:
          # A secretKeyRef to a google cloud json service account
          serviceAccountSecretRef:
            name: clouddns-service-account
            key: service-account.json
          # The project in which to update the DNS zone
          project: gcloud-prod-project
      - name: cf-dns
        cloudflare:
          email: user@example.com
          # A secretKeyRef to a cloudflare api key
          apiKeySecretRef:
            name: cloudflare-api-key
            key: api-key.txt

jetstack-ci-bot · 2018-02-22T08:00:42Z

@simonfuhrer PR needs rebase

munnerz

Thanks once again for this, and I'm sorry its taken so long to get merged. There's just the one comment now about removing the timeout field, and then I'll add a lgtm ready for the 0.3 release! 😄

munnerz · 2018-03-01T12:05:05Z

pkg/apis/certmanager/v1alpha1/types.go

+	TSIGSecret    SecretKeySelector `json:"tsigSecretSecretRef"`
+	TSIGKey       string            `json:"tsigKey"`
+	TSIGAlgorithm TSIGAlgorithm     `json:"tsigAlgorithm"`
+	Timeout       string            `json:"timeout"`


The 'timeout' field isn't actually used within the provider, in favour of a static global timeout.

None of the other DNS providers have this field, so for now I think we're best to remove it from our API surface. In future, we may add it back in (potentially as a top-level timeout field on the issuer).

Corresponding docs need updating too to reflect this!

dippynark · 2018-06-20T11:50:15Z

/unassign

munnerz · 2018-09-12T07:45:38Z

Closing in favour of #661

jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 13, 2018

jetstack-ci-bot assigned munnerz and dippynark Jan 13, 2018

munnerz reviewed Jan 13, 2018

View reviewed changes

simonfuhrer force-pushed the rfc2136 branch from fa4f5a1 to e2628e6 Compare January 14, 2018 11:37

simonfuhrer changed the title ~~[ACME] Add RFC2136 DNS Provider~~ WIP: [ACME] Add RFC2136 DNS Provider Jan 14, 2018

jetstack-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 14, 2018

simonfuhrer force-pushed the rfc2136 branch 2 times, most recently from c2694a6 to 6d7aa84 Compare January 14, 2018 11:43

jetstack-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 14, 2018

simonfuhrer force-pushed the rfc2136 branch 2 times, most recently from f59a549 to 593edb5 Compare January 14, 2018 12:04

simonfuhrer commented Jan 14, 2018

View reviewed changes

simonfuhrer changed the title ~~WIP: [ACME] Add RFC2136 DNS Provider~~ [ACME] Add RFC2136 DNS Provider Jan 14, 2018

jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 14, 2018

jetstack-bot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 15, 2018

simonfuhrer force-pushed the rfc2136 branch 3 times, most recently from 311bad3 to 3594882 Compare January 15, 2018 20:11

munnerz added the area/acme Indicates a PR directly modifies the ACME Issuer code label Jan 15, 2018

jetstack-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 15, 2018

simonfuhrer force-pushed the rfc2136 branch from 3594882 to 8463ae2 Compare January 16, 2018 08:05

munnerz requested changes Jan 23, 2018

View reviewed changes

simonfuhrer force-pushed the rfc2136 branch from 8463ae2 to 9f667fd Compare January 24, 2018 14:25

simonfuhrer force-pushed the rfc2136 branch 2 times, most recently from 956da58 to 6dd9689 Compare January 24, 2018 14:36

add dns rfc2136 provider

4552188

simonfuhrer force-pushed the rfc2136 branch from 6dd9689 to 4552188 Compare January 25, 2018 12:02

jetstack-ci-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2018

munnerz reviewed Mar 1, 2018

View reviewed changes

munnerz added area/acme Indicates a PR directly modifies the ACME Issuer code and removed area/acme Indicates a PR directly modifies the ACME Issuer code labels Apr 25, 2018

munnerz mentioned this pull request Jun 6, 2018

Support a generic DNS solver #468

Closed

splashx mentioned this pull request Jun 17, 2018

Rebase based on latest master from jetstack simonfuhrer/cert-manager#1

Closed

splashx added a commit to splashx/cert-manager that referenced this pull request Jun 17, 2018

original pr cert-manager#245 - rebase, remove timeout

5baca46

splashx mentioned this pull request Jun 17, 2018

[ACME] Add RFC2136 DNS Provider (2nd attempt) #661

Merged

jetstack-bot unassigned dippynark Jun 20, 2018

splashx mentioned this pull request Jun 20, 2018

[ACME] Add RFC2136 DNS Provider pan-net-security/cert-manager#1

Merged

munnerz closed this Sep 12, 2018

wallrj mentioned this pull request Jan 26, 2024

Stop using the deprecated SingleInflight field of miekg/dns #6669

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ACME] Add RFC2136 DNS Provider #245

[ACME] Add RFC2136 DNS Provider #245

simonfuhrer commented Jan 13, 2018

jetstack-ci-bot commented Jan 13, 2018

munnerz left a comment

munnerz Jan 13, 2018

munnerz Jan 13, 2018

simonfuhrer Jan 14, 2018

whereisaaron commented Jan 13, 2018

simonfuhrer left a comment

simonfuhrer Jan 14, 2018

munnerz commented Jan 15, 2018 •

edited

munnerz commented Jan 15, 2018

jetstack-ci-bot commented Jan 15, 2018

simonfuhrer commented Jan 19, 2018

munnerz left a comment

munnerz Jan 18, 2018

munnerz Jan 18, 2018

munnerz Jan 18, 2018

munnerz Jan 23, 2018

jetstack-bot commented Jan 24, 2018

simonfuhrer commented Jan 26, 2018

jetstack-bot commented Jan 26, 2018

simonfuhrer commented Jan 28, 2018

munnerz commented Jan 28, 2018 via email

whereisaaron commented Jan 28, 2018

simonfuhrer commented Jan 28, 2018

whereisaaron commented Jan 28, 2018

jetstack-ci-bot commented Feb 22, 2018

munnerz left a comment

munnerz Mar 1, 2018

dippynark commented Jun 20, 2018

munnerz commented Sep 12, 2018

[ACME] Add RFC2136 DNS Provider #245

[ACME] Add RFC2136 DNS Provider #245

Conversation

simonfuhrer commented Jan 13, 2018

jetstack-ci-bot commented Jan 13, 2018

munnerz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whereisaaron commented Jan 13, 2018

simonfuhrer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

munnerz commented Jan 15, 2018 • edited

munnerz commented Jan 15, 2018

jetstack-ci-bot commented Jan 15, 2018

simonfuhrer commented Jan 19, 2018

munnerz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jetstack-bot commented Jan 24, 2018

simonfuhrer commented Jan 26, 2018

jetstack-bot commented Jan 26, 2018

simonfuhrer commented Jan 28, 2018

munnerz commented Jan 28, 2018 via email

whereisaaron commented Jan 28, 2018

simonfuhrer commented Jan 28, 2018

whereisaaron commented Jan 28, 2018

jetstack-ci-bot commented Feb 22, 2018

munnerz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dippynark commented Jun 20, 2018

munnerz commented Sep 12, 2018

munnerz commented Jan 15, 2018 •

edited