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.

Sign up for GitHub

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

Jump to bottom

ns1: add minttlseconds #1576

Merged
merged 4 commits into from
Sep 2, 2020
Merged

ns1: add minttlseconds #1576

merged 4 commits into from
Sep 2, 2020

Conversation

dennisme
Copy link
Contributor

This PR adds the ability to set a ns1-min-ttl for the ns1 provider.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 14, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @dennisme!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 14, 2020
@dennisme
Copy link
Contributor Author

Happy to squash commits, post review :)

@Raffo
Copy link
Contributor

Raffo commented May 16, 2020

Hi @dennisme ! I'm not sure I understand why we would need this. To me, it looks like you want to protect from a TTL that is too low, but this is just another setting and thus subject to the same considerations: it needs to be tested / audited to be considered good. Is there anything I'm missing here?

@dennisme
Copy link
Contributor Author

@Raffo hey, good questions. I am trying to just make it easier to control the ttl of ns1 records created by external-dns. Default of 10 is quite low. Yes its possible to add external-dns.alpha.kubernetes.io/ttl annotation and redeploy your service, but in a world of dozens of clusters and hundreds of deployments that could be take some time.

I am cool doing the testing and even adding more validation.

@dennisme
Copy link
Contributor Author

@Raffo thoughts on the above?

njuettner
njuettner requested changes May 26, 2020
View reviewed changes
Copy link
Member

@njuettner njuettner left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Ok to me only a nit but you may want to add a test for pkg/apis/externaldns/types.go. To see if ttl get's overwritten.

main.go
NS1Endpoint: cfg.NS1Endpoint,
NS1IgnoreSSL: cfg.NS1IgnoreSSL,
DryRun: cfg.DryRun,
MinTTLSeconds: cfg.NS1MinTTLSeconds,
Copy link
Member

Choose a reason for hiding this comment

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

Why not N1TTLSeconds? Same for the flag ns1-min-ttl -> ns1-ttl

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 like your name, I can update

njuettner
njuettner reviewed May 28, 2020
View reviewed changes
pkg/apis/externaldns/types.go
@@ -361,6 +362,7 @@ func (cfg *Config) ParseFlags(args []string) error {
app.Flag("pdns-tls-enabled", "When using the PowerDNS/PDNS provider, specify whether to use TLS (default: false, requires --tls-ca, optionally specify --tls-client-cert and --tls-client-cert-key)").Default(strconv.FormatBool(defaultConfig.PDNSTLSEnabled)).BoolVar(&cfg.PDNSTLSEnabled)
app.Flag("ns1-endpoint", "When using the NS1 provider, specify the URL of the API endpoint to target (default: https://api.nsone.net/v1/)").Default(defaultConfig.NS1Endpoint).StringVar(&cfg.NS1Endpoint)
app.Flag("ns1-ignoressl", "When using the NS1 provider, specify whether to verify the SSL certificate (default: false)").Default(strconv.FormatBool(defaultConfig.NS1IgnoreSSL)).BoolVar(&cfg.NS1IgnoreSSL)
app.Flag("ns1-min-ttl", "Minimal TTL (in seconds) for records. This value will be used if the provided TTL for a service/ingress is lower than this.").IntVar(&cfg.NS1MinTTLSeconds)
Copy link
Member

Choose a reason for hiding this comment

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

Another suggestion you could add a default TTL if you want, IIIRC we use is in different providers already

Copy link
Member

Choose a reason for hiding this comment

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

As an example:

app.Flag("rfc2136-min-ttl", "When using the RFC2136 provider, specify minimal TTL (in duration format) for records. This value will be used if the provided TTL for a service/ingress is lower than this").Default(defaultConfig.RFC2136MinTTL.String()).DurationVar(&cfg.RFC2136MinTTL)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 1, 2020
@njuettner
Copy link
Member

@dennisme in case you're still interested in merging this PR, please run another rebase and please add you PR into the CHANGELOG.md.

@dennisme
Copy link
Contributor Author

@dennisme in case you're still interested in merging this PR, please run another rebase and please add you PR into the CHANGELOG.md.

Still interested just need to find some cycles.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2020
@seanmalloy
Copy link
Member

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 19, 2020
@seanmalloy
Copy link
Member

@dennisme could you please update the CHANGELOG.md?

@Raffo
Copy link
Contributor

Raffo commented Sep 2, 2020

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dennisme, Raffo

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

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 2, 2020
@k8s-ci-robot k8s-ci-robot merged commit 8c3220b into kubernetes-sigs:master Sep 2, 2020
@dennisme
Copy link
Contributor Author

dennisme commented Sep 2, 2020

@seanmalloy yes happy to open a second PR for that.

thanks @Raffo, sorry for the delay here I was taking a bit of a break from after hours work :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njuettner njuettner njuettner requested changes

@hjacobs hjacobs Awaiting requested review from hjacobs

@Raffo Raffo Awaiting requested review from Raffo

Assignees

@Raffo Raffo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dennisme @k8s-ci-robot @Raffo @njuettner @seanmalloy