Implement --watch-namespace flag #1317
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
rainest
force-pushed
the
feat/rg-namespace-watch
branch
from
69343d1
to
9d6182b
Compare
shaneutt
approved these changes
May 18, 2021
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, | |||
Comment on lines
+80
to
+82
| // 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. |
Comment on lines
+314
to
+316
| if useLegacyKIC() { | ||
| t.Skip("support for distinct namespace watches is not supported in legacy KIC") | ||
| } |
There was a problem hiding this comment.
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?
3 tasks
added 4 commits
May 18, 2021 12:46
- Move supplemental namespace creation to test. - Replace magic string with constant. - Clean up extra created namespaces.
shaneutt
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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