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

Fix duplicate clustersrolebindings issue #10479

Conversation

Eric84626
Copy link
Contributor

@Eric84626 Eric84626 commented Aug 21, 2024

Type of change

  • Bugfix

Description

Duplicate ClusterRoleBindings will be attempted to be created when we set watchAnyNamespace to true and add some namespaces to watchNamespaces.

Fixes: #10378

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Write tests
  • Make sure all tests pass
  • Update documentation
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md
  • Supply screenshots for visual changes, such as Grafana dashboards

Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I left some questions for some more explanation.

Comment on lines +3 to +7
{{- $watchNamespaces := .Values.watchNamespaces -}}
{{- if $root.Values.watchAnyNamespace }}
{{- $watchNamespaces = list -}}
{{- end }}
{{- range append $watchNamespaces .Release.Namespace }}
Copy link
Member

Choose a reason for hiding this comment

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

As we are not really Helm experts, would you mind explaining (perhaps int he PR description) what exactly does this code do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure @scholzj

@@ -1,6 +1,10 @@
{{- if .Values.rbac.create }}
{{- if .Values.rbac.create -}}
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, what does the - at the end do here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice that we received a few empty lines in our YAML. Why? When the template engine runs, it removes the contents inside of {{ and }}, but it leaves the remaining whitespace exactly as is.

YAML ascribes meaning to whitespace, so managing the whitespace becomes pretty important. Fortunately, Helm templates have a few tools to help.
First, the curly brace syntax of template declarations can be modified with special characters to tell the template engine to chomp whitespace. {{- (with the dash and space added) indicates that whitespace should be chomped left, while -}} means whitespace to the right should be consumed. Be careful! Newlines are whitespace!

If it is the first line in a yaml file, I think it will not impact anything.
If you concern about it, I can remove - at the end.

@scholzj scholzj requested a review from a team August 21, 2024 07:40
@scholzj scholzj added this to the 0.44.0 milestone Aug 21, 2024
@scholzj
Copy link
Member

scholzj commented Aug 21, 2024

/azp run helm-acceptance

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -1,6 +1,10 @@
{{- if .Values.rbac.create -}}
{{- $root := . -}}
{{- range append .Values.watchNamespaces .Release.Namespace }}
{{- $watchNamespaces := .Values.watchNamespaces -}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I defined a new variable - "watchNamespaces", the default value is .Values.watchNamespaces.

{{- range append .Values.watchNamespaces .Release.Namespace }}
{{- $watchNamespaces := .Values.watchNamespaces -}}
{{- if $root.Values.watchAnyNamespace }}
{{- $watchNamespaces = list -}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If "watchAnyNamespace" is set to true, we will set variable watchNamespaces to empty List, and ignore the values of ".Values.watchNamespaces"

Loop over new variable($watchNamespaces) instead of ".Values.watchNamespaces"

@@ -1,6 +1,10 @@
{{- if .Values.rbac.create }}
{{- if .Values.rbac.create -}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice that we received a few empty lines in our YAML. Why? When the template engine runs, it removes the contents inside of {{ and }}, but it leaves the remaining whitespace exactly as is.

YAML ascribes meaning to whitespace, so managing the whitespace becomes pretty important. Fortunately, Helm templates have a few tools to help.
First, the curly brace syntax of template declarations can be modified with special characters to tell the template engine to chomp whitespace. {{- (with the dash and space added) indicates that whitespace should be chomped left, while -}} means whitespace to the right should be consumed. Be careful! Newlines are whitespace!

If it is the first line in a yaml file, I think it will not impact anything.
If you concern about it, I can remove - at the end.

Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the explanations. That is a big help.

@scholzj scholzj requested a review from a team August 21, 2024 10:45
Copy link
Member

@tombentley tombentley left a comment

Choose a reason for hiding this comment

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

Thanks!

@scholzj scholzj merged commit 8d34208 into strimzi:main Aug 22, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants