-
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
Restore support for exclusion-only domain filters for AWS provider #2850
Restore support for exclusion-only domain filters for AWS provider #2850
Conversation
|
Welcome @orirawlings! |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
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.
Added a few comments, but I am a bit confused by this PR. It is changing other providers as well as the AWS provider. Can you clarify the changes that you made here?
@@ -146,35 +149,32 @@ func matchRegex(regex *regexp.Regexp, negativeRegex *regexp.Regexp, domain strin | |||
|
|||
// MatchParent checks wether DomainFilter matches a given parent domain. | |||
func (df DomainFilter) MatchParent(domain string) bool { | |||
if !df.IsConfigured() { |
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.
Why did you modify IsConfigured
below if you are removing it here?
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.
IsConfigured
is still consumed at other call sites.
return false | ||
} | ||
|
||
// IsConfigured returns true if DomainFilter is configured, false otherwise | ||
// IsConfigured returns true if any inclusion or exclusion rules have been specified. | ||
func (df DomainFilter) IsConfigured() bool { |
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.
What is the purpose of modifying this method?
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.
There are call sites in the code that need to determine if filtering rules have been defined. As far as I can tell, those call sites are generally not concerned with "are allow rules defined" separately from "are exclude rules defined". As such, I've normalized this method to align with its name, "are any rules defined".
true, | ||
false, |
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.
This changes the behavior completely. Was the previous one wrong? If so, why?
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.
I think this behavior is confusing and unexpected. Logically a filter list composed entirely of ignored values should be equivalent to a filter list that was never configured at all. It is especially confusing when a list with a single ignored value is considered as not configured, but multiple ignored values are considered configured.
This PR normalizes IsConfigured
so that it only returns true if valid values have been included.
Thanks for the review @Raffo I tried to document some of the context in the linked issue (#2821), but let me try to clarify here. (It's been a while since I prepared this PR so I have to refresh my memory as well) If I recall there were two surprises:
From the issue:
So this PR attempts to normalize a few things.
Changes have been made to some providers outside of the AWS provider to align with normalization number 1, above. Other providers have been changed to fix log warnings that might be misleading when only exclusion rules are configured. In those cases the call site was calling |
@orirawlings ACK that I've seen this, but I wasn't able to dig deep into this again. Thanks for your detailed explanation, I'll try to give feedback as soon as I manage. |
Description
This aims to fix regression #2821.
I've tweaked
endpoint.DomainFilter.IsConfigured()
to pass for filters that only express exclusion rules (either regular expression-based exclusion or an explicit exclusion list). I've also tweaked some of the initialization to handle empty strings in explicit filter lists less ambiguously.Fixes #2821
Checklist