-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 and use a deduplicating source #185
Conversation
We definitely should add logging (at least debug). |
This is a cleaner abstraction, thanks for doing that. I agree that having the logging in if there is another record for the DNS entry with a different Target though would be desirable for a debugging/usability standpoint. |
} | ||
|
||
for _, ep := range endpoints { | ||
identifier := ep.DNSName + " / " + ep.Target |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flaky I would say, especially since we don't do any sanitisation on hostnames at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uglier, but more bullet-proof would be collected := map[string]map[string]bool{}
or map[string][]string
:(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would a nested map make it more bullet-proof? E.g. the sanitization issue you mention still applies, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, you are right. probably potential issues with sanitisation can be postponed as well :)
small comment above, otherwise LGTM |
Just a note that the current PR has a different purpose than #183 . This PR resolves the harmless case when two resources have same target and DNS names, hence can and should be deduplicated. This #183 on the other hand aims to resolve a bigger problem of clashing DNS names, however is initially aimed at resolving #182, which is resolved by this PR in a simpler way :) |
👍 |
* feat: add and use a deduplicating source * chore: add log entry when endpoint was deduplicated
This provides an alternative solution to #182 by using a deduplicating source on the outer edge of the sources chain.
Let me know what you think about this and which approach you prefer compared to #183.
However, as Sources were intended to be extensible a
DedupSource
doesn't harm and might come in handy in any case./cc @jrnt30 @ideahitme