-
Notifications
You must be signed in to change notification settings - Fork 591
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
Implement --watch-namespace flag #1317
Conversation
Add a --watch-namespace flag that mimics the 1.x flag behavior (watch all namespaces by default and allow limiting the controller to one, and only one namespace).
Codecov Report
@@ Coverage Diff @@
## next #1317 +/- ##
==========================================
+ Coverage 52.90% 56.18% +3.28%
==========================================
Files 35 35
Lines 3272 3394 +122
==========================================
+ Hits 1731 1907 +176
+ Misses 1408 1347 -61
- Partials 133 140 +7
Continue to review full report at Codecov.
|
Implement support for watching multiple distinct namespaces using a controller-runtime MultiNamespacedCacheBuilder.
69343d1
to
9d6182b
Compare
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.
Ultimately I have no blockers, but please do see my comments and let me know what you think.
railgun/manager/config.go
Outdated
@@ -105,6 +107,9 @@ func (c *Config) FlagSet() *pflag.FlagSet { | |||
flagSet.StringVar(&c.LeaderElectionID, "election-id", "5b374a9e.konghq.com", `Election id to use for status update.`) | |||
flagSet.StringVar(&c.FilterTag, "kong-filter-tag", "managed-by-railgun", "TODO") | |||
flagSet.IntVar(&c.Concurrency, "kong-concurrency", 10, "TODO") | |||
flagSet.StringVar(&c.WatchNamespace, "watch-namespace", apiv1.NamespaceAll, |
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 mode does not set the Namespace option, so the manager will default to watching all namespaces | ||
// MultiNamespacedCacheBuilder imposes a filter on top of that watch to retrieve scoped resources | ||
// from the watched namespaces only. |
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.
Good comments thanks 👍
if useLegacyKIC() { | ||
t.Skip("support for distinct namespace watches is not supported in legacy KIC") | ||
} |
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.
Since we're adding a compelling new option that wasn't available before, would this PR be a good a time as any to start up some 2.0 release notes and add this functionality as a bullet point for new gains?
- Move supplemental namespace creation to test. - Replace magic string with constant. - Clean up extra created namespaces.
Implement the --watch-namespace flag for Railgun's manager. The initial commit implements the same behavior as the 1.x flag; the second commit implements additional support for watching multiple discrete namespaces (as requested internally in FTI-2395 and frequent chat requests) while maintaining compatibility with 1.x behavior.
The improved version is simple to implement (almost everything is handled by controller-runtime), so I suggest we go with it and squash these into a single commit before merging. The first commit is mostly just to showcase how we would implement this if we want to implement only the 1.x behavior.
Note that MultiNamespacedCacheBuilder does not work independent of Namespace in Manager options, and notably cannot handle the default "watch all namespaces" behavior, hence the "determine how to configure namespace watchers" bit in run.go (adapted from an Operator Framework example).
Integration tests for the first option would be a bit difficult since they'd limit us to a single test namespace. However, I assert that it does at least work--the following change to existing tests will make everything fail by excluding the default namespace:
The second option is a bit more reasonable to test, though we do now need to maintain a list of namespaces with test content and can't test the default without an additional controller instance. The negative test is also a bit hand-wavy, as there's not really a good way to confirm that we don't see that config specifically because the Ingress is in another namespace. Unlike ingress class, we can't modify an existing Ingress to watch its routes flip on and off again.
Fix #1299