Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flag for setting nameservers for DNS01 check #710

Merged
merged 8 commits into from Jul 11, 2018

Conversation

kragniz
Copy link
Contributor

@kragniz kragniz commented Jul 5, 2018

This adds a new flag, --dns01-nameservers, which allows setting a list of custom nameservers for the DNS01 check.

Fixes #506.

Add `--dns01-nameservers` flag for setting nameservers for DNS01 check

@jetstack-bot jetstack-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 5, 2018
@jetstack-bot jetstack-bot requested a review from wallrj July 5, 2018 11:45
@jetstack-bot jetstack-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 5, 2018

Cert-manager will check the correct DNS records exist before attempting a DNS01
challenge. By default, the DNS servers for this check will be taken from
``/etc/resolv.conf``. If this is not desired, the cert-manager controller
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand If this is not desired, to give some examples of where this may not be desired (i.e. if you have two authoritative nameservers for the same zone/are using split-brain/split-horizon DNS)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a little extra, let me know if some larger examples would be needed

@@ -120,6 +126,8 @@ func (s *ControllerOptions) AddFlags(fs *pflag.FlagSet) {
fs.StringVar(&s.DefaultACMEIssuerDNS01ProviderName, "default-acme-issuer-dns01-provider-name", defaultACMEIssuerDNS01ProviderName, ""+
"Required if --default-acme-issuer-challenge-type is set to dns01. The DNS01 provider to use for ingresses using ACME dns01 "+
"validation that do not explicitly state a dns provider.")
fs.StringVar(&s.DNS01Nameservers, "dns01-nameservers", defaultDNS01Nameservers, ""+
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dns01-self-check-nameservers maybe? Not sure if it's such an important differentiation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd also be good to provide some examples of this flags usage. As it stands, the user will need to guess that they should specify 8.8.8.8:53 here. I imagine many users will specify 8.8.8.8.

Let's add a (e.g. 8.8.8.8:53,8.8.4.4:53) to the help message, and also perform validation in the options Validate function to ensure that the NS's entered are validated and checked on start-up (we don't need to actually query the NS, just ensure that the values inputted are all sensible, e.g. is a valid IPv4/IPv6 address followed by a port number.

@@ -95,6 +97,15 @@ func buildControllerContext(opts *options.ControllerOptions) (*controller.Contex
return nil, nil, fmt.Errorf("error creating kubernetes client: %s", err.Error())
}

nameservers := []string{}
if opts.DNS01Nameservers != "" {
nameservers = strings.Split(opts.DNS01Nameservers, ",")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to split manually - pflag supports slices natively: https://godoc.org/github.com/spf13/pflag#StringSlice

@munnerz
Copy link
Member

munnerz commented Jul 11, 2018

/lgtm
/approve

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2018
@jetstack-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2018
@jetstack-bot jetstack-bot merged commit bd7f15d into cert-manager:master Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants