Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

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

Merged
merged 1 commit into from
Aug 20, 2021
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion chart/flux/templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ spec:
name: {{ .Values.env.secretName }}
{{- end }}
args:
{{- if not .Values.clusterRole.create }}
{{- if and (not .Values.clusterRole.create) ( .Values.allowedNamespaces) }}
Copy link
Member

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.

Copy link
Member

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

Copy link
Member

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

- --k8s-allow-namespace={{ join "," (append .Values.allowedNamespaces .Release.Namespace) }}
{{- end}}
{{- if .Values.defaultNamespace }}
Expand Down