Installing ACK charts in namespaced mode prevents multiple installs

[ericlarssen]

Describe the bug
When you install a helm chart in namespaced mode a cluster role is created with a name not unique to the helm install.

Error: Unable to continue with install: ClusterRole "ack-namespaces-cache-acm-controller" in namespace "" exists and cannot be imported into the current release: invalid ownership metadata; annotation validation error: key "meta.helm.sh/release-namespace" must equal "test-b": current value is "test-a"

https://github.com/aws-controllers-k8s/acm-controller/blob/v1.0.7/helm/templates/caches-role.yaml#L4

I noticed this file exists in a number of charts but did not identify them all. This would not be a problem however when helm installs the files, it adds annotations unique to the the helm install, preventing multiple helm releases from owning the same resource.

meta.helm.sh/release-name: ack-chart
meta.helm.sh/release-namespace: my-namespace

Steps to reproduce

1. Helm install acm chart into a namespace (test-a), with the installScope set to namespace
2. Attempt to Helm install acm chart into another namespace (test-b), with the installScope set to namespce

Expected outcome
Both installs should work but will fail with a like error.

Error: Unable to continue with install: ClusterRole "ack-namespaces-cache-acm-controller" in namespace "" exists and cannot be imported into the current release: invalid ownership metadata; annotation validation error: key "meta.helm.sh/release-namespace" must equal "test-b": current value is "test-a"

Environment

• All k8s versions
• Not EKS related
• Afaict all helm chart

[github-actions]

Hello @ericlarssen 👋 Thank you for opening an issue in ACK! A maintainer will triage this issue soon.

We encourage community contributions, so if you're interested in tackling this yourself or suggesting a solution, please check out our Contribution and Code of Conduct guidelines.

You can find more information about ACK on our website.

[a-hilaly]

Good catch @ericlarssen ! indeed we need to make the cluster-scoped resources names unique across the cluster!

[ericlarssen]

I believe the cluster role binding will also need updated

[alexzandershevchenko]

+1 same issue with s3, apigateway/v2, iam :(

[itaiatu]

We have the same problem, but in a different scenario.

When leveraging the sharding mechanism in cluster mode, in a GitOps context, ArgoCD complains about SharedResourceWarning

Example for two ack-s3 controllers deployed on the same cluster on two different namespaces.

ClusterRole/ack-namespaces-cache-s3-controller is part of X and Y
ClusterRole/ack-s3-controller is part of X and Y
ClusterRoleBinding/ack-namespaces-cache-s3-controller is part of X and Y
ClusterRoleBinding/ack-s3-controller is part of X and Y

---

Instead of creating multiple ClusterRoles and ClusterRoleBindings with the same spec, a possible solution would be to have a feature flag for these 4 ClusterRoles and ClusterRoleBindings.

In this way, in a cluster-scoped mode, we will deploy the ClusterRole and ClusterRoleBinding only on a single shard (one ArgoCD App) and for the rest, we will disable that flag.

[alexzandershevchenko]

We have the same problem, but in a different scenario.

When leveraging the sharding mechanism in cluster mode, in a GitOps context, ArgoCD complains about SharedResourceWarning

Example for two ack-s3 controllers deployed on the same cluster on two different namespaces.

ClusterRole/ack-namespaces-cache-s3-controller is part of X and Y
ClusterRole/ack-s3-controller is part of X and Y
ClusterRoleBinding/ack-namespaces-cache-s3-controller is part of X and Y
ClusterRoleBinding/ack-s3-controller is part of X and Y

Instead of creating multiple ClusterRoles and ClusterRoleBindings with the same spec, a possible solution would be to have a feature flag for these 4 ClusterRoles and ClusterRoleBindings.

In this way, in a cluster-scoped mode, we will deploy the ClusterRole and ClusterRoleBinding only on a single shard (one ArgoCD App) and for the rest, we will disable that flag.

That sounds like overcomplicating simple things...

I'm also using ArgoCD and using simple workaround with ignoreDifferences in Application's/ApplicationSet Template spec:

  ignoreDifferences:
    - group: '*'
      kind: ClusterRole
      jsonPointers:
        - /metadata/labels/argocd.argoproj.io~1instance
    - group: '*'
      kind: ClusterRoleBinding
      jsonPointers:
        - /metadata/labels/argocd.argoproj.io~1instance

This solves warnings and continuous sync. But would be great to have correct helmcharts for all resources. I'm not sure why name generation implemented for some resources and not implemented for ClusterRole and ClusterRoleBindings... :)

[itaiatu]

That sounds like overcomplicating simple things...

We can make the ClusterRole and ClusterRoleBinding names dynamically then.

[alexzandershevchenko]

We can make the ClusterRole and ClusterRoleBinding names dynamically then.

Yep. Like

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: {{ include "ack-s3-controller.app.fullname" . }}-cache
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: {{ include "ack-s3-controller.app.fullname" . }}-cache

and so on...

[alexzandershevchenko]

I'm also using ArgoCD and using simple workaround with ignoreDifferences in Application's/ApplicationSet Template spec:

  ignoreDifferences:
    - group: '*'
      kind: ClusterRole
      jsonPointers:
        - /metadata/labels/argocd.argoproj.io~1instance
    - group: '*'
      kind: ClusterRoleBinding
      jsonPointers:
        - /metadata/labels/argocd.argoproj.io~1instance

This solves warnings and continuous sync.

well, this only solves issue with a sync, but at the end ClusterRoleBinding is incorrect :(
Proper naming is a must!

[michaelhtm]

Hello,
Thank you for the feedback/suggestions.
Seems like we need to make changes to these two https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/helm/templates/caches-role-binding.yaml.tpl
https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/helm/templates/caches-role.yaml.tpl

And regenerate all controllers.
Will keep you all updated on it!

[alexzandershevchenko]

Hello, Thank you for the feedback/suggestions. Seems like we need to make changes to these two https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/helm/templates/caches-role-binding.yaml.tpl https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/helm/templates/caches-role.yaml.tpl

And regenerate all controllers. Will keep you all updated on it!

Cool, thanks! Can we add labels to all resources for consistency?

I guess leader election role and rolebinding as well, for consistency as well :)

oh, cluster-role-controller, role-reader and role-writer has hardcoded names as well.