fluxcd / flux2

Open and extensible continuous delivery solution for Kubernetes. Powered by GitOps Toolkit.
https://fluxcd.io
Apache License 2.0
6.58k stars 608 forks source link

Add environment variable flag to CLI in order to respect kubeconfig namespace #5028

Open adberger opened 1 month ago

adberger commented 1 month ago

This PR is a follow-up from the following issue: https://github.com/fluxcd/flux2/issues/3453

Since this is my first contribution, I probably need some help in order to get this merged.

For example: Where should I add documentation?
For the already existing flag FLUX_SYSTEM_NAMESPACE I didn't found any reference in the repository.

adberger commented 4 days ago

Anyone willing to take a look at it?

darkowlzz commented 4 days ago

Thanks for submitting the change. I'm afraid this will have to go through wider discussion with other developers and maintainers to determine how we would like to solve the issue. The discussion in https://github.com/fluxcd/flux2/issues/3453 contains two separate solutions for this without a finalized solution.

Going through some of the docs that refer to kubeconfig context namespace

it seems to me that it's a very explicit configuration that a user has to manually set. By default, it's empty. So the existence of the field seems like an explicit declaration. There could be an argument that the CLI respects the kubeconfig context and if the namespace is part of the context, it should also be respected. Absence of the context namespace, which is mostly the case, would default to the flux default.

At present, respecting the kubeconfig seems more elegant to me than introducing FLUX_NS_FOLLOW_KUBECONTEXT environment variable to signal to respect namespace too from the kubeconfig. But we'll need to gather more opinion about this to come to any conclusion.

adberger commented 4 days ago

Thanks for submitting the change. I'm afraid this will have to go through wider discussion with other developers and maintainers to determine how we would like to solve the issue. The discussion in #3453 contains two separate solutions for this without a finalized solution.

Going through some of the docs that refer to kubeconfig context namespace

it seems to me that it's a very explicit configuration that a user has to manually set. By default, it's empty. So the existence of the field seems like an explicit declaration. There could be an argument that the CLI respects the kubeconfig context and if the namespace is part of the context, it should also be respected. Absence of the context namespace, which is mostly the case, would default to the flux default.

At present, respecting the kubeconfig seems more elegant to me than introducing FLUX_NS_FOLLOW_KUBECONTEXT environment variable to signal to respect namespace too from the kubeconfig. But we'll need to gather more opinion about this to come to any conclusion.

@darkowlzz

Thank you for taking the time to take a look at this issue. I really welcome your suggested option. I thought this was undesirable as it somehow changes the current behavior of the Flux CLI.

If you have discussed/decided how & if you want to address this, please let me know if I can help in any way. I'm looking forward on using this feature, since we're working heavily with the namespace in the kubeconfig context, since using kubie.

kingdonb commented 3 days ago

There are definitely two different use cases we need to cater to, at least.

The first user is the sys-admin, who has cluster-admin, and the second user is an app-admin who likely has admin but only within a tenant namespace, or namespaces. The tenant with multiple namespaces would really like to follow the kubectx namespace, but the sys-admin with zero tenants (a single-tenant system) would prefer to have the default flux-system or whatever is set in FLUX_SYSTEM_NAMESPACE honored as the only namespace, or the most likely namespace, to have any Flux resources in it.

Since Flux is not multi-tenant enabled by default, you have to enable some patches or flags at bootstrap time, and you have to generate tenants (which bootstrap does not do) it's clear enough that the single-tenant use case is the default one.

It is a goal of the documentation to coach the early users of Flux to keep their Flux resources together, and to not put anything else inside of the "home" flux-system kustomization path. Only flux resources and/or cluster-level resources like Namespace should go there, to facilitate the GitOps workflows in the cluster, or to facilitate the creation of tenants. Tenants must be prevented from accessing those resources, only allowed control and often even visibility into their own namespaces.

So, I agree that there's a quality-of-life change necessary here, but I need to take a closer look at what this change does to see if it can handle both use cases. (Is there another use case I didn't consider? Maybe the tenant with only one namespace, but that is a special case I think the current FLUX_SYSTEM_NAMESPACE already solves well enough.)

adberger commented 1 day ago

I am a "sys-admin" in my organization and I would also prefer flux following the kubeconfig context namespace.

Since most users who work via the command line also use kubectl, I would adapt the behavior of flux to the behavior of kubectl. helm did the same thing.

There are many scripts, plugins etc. already supporting the kubeconfig context namespace, so I see no point in going in another direction here.

Since the change is very small to have our desired functionality, I'm also open to close this PR and we will just patch the Flux CLI for our needs.