AllowedNamespaces parameter removed and breaks flux

andrewhertog commented 3 years ago

Describe the bug

We have flux configured in a tenanted format where we have a flux installation per namespaces/tenant.

We just upgraded flux from 1.18.0 to 1.24.1 and the chart from 1.2.0 to 1.11.1

We noticed quite quickly that flux stopped updating images in that namespace it was watching.

I went back to chart version 1.2.0 and used flux docker image for 1.24.1 and saw that flux immediately started working. At that point I upgraded the chart one minor version at a time to 1.11.0, and determined that it was the change from 1.10.2 to 1.11.0 that broke the chart.

I'm assuming that this change -https://github.com/fluxcd/flux/pull/3482 is not backwards compatible if allowedNamespaces is not set in the values file.

We've set this value and 1.24.1 is working again with the latest helm chart.

Steps to reproduce

Install chart version 1.10 with app version 1.24.1, and the following values in NOT the default namespace

USER-SUPPLIED VALUES:
additionalArgs:
- --git-label=flux
- --git-sync-tag=flux
annotations:
  ad.datadoghq.com/tags: '{"tenanted-flux":"true"}'
  sidecar.istio.io/inject: "false"
clusterRole:
  create: false
createCRD: false
git:
  branch: main
  path: src/
  pollInterval: 1m
  readonly: false
  secretName: flux-git
  timeout: 40s
  url: <<gitops repo url>>
  user: flux
image:
  repository: fluxcd/flux
  tag: 1.24.1
manifestGeneration: true
memcached:
  annotations:
    ad.datadoghq.com/tags: '{"tenanted-flux":"true"}'
    sidecar.istio.io/inject: "false"
  nodeSelector:
    nodegroup: system
  repository: memcached
  tag: 1.6.10-alpine
nodeSelector:
  nodegroup: system
registry:
  trace: true
sync:
  timeout: 2m

Then upgrade to chart version 1.11. Push a new container image to test registry scanning, and see that it stops working with this upgrade

Expected behavior

Flux upgrade should be backwards compatible. Fixing this required us to set the allowedNamespaces parameter.

Kubernetes version / Distro / Cloud provider

1.18 - EKS

Flux version

Flux 1.24.1 Chart 1.11.0

Git provider

Github

Container Registry provider

Jfrog Artifactory

Additional context

No response

Maintenance Acknowledgement

[X] I am aware of Flux v1's maintenance status

Code of Conduct

[X] I agree to follow this project's Code of Conduct

kingdonb commented 3 years ago

I'm very sorry this has caused you some trouble. We looked at #3482 many different ways and failed to see any way this could result in problems.

It looks like I almost hit the nail on the head with my first question in the thread:

What will be the impact of this change, on users who have provided a clusterrole and who have not set allowedNamespaces?

If I understood correctly, you have not provided any clusterrole (and you don't want one).

We judged that it was a bug, since users who provide their own clusterRole for use with a constrained set of multiple namespaces would need to disable clusterRole.create in order to provide their own, and for them, there was no extant way to set no value in --k8s-allow-namespaces. Setting this value constrains the list of namespaces that can be used, which is redundant with the way @schizoid90 was using ClusterRole (that already constrains the list of namespaces that can be used, (or at least could constrain it?) ...)

It seems we didn't consider folks who disable clusterRole.create not because they want to provide their own clusterRole, for use with several different namespaces, but because they only want Flux to work within one given ns wherever it is deployed.

Thank you for reporting this issue, we strongly intend for every release of Flux to be backwards compatible. I will investigate and figure out the best way to make this work for all.

At a glance, I think the best change would permit existing users who didn't pass any value in allowedNamespaces to keep the value of --k8s-allow-namespace=<Release.Namespace> that would have been passed before, but now also permit users who pass an empty array to override this to omit --k8s-allow-namespace as intended by #3482.

It's not much use though, if omitting --k8s-allow-namespace unintentionally disables image update automation completely.

Perhaps the best thing to do would be to simply revert #3482.

kingdonb commented 3 years ago

I believe this was resolved, the chart-1.11.2 has been published since last week.

https://github.com/fluxcd/flux/milestone/40

fluxcd / flux