Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit crd-controller access to installed CRDs #215

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nikParasyr
Copy link

@nikParasyr nikParasyr commented Mar 11, 2024

What this PR does / why we need it:

Limit the access given to the crd-controler ClusterRole based on the enabled flux components. Similarly create ClusterRoleBinding only when the target component and SA is enabled.

Checklist

  • DCO signed
  • Chart Version bumped
  • helm-docs are updated
  • Helm chart is tested
  • Update artifacthub.io/changes in Chart.yaml
  • Run make reviewable

Limit the access given to the crd-controler ClusterRole
based on the enabled flux components. Similarly create
ClusterRoleBinding only when the target component and SA
is enabled.

Signed-off-by: Nikolaos Parasyris <git@nikparasyr.com>
Signed-off-by: nikParasyr <nik.parasyr@protonmail.com>
@@ -13,21 +13,31 @@ metadata:
app.kubernetes.io/part-of: flux
helm.sh/chart: {{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}
rules:
{{- if .Values.sourceController.create }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not Ok, as all other controllers need access to sources.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption was that the source controller will also be installed via the helm chart in that case and therefore this flag will be true.

Do we also facilitate cases where a subset of the controllers is installed through the helm chart and others are installed differently? Cause if that is the case then this PR needs to be re-assessed ( from my side )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants