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 --zone-name-filter option for azure provider #1060

Merged
merged 2 commits into from
Sep 16, 2020

Conversation

titilambert
Copy link
Contributor

Use case

  • We have ONE Azure DNS domain (k8s.example.com)
  • We have multiple Kubernetes clusters with one ExternalDNS per cluster.
  • Each Kubernetes cluster manages its own subdomain
    • DEV cluster: dev.k8s.example.com
    • QA cluster: qa.k8s.example.com
  • We need to protect all the subdomains
    Example: the cluster handling dev.k8s.example.com can NOT CREATE (and edit/delete) entry in the subdmain qa.k8s.example.com

With the latest externalDNS, we can not handle this use case.

The patch

Summary

This patch add a new parameter --zone-name-filter.
This parameter is used only by the azure provider (for now).
When it used, the --domain-filter behavior changes: --domain-filter filter ingress domains and NOT Azure DNS zone domain.
There is no breaking change for current users of the Azure provider.

How to run to handle the use case

With this patch:

  • we can run externalDNS in the DEV cluster with the following parameter:
external-dns \
  --source=service \
  --source=ingress \
  --zone-name-filter=k8s.example.com \
  --domain-filter=dev.k8s.example.com \
  --provider=azure \
  --azure-resource-group=externaldns \
  --azure-config-file=azure.json
  • we can run externalDNS in the DEV cluster with the following parameter:
external-dns \
  --source=service \
  --source=ingress \
  --zone-name-filter=k8s.example.com \
  --domain-filter=qa.k8s.example.com \
  --provider=azure \
  --azure-resource-group=externaldns \
  --azure-config-file=azure.json

@k8s-ci-robot
Copy link
Contributor

Welcome @titilambert!

It looks like this is your first PR to kubernetes-incubator/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-incubator/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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 7, 2019
@titilambert titilambert force-pushed the master branch 2 times, most recently from befe3ce to c39fe3f Compare June 7, 2019 17:17
@alfredkrohmer
Copy link
Contributor

Why not just use two zones? If you / external-dns has access to create records in k8s.example.com, I would guess you also have access to create an NS record for dev.k8s.example and qa.k8s.example.com and delegate to these two zones?

@djsly
Copy link

djsly commented Jun 12, 2019

@devkid , we already had a discussion on slack #external-dns , with @stromming. While your approach can work for small amount of subnets. we have many zones and subnet to manage (one per cluster)

I will let @titilambert share the discussion we had in Slack in this PR

@titilambert
Copy link
Contributor Author

@devkid here the first comment of this PR: https://kubernetes.slack.com/archives/C771MKDKQ/p1559914092057000

provider/azure.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@njuettner
Copy link
Member

@titilambert Do you mind rebasing from master again and resolve the conflicts? I think we can merge it.

@titilambert
Copy link
Contributor Author

@njuettner rebased !!! Thanks !

provider/azure.go Outdated Show resolved Hide resolved
@titilambert
Copy link
Contributor Author

And rebased !

@djsly
Copy link

djsly commented Jul 25, 2019

is this good to be merged @njuettner ?

@njuettner
Copy link
Member

@titilambert ^^

@njuettner
Copy link
Member

@titilambert Could you please take a look on my last comment?

@djsly
Copy link

djsly commented Sep 10, 2019 via email

@titilambert
Copy link
Contributor Author

titilambert commented Oct 17, 2019

@njuettner sorry for the delay, I'm now back, and I just fixed the last issue

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.

@titilambert thank you again, do you mind fix one last nit and solve the conflicting files? Then I'm happy to merge your PR 😄.

pkg/apis/externaldns/types.go Outdated Show resolved Hide resolved
@titilambert
Copy link
Contributor Author

@njuettner the tests are working on my laptop :/ and I'm not sure to understand the issue on travis. Could you help me with that ? Thanks

@djsly
Copy link

djsly commented Jan 14, 2020

@njuettner we would like to start using the official release, can you se if you can help @titilambert with the travis error ?

@titilambert
Copy link
Contributor Author

@njuettner any news on this PR ?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2020
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.

added some comments, PTAL

pkg/apis/externaldns/types.go Outdated Show resolved Hide resolved
pkg/apis/externaldns/types_test.go Outdated Show resolved Hide resolved
provider/azure.go Outdated Show resolved Hide resolved
provider/azure.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2020
@titilambert
Copy link
Contributor Author

@vinny-sabatini I just rebase this PR ! Hopefully it will be merged quickly :)
Thanks !

@seanmalloy
Copy link
Member

/assign

Copy link
Contributor

@vinny-sabatini vinny-sabatini 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 updating the PR @titilambert . I think the code looks good to me, but would you be able to add some tests for the code changes made in azure.go?

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 8, 2020
@titilambert
Copy link
Contributor Author

@vinny-sabatini I just added the tests. It should cover all the new lines

Copy link
Contributor

@vinny-sabatini vinny-sabatini 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 adding those tests, this looks good to me.

@njuettner / @Raffo could one of you take a look at this when you have a chance?

@vinny-sabatini
Copy link
Contributor

/assign @Raffo @njuettner

@djsly
Copy link

djsly commented Sep 14, 2020

@vinny-sabatini are we missing something ?

@seanmalloy
Copy link
Member

/unassign

@vinny-sabatini
Copy link
Contributor

Hi @djsly, I am not a maintainer of the org so I don't have approve/merge access. At this point we just need to give the maintainers some time to review the code. I will reach out to them to try to get it reviewed and hopefully merged soon. Thanks for your patience.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2020
@Raffo
Copy link
Contributor

Raffo commented Sep 16, 2020

Sorry for the delay with this PR! The life of a OSS maintainer is hard 😬

@Raffo
Copy link
Contributor

Raffo commented Sep 16, 2020

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 16, 2020
@k8s-ci-robot k8s-ci-robot merged commit 699a8b3 into kubernetes-sigs:master Sep 16, 2020
@titilambert
Copy link
Contributor Author

Yeah ! Thanks !

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. 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. needs-change size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants