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

Ignore ambassador-hosts with invalid annotations #3008

Merged
merged 2 commits into from
Sep 7, 2022

Conversation

olemarkus
Copy link
Contributor

Description

If e.g the ambassador service annotation points to an nonexisting service, external-dns will not update any DNS records. This PR makes external-dns ignore the host and carry on instead.

Didn't provide any unit tests as this code didn't seem to have anything like a fake client that knows about ambassador hosts. Did however validate the logic in a live cluster.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 7, 2022
@szuecs
Copy link
Contributor

szuecs commented Sep 7, 2022

Thanks code looks fine.
@olemarkus what about tests?

@olemarkus
Copy link
Contributor Author

As mentioned, its not entirely straightforward to introduce unit test to that code. Would need to wire up the dynamic client and such. It would be a significantly larger PR alone. But this source is important to us, so I don't mind doing another PR for that. It will take a few days maybe.

Ole Markus With added 2 commits September 7, 2022 19:38
If e.g the ambassador service annotation points to an nonexisting service, external-dns will not update any DNS records. This PR makes external-dns ignore the host and carry on instead.
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 7, 2022
@olemarkus
Copy link
Contributor Author

@szuecs added test only for this particular bug now.

@szuecs
Copy link
Contributor

szuecs commented Sep 7, 2022

Thanks @olemarkus !

@szuecs
Copy link
Contributor

szuecs commented Sep 7, 2022

/approve
/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 7, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: olemarkus, szuecs

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 7, 2022
@k8s-ci-robot k8s-ci-robot merged commit 47b3a15 into kubernetes-sigs:master Sep 7, 2022
@KyleMartin901
Copy link

Hi @szuecs and @olemarkus I have added tests in my PR that also updates the Ambassador source to use label and annotation filters #2633. If you wouldn't mind taking a look that would be awesome. I will have to refactor based on this PR

@olemarkus
Copy link
Contributor Author

@KyleMartin901 The PR looks good, I think. Ping me when you've refactored. I can review, but I cannot merge, unfortunately.

@KyleMartin901
Copy link

Thanks, @olemarkus I managed to find some time to rebase the tests today so 🤞 it's all good.

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. 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants