search

cloudfoundry-incubator / quarks-operator

BOSH releases deployed on Kubernetes
https://www.cloudfoundry.org/project-quarks/
Apache License 2.0
49 stars 35 forks source link

Use .Release.Namespace instead of .Release.Name as a prefix for cluster-scoped objects #1257

Open jandubois opened 3 years ago

jandubois commented 3 years ago

Right now cf-operator.fullname prefixes the chart name with the release name and not the namespace:

https://github.com/cloudfoundry-incubator/quarks-operator/blob/master/deploy/helm/quarks/templates/_helpers.tpl#L14-L25

This is used to create names for e.g. cluster roles and cluster rolebindings. The problem is that with helm 3 the release name is no longer unique (and it wasn't with helm 2 either if you had more than one tiller instance).

So you can have a cf-operator release in both ns1 and ns2. Both will try to create a cluster role called cf-operator-quarks-cluster (among others for job, secret, and statefulset), creating a collision.

If you prefix with the namespace, the role and rolebinding will be ns1-quarks-cluster and ns2-quarks-cluster, which can be removed independently when their corresponding release is removed from ns1 or ns2.

Given that the namespace-scoped objects don't include the release name in their object names, it isn't supported to install multiple releases of the same quarks chart into the same namespace anyways, so dropping the release name from the objects should not be an issue.

The kubecf cluster-scoped objects have the same problem, but I thought I would raise it here first.

cf-gitbot commented 3 years ago

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/175984792

The labels on this github issue will be updated when the story is started.

mudler commented 3 years ago

Makes perfectly sense, and this I think got mostly unnoticed since we added multi-ns support for a single operator install

manno commented 3 years ago

I don't think we have any explicit tests for installing multiple operators in a single cluster. The e2e tests on the flintstone Concourse could have catched this, but they are red. This shouldn't create any problems when updating the operator, right?

jandubois commented 3 years ago

This shouldn't create any problems when updating the operator, right?

I don't think so; during update the roles and rolebindings with the old names should get deleted, and the new ones should get created. But I've obviously not actually tested it...

manno commented 3 years ago

If we want to allow multipe operators per namespace, we could use 'monitoredID' to prefix all cluster resources. We'd also have to prefix all the release namespace resources, right now I think only webhook is missing.

However, if we decide not to support multiple per namespace, we can drop the prefixes and just prefix cluster resources with the Release.Namespace as suggested.

In any case you should be able to use Values.fullnameOverride to work around the helm chart issue. It's not a straight forward fix, since the operator update test relies on the default name and we probably have to do it for all helm charts.

jandubois commented 3 years ago

If we want to allow multipe operators per namespace

Does this make sense? In general helm charts don't really support installing the same chart multiple times into the same namespace because things like services and statefulset have fixed names. Namespace are already the mechanism to avoid name collisions, so why invent another mechanism to create essentially nested namespaces?