Always honor the allowedNamespaces setting (or absence), for use with dynamically created namespaces

[david-curran-90]

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

[kingdonb]

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 clusterRole.create is false then no cluster role is created at install time, and the setting for k8s-allow-namespace is included: .Values.allowedNamespaces and .Release.Namespace are appended together and those are the allowed namespaces. It's expected that you create the named clusterRole, if you want Flux to be able to reach outside, and Flux will populate it with the appropriate role bindings in each namespace that you list in allowedNamespaces, so that it works.

(2) If the clusterRole.create is true then a cluster role is created, with cluster-wide access, and the allowedNamespaces setting is ignored to match the clusterRole, (which is not restricted unless ... it's not obvious what conditions clusterRole.create=true are actually restricted, I think that Flux will always create a clusterrole with broad permissions unless this is set to false, if you want otherwise, return to (1) and try again.)

Here's the section of the values file that covers this:

flux/chart/flux/values.yaml

Lines 43 to 50 in d551529

	# If create is `false` and no name is given, Flux will be restricted to
	# namespaces listed in allowedNamespaces and the namespace where it is
	# deployed, and the kubeconfig default context will be set to that namespace.
	clusterRole:
	create: true
	# The name of a cluster role to bind to; if not set and create is
	# true, a name based on fullname is generated
	name:

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 values.yaml from running helm upgrade --install -n fluxcd --create-namespace flux fluxcd/flux --values flux-values.yaml:

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 helm upgrade or helm install with --set, it is actually quite complicated to set array values correctly. (I would suggest for this reason using a values override file instead, to avoid the potential for misapplied/incorrectly formatted values getting discarded accidentally.)

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?

[david-curran-90]

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?

[kingdonb]

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 --k8s-allowed-namespaces is not passed into the daemon. Sounds like you want to pass in your own clusterrole so that it can be limited to certain namespaces, but they are not all known upfront and so cannot be enumerated at install time. This is a problem. It is unfortunately as I understand it a long-standing limitation of RBAC in Kubernetes.

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 kube-system (or others?)

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 --k8s-allowed-namespaces when creating your own clusterrole that is limited to certain namespaces (even if they are specifically enumerated), Flux is not smart enough to avoid reaching into namespaces that it cannot access.

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.)

[david-curran-90]

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 clusterrole.create = false the chart adds the --k8s-allow-namespaces flag. This limits Flux to only the namespaces that are specified. Which may not be known ahead of time.

My PR will omit that flag if you specify a role name. The use case would be

• Externally created role that gives access to the whole cluster
• Do not want to limit Flux to specific namespaces

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 clusterRole.name it checks for allowedNamespaces. This way a user will specify namespaces where they have a restricted ClusterRole and an empty list (unrestricted ClusterRole) will result in the k8s-allow-namespaces flag being omitted.

          {{- if and (not .Values.clusterRole.create) ( .Values.allowedNamespaces) }}
          - --k8s-allow-namespace={{ join "," (append .Values.allowedNamespaces .Release.Namespace) }}
          {{- end}}

This results in the same deployment template as when using clusterrole.create=true

[kingdonb]

To me, it makes sense to include {{ .Values.allowedNamespaces }} whenever it is provided, and not depending on the value of some other parameter. That's a surprising behavior and hard to discover.

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.

[kingdonb]

This results in the same deployment template as when using clusterrole.create=true

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!)

[kingdonb]

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 👍

[kingdonb]

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.

• Always honor the allowedNamespaces setting (or absence), for use with dynamically created namespaces

If you pass in an empty value for allowedNamespaces, or false, then the --allowed-namespaces option will be omitted.

I'll be merging this for the next chart release that will come out with Flux 1.24.0

[kingdonb]

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.

[kingdonb]

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

[kingdonb]

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

[kingdonb]

Hello, there is some discussion in #3552 from a user who had clusterRole.create set to false, and didn't pass allowedNamespaces – they alerted me that the upgrade past 1.24.0 completely disabled image update automation for them.

I just wanted to check with you,

1. Are you still using Flux v1 (or have you moved on)
2. Did you notice if the Image Update Automation feature works or not with this configuration (allowedNamespaces: [])
3. Do you have an alternate approach that could preserve backwards compatibility for the issue described in AllowedNamespaces parameter removed and breaks flux #3552?

I am afraid we may have to revert this PR, if it has spoiled backwards compatibility. There are lots of Flux users who disable clusterRole.create so that they can use Flux v1 within a single namespace, and I'm afraid we didn't consider their use case.

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

	if len(c.allowedNamespaces) == 0 {
	// All resources are allowed when all namespaces are allowed
	return true
	}

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.

[david-curran-90]

@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.