-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Always honor the allowedNamespaces setting (or absence), for use with dynamically created namespaces #3482
Conversation
c59f337
to
c0cf80f
Compare
Thank you for your contribution! I'm trying to understand any potential unforeseen implications of making a change like this. What will be the impact of this change, on users who have provided a clusterrole and who have not set allowedNamespaces? It already looks like a bit of a mess in here. I'm not sure this change is needed, after spending some time looking into it. Explained below, hope this helps. Current state (as I understand it): (1) If the (2) If the Here's the section of the values file that covers this: Lines 43 to 50 in d551529
Neither of these conditions should actually limit the access of Flux to only the namespace it is deployed in as you've described, unless allowedNamespaces actually hasn't been set at all. Can you provide the values you installed Flux with in order to get these results? I think they must be wrong. I just tried this for myself, and got this result (snipped from flux deployment): spec:
containers:
- args:
- --k8s-allow-namespace=bar,baz,bep,fluxcd
... Here is my flux clusterRole:
create: false
name: foo
allowedNamespaces:
- bar
- baz
- bep I haven't incorporated your change, and got these results with no problem... I think most likely, you have invoked allowedNamespaces incorrectly which would explain why only the flux install namespace is in this list. If you are using Is it possible that you passed in a comma-separated string, or somehow other incorrectly populated allowedNamespaces with some data that Helm has discarded, instead of the expected array of namespace strings? |
I am also seeing the allowed namespaces before incorporating my change. I am solving the situation where we are using lots of namespaces that can be created by many users as required, we would have to be updating the list of allowed namespaces constantly. Perhaps I am misunderstanding then. In the event that the named clusterRole exists does Flux ignore the k8s-allow-namespaces setting? |
Ah, I see. Does the clusterrole you create for Flux already have unbounded access ahead of installing flux, or is it limited somehow? This may be a real limitation you've stumbled onto, which came up in a discussion recently (so I think I understand it...) Flux will limit itself to only reaching into named namespaces when allowedNamespaces is set, and it will try to monitor all namespaces when Please check the discussion in fluxcd/flux2#1398 and clarify if this matches your intended use case. Are you trying to implement "deny rule" and give Flux a clusterrole that has access to everything except for If so, this apparently is not possible. RBAC roles must choose between an un-restricted wildcard and a specific enumeration. If we make a change that allows you to omit So flux will try reaching into them, and emit errors. Like RBAC itself, Flux needs that specific enumeration of namespaces. Please let me know if this is your use case; I don't think we can merge this change as-is, but I'd like to have a better answer than "sorry can't help you" as this is apparently a common question and I want to make sure Flux can work with dynamically provisioned namespaces. (I'd personally bet the farm that self-service namespaces are going to be the next big thing.) |
I have an unrestricted clusterRole that I've created outside of this deployment. I want to use that clusterRole with Flux but when I specify My PR will omit that flag if you specify a role name. The use case would be
I can see now that unintended consequence could be Flux would then be attempting to reach namespaces it has no access to should an administrator limit that role within RBAC. Perhaps instead of checking for
This results in the same deployment template as when using |
To me, it makes sense to include We have to change this carefully if at all. Because Flux v1 is in maintenance mode, breaking changes cannot be permitted and any behavior change is a breaking change. If someone upgrades to the next version of Flux and does not change any values, then nothing about their deployment should change unless it fixes a bug. This should hold true for any valid configuration of Flux; if you want to introduce a behavior change to address a bug, I would suggest it should usually be done through a new option which would not have been a valid/effective configuration before. |
If that's the case, great. The DCO check will block merging until it is satisfied, please following the instructions in https://github.com/fluxcd/flux/pull/3482/checks?check_run_id=2557151750 I will need some time to evaluate this change and see if it's possible to sneak it in, or if some different change is better. Please squash and signoff your commits, I'm marking them for the 1.23.0 release though they will still need to be considered and tested further to be sure there are no un-intended consequences. Please also consider adding a note about how to accomplish your use case with this in the source code, so it will be discoverable as documentation after the PR is merged, (if it can actually be approved, can't make any promises at this point!) |
Hello friend, I am rebasing a few PRs that remain outstanding so they are caught up to master. I found yours had a merge conflict so I squashed the commits and resolved it. Please take a look if you have time, to help me be certain that I did it correctly. I am forced pushing over your branch from a2e7a40 to ad2eff5 – these commits appear to be identical 👍 |
2e0b733
to
150e033
Compare
Found an issue where, when providing your own clusterRole, flux is not able to provision in namespaces other than it's own. The work around is to add all namespaces to the allowedNamespaces value but this change will not add that option if a clusterRole name has been provided * Add second condition for adding allowed namspeaces Signed-off-by: David Curran <david.curran@health.telstra.com> Signed-off-by: Kingdon Barrett <kingdon@weave.works>
I think we talked this issue out some time ago, it was quite complex. The change is really simple. It just does what is in the title of this issue.
If you pass in an empty value for I'll be merging this for the next chart release that will come out with Flux 1.24.0 |
@@ -195,7 +195,7 @@ spec: | |||
name: {{ .Values.env.secretName }} | |||
{{- end }} | |||
args: | |||
{{- if not .Values.clusterRole.create }} | |||
{{- if and (not .Values.clusterRole.create) ( .Values.allowedNamespaces) }} |
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.
Testing this by hand with helm template
With a values of:
clusterRole:
create: false
allowedNamespaces: []
there is no --allow-namespaces
string present at all in the template output. It is not in the Flux daemon's deployment args
, (this is the state that could not be configured before this change is merged.)
With a value of:
clusterRole:
create: false
allowedNamespaces:
- abc
- def
the --allow-namespace
option is as follows:
- --k8s-allow-namespace=abc,def,default
(since the Flux namespace is default
in this case as I am installing with the helm chart in the default namespace, I believe this is correct)
In the template output by 1.10.2 Flux Helm Chart version, this outcome is impossible. The allow-namespace would still get passed with the Flux install namespace, something like this:
- --k8s-allow-namespace=default
The allow-namespace directive is still used when the clusterRole.create
is false, because it is assumed you will list the namespaces that Flux can access. I think this actually suggests there is another patch needed for your use case, where you have already provided a clusterRole and it need not be created for you by Flux's helm chart.
Not to delay the merging of this patch any longer, since it has been blocked from merging for long enough.
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 I have that a bit backwards, with clusterRole.create
set to false, allow-namespace can be used, but with clusterRole.create
set to true, allow-namespace is never used. This behavior is the same as before, as Flux can apparently assume that when a ClusterRole is created, it will have unfettered access to all namespaces.
I'm not sure this is correct. Mulling it over out loud while I decide whether this can merge as-is or if it needs something additional. We can always patch it again, but I'd rather be sure I'm not breaking anything at all inside of the Helm chart, as I'm sure you'll understand
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 guess it makes sense. You can create a clusterRole that is allowed access to only certain namespaces, but Flux won't create one. Flux only creates a clusterRole that has unfettered access. So it would be wrong to change that, as it will be a breaking change for people who have that configuration. I'm going to leave it alone.
My judgment is this PR can merge as-is 👍 LGTM
Hello, there is some discussion in #3552 from a user who had I just wanted to check with you,
I am afraid we may have to revert this PR, if it has spoiled backwards compatibility. There are lots of Flux users who disable If we can preserve the intent of your original PR without breaking image updates for those users, I will happily keep the behavior. If you moved on, though, since Flux v1 maintenance is likely nearing its end, and this change would represent more new development on a feature that wasn't present before maintenance, I would prefer to simply revert the change, preserving backwards compatibility. It seems this bug was deeper than it looks. flux/pkg/cluster/kubernetes/kubernetes.go Lines 353 to 356 in 4358a3b
I can see here the intent is, when allowedNamespaces is zero length, all namespaces should be allowed. So, the jury is out on where the fault lies in the code now. I am leery to make any further changes in this thread unless we are certain they will solve all associated issues with this change. |
@kingdonb I'm in favour of reverting the PR if it's causing issues. I've moved away from v1 myself so not in a position to try anything different to fix the issue. |
Found an issue where, when providing your own clusterRole, flux is not able to provision in namespaces other than it's own. The work around is to add all namespaces to the allowedNamespaces value but this change will not add that option if a clusterRole name has been provided