Add --ns-follows-kube-context global flag for using the kubeconfig context namespace#5831
Conversation
168c848 to
78b99e0
Compare
|
This is a major breaking change that we can not accept. Imagine how many CI pipelines and custom scripts we are going to break with this, e.g. you have |
|
We had lots of discussions around this over the years. This is a bug that became expected behavior at this point, so we cannot break it. However, across discussions someone suggested an env var for opting into this behavior, which would not be a breaking change and I'd be ok with it. Env var or flag, not sure what's best, though. |
|
Thanks for the feedback, @stefanprodan. I understand your concern. This behavior was discussed and classified as a bug in #3453, with the consensus being that the kubeconfig context namespace should be respected (see #3453 (comment)). That said, I understand the concern about breaking existing CI pipelines and scripts that rely on Would an opt-in approach work for now? For example, introducing a Or would you prefer a different approach entirely? |
|
Thanks for joining the discussion, @matheuscscp. How about supporting both?
This follows the existing pattern where Would that be acceptable? |
|
I have no objections to this solution |
e16ed58 to
a1ddbb5
Compare
|
@matheuscscp @stefanprodan I have updated the implementation to make the context namespace resolution opt-in. It can be enabled via one of the following:
Without either of these, the existing behavior is preserved ( |
bb43542 to
39e6c46
Compare
| // explicitly set and FLUX_SYSTEM_NAMESPACE env var is not set, | ||
| // respect the namespace from the kubeconfig context. | ||
| if (rootArgs.nsFollowsKubeContext || os.Getenv("FLUX_NS_FOLLOWS_KUBE_CONTEXT") != "") && | ||
| !cmd.Flags().Changed("namespace") && os.Getenv("FLUX_SYSTEM_NAMESPACE") == "" { |
There was a problem hiding this comment.
I'm not sure I understand why FLUX_SYSTEM_NAMESPACE is checked here. I don't always care about that as my Flux resources may be scattered across multiple namespaces on the cluster. Why should this generally disregard the kubeconfig NS if FLUX_SYSTEM_NAMESPACE is set?
There was a problem hiding this comment.
Yeah, if FLUX_SYSTEM_NAMESPACE is not used when e.g. -n is set, then it makes no sense to be checked here, good catch!
There was a problem hiding this comment.
Fixed in the last commit.
| expectedNamespace: "context-ns", | ||
| }, | ||
| { | ||
| name: "FLUX_SYSTEM_NAMESPACE takes precedence over context namespace", |
There was a problem hiding this comment.
I don't believe this should be the expected behaviour as explained above.
There was a problem hiding this comment.
Fixed in the last commit.
7cd113a to
cde48c1
Compare
|
@stefanprodan Please could you review this again? |
|
@stefanprodan @matheuscscp Any idea why the E2E test is failing? |
@jtyr I investigated this for a while with Opus 4.7 and this is how far I got: |
|
Thanks for providing the analysis, @matheuscscp. I have refactored
Please run the tests again so we can see if it passes now. |
|
Cool, tests pass now. Can we merge it now? ;o) |
@stefanprodan The implementation is no longer a breaking change, it's opt-in now. Can we merge? |
--ns-follows-kube-context global flag for using the kubeconfig context namespace
|
@jtyr Please squash the commits into one matching the PR title and force-push so we can merge 🙏 |
|
@matheuscscp I have squashed the commits 👍 |
Signed-off-by: Jiri Tyr <jiri.tyr@gmail.com>
|
@jtyr Thanks very much! You may not realize, but this is quite a feature! Probably top 10 requested by users historically. We should do a bit of noise about it! Can you please open a PR here: https://github.com/fluxcd/website/blob/main/content/en/roadmap.md?plain=1#L108-L113 |
|
Absolutely agree that this is a feature much longed for! Thanks @jtyr. |
Problem
The
fluxCLI ignores the namespace set in the current kubeconfig context and always defaults toflux-systemwhen--namespaceis not explicitly provided. This means users who set a namespace viakubectl config set-context --current --namespace=...still have to pass-non everyfluxcommand.Fix: #3453
Fix
Resolve the namespace from the kubeconfig context when neither
--namespacenorFLUX_SYSTEM_NAMESPACEis set. Theflux-systemdefault is preserved as the final fallback.The resulting priority order (highest to lowest):
--namespace/-nflagFLUX_SYSTEM_NAMESPACEenvironment variableflux-systemThe context lookup runs in
PersistentPreRunE(after flag parsing), so it correctly respects--contextand--kubeconfigflags when determining which context to read from.Testing
Added unit tests covering:
--contextflag selecting a different context's namespaceFLUX_SYSTEM_NAMESPACEtaking precedence over context namespace--namespaceflag taking precedence over context namespaceflux-systemwhen context has no namespace