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

AWS provider: Properly check suitable domains #594

Merged
merged 1 commit into from
Jun 20, 2018

Conversation

elordahl
Copy link
Contributor

Similar to #478 , this will correctly detect zones when names overlap with subdomains. I added a note to #446, but let me know if i should create a separate issue.

Note: I am not sure why all matching private zones are considered suitable, but did not modify that logic.

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 14, 2018
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jun 14, 2018
@elordahl elordahl force-pushed the names branch 2 times, most recently from e8eee44 to 2896444 Compare June 14, 2018 04:39
@njuettner
Copy link
Member

njuettner commented Jun 14, 2018

Thank you for your PR 🙂 , could you first sign the CLA please?

We'll have a look on your PR afterwards.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 14, 2018
@elordahl
Copy link
Contributor Author

CLA updated (force pushed w/ correct email too).

@hjacobs
Copy link
Contributor

hjacobs commented Jun 14, 2018

/lgtm

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

hjacobs commented Jun 14, 2018

Hmm, why are we not using ZoneFinder here?

@elordahl
Copy link
Contributor Author

Based on #356, it looks like logic is slightly different since multiple zones can be updated.

// bar.example.org is NOT suitable
{"foobar.example.org.", []*route53.HostedZone{zones["example-org-private"], zones["example-org"]}},

// all matching private zones are suitable (i'm not sure why)
Copy link
Member

Choose a reason for hiding this comment

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

It's explained in #356 (comment) and your test matches the one above, so I think it's fine how you did it. Can you move this test line to first block so it's easier to see?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Submitted in new PR since this was merged. Feel free to close if no longer needed. #608

@linki
Copy link
Member

linki commented Jun 20, 2018

@elordahl LGTM. Thanks for the fix!

@njuettner njuettner merged commit 2e4778f into kubernetes-sigs:master Jun 20, 2018
@elordahl elordahl deleted the names branch June 20, 2018 15:30
@elordahl elordahl restored the names branch June 20, 2018 15:40
@elordahl elordahl deleted the names branch June 20, 2018 16:14
@cmattoon cmattoon mentioned this pull request Jul 12, 2018
lou-lan pushed a commit to lou-lan/external-dns that referenced this pull request May 11, 2022
* Modify upgrade to work with multiple indexes

* Use plugin display name in output

* Code review changes

* Return error when passing canonical name to upgrade/uninstall

* Code review updates

* Code review changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants