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

[bitnami/redis] Do not create master and replica serviceaccounts when using sentinel #22716

Merged
merged 3 commits into from
Jan 26, 2024

Conversation

wilmardo
Copy link
Contributor

@wilmardo wilmardo commented Jan 25, 2024

Description of the change

Currently when choosing to use sentinel you get all the serviceaccounts not just the one needed.

Currently is creates the master and replica service accounts:

default          0         8h
redis1           0         73m
redis1-master    0         64s
redis1-replica   0         64s
redis2           0         51m
redis2-master    0         62s
redis2-replica   0         62s

Benefits

Less clutter, removes unneeded resources. More inline with the other resources in the master/ and replica/ template folder. They all have this if check, this is the one resource where it was missing.

Checklist

  • Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
  • Variables are documented in the values.yaml and added to the README.md using readme-generator-for-helm
  • Title of the pull request follows this pattern [bitnami/<name_of_the_chart>] Descriptive title
  • All commits signed off and in agreement of Developer Certificate of Origin (DCO)

@wilmardo
Copy link
Contributor Author

wilmardo commented Jan 25, 2024

I could add a check like this to the serviceaccount creation: ``` {{- if and .Values.sentinel.enabled .Values.serviceAccount.create }} ``` Which would be more inline with the previous if statement but to me it makes more sense that the `ServiceAccount` creation only depends on the true/false of the `.Values.serviceAccount.create`. The `RoleBinding` references this `ServiceAccount` for example so then a working statement would be:
{{- if  and .Values.serviceAccount.create .Values.sentinel.enabled .Values.rbac.create }}

Again, just depending on the create true/false makes more sense to avoid edge cases like this in the future

Not relevant, solved in #22223

@carrodher carrodher added verify Execute verification workflow for these changes in-progress labels Jan 25, 2024
@github-actions github-actions bot removed the triage Triage is needed label Jan 25, 2024
@github-actions github-actions bot removed the request for review from javsalgar January 25, 2024 12:43
@github-actions github-actions bot requested a review from Mauraza January 25, 2024 12:43
@italoiz
Copy link

italoiz commented Jan 25, 2024

Is this PR a duplicate of #22223 ?

@Mauraza
Copy link
Contributor

Mauraza commented Jan 25, 2024

Hi @wilmardo,

Thanks for the contribution!! Could you check the conflicts?

@wilmardo wilmardo changed the title [bitnami/redis] Fix sentinel statefulset not starting with the latest chart [bitnami/redis] Do not create master and replica serviceaccounts when using sentinel Jan 25, 2024
@wilmardo
Copy link
Contributor Author

wilmardo commented Jan 25, 2024

@italoiz yes excuse me duplicated indeed. Dedupped the PR and changed the description/title to reflect the dedup

@Mauraza rebased and resolved the conflicts.

@wilmardo wilmardo force-pushed the redis-sa-fixes branch 3 times, most recently from 4a75994 to 6d028a4 Compare January 25, 2024 17:06
Mauraza
Mauraza previously approved these changes Jan 26, 2024
Copy link
Contributor

@Mauraza Mauraza left a comment

Choose a reason for hiding this comment

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

Hi @wilmardo,

Thanks for the contribution!!! LGTM!!

bitnami/redis/Chart.yaml Outdated Show resolved Hide resolved
Co-authored-by: Ibone González Mauraza <[email protected]>
Signed-off-by: Wilmar den Ouden <[email protected]>
Signed-off-by: Ibone González Mauraza <[email protected]>
@Mauraza Mauraza enabled auto-merge (squash) January 26, 2024 11:00
@Mauraza Mauraza removed the verify Execute verification workflow for these changes label Jan 26, 2024
@Mauraza Mauraza added the verify Execute verification workflow for these changes label Jan 26, 2024
@Mauraza Mauraza merged commit 13c6479 into bitnami:main Jan 26, 2024
15 checks passed
Wielewout added a commit to Wielewout/bitnami-charts that referenced this pull request Jan 26, 2024
Since 18.8.3 (bitnami#22716) no service account is created with architecture set to standalone.

In this case the serviceaccount and application condition didn't match for master (instead the condition for replicas was used for the master service account).
Wielewout added a commit to Wielewout/bitnami-charts that referenced this pull request Jan 26, 2024
Since 18.8.3 (bitnami#22716) no service account is created with architecture set to standalone.

In this case the serviceaccount and application condition didn't match for master (instead the condition for replicas was used for the master service account).

Signed-off-by: Wout Van De Wiel <[email protected]>
Wielewout added a commit to Wielewout/bitnami-charts that referenced this pull request Jan 26, 2024
Since 18.8.3 (bitnami#22716) no service account is created with architecture set to standalone.

In this case the serviceaccount and application condition didn't match for master (instead the condition for replicas was used for the master service account).

Signed-off-by: Wout Van De Wiel <[email protected]>
Wielewout added a commit to Wielewout/bitnami-charts that referenced this pull request Jan 26, 2024
Since 18.8.3 (bitnami#22716) no service account is created with architecture set to standalone.

In this case the serviceaccount and application condition didn't match for master (instead the condition for replicas was used for the master service account).

Signed-off-by: Wout Van De Wiel <[email protected]>
Wielewout added a commit to Wielewout/bitnami-charts that referenced this pull request Jan 26, 2024
Since 18.8.3 (bitnami#22716) no service account is created with architecture set to standalone.

In this case the serviceaccount and application condition didn't match for master (instead the condition for replicas was used for the master service account).

Signed-off-by: Wout Van De Wiel <[email protected]>
migruiz4 pushed a commit that referenced this pull request Jan 29, 2024
Since 18.8.3 (#22716) no service account is created with architecture set to standalone.

In this case the serviceaccount and application condition didn't match for master (instead the condition for replicas was used for the master service account).

Signed-off-by: Wout Van De Wiel <[email protected]>
anthosz pushed a commit to anthosz/charts that referenced this pull request Feb 1, 2024
… using sentinel (bitnami#22716)

* fix(redis): skip creation of master and replica serviceaccount when using sentinel

Signed-off-by: wilmarguida <[email protected]>

* chore(redis): bump the chart version

Co-authored-by: Ibone González Mauraza <[email protected]>
Signed-off-by: Wilmar den Ouden <[email protected]>

---------

Signed-off-by: wilmarguida <[email protected]>
Signed-off-by: Wilmar den Ouden <[email protected]>
Signed-off-by: Ibone González Mauraza <[email protected]>
Co-authored-by: Ibone González Mauraza <[email protected]>
Co-authored-by: Ibone González Mauraza <[email protected]>
anthosz pushed a commit to anthosz/charts that referenced this pull request Feb 1, 2024
Since 18.8.3 (bitnami#22716) no service account is created with architecture set to standalone.

In this case the serviceaccount and application condition didn't match for master (instead the condition for replicas was used for the master service account).

Signed-off-by: Wout Van De Wiel <[email protected]>
joancafom pushed a commit to dalbani/charts that referenced this pull request Feb 22, 2024
… using sentinel (bitnami#22716)

* fix(redis): skip creation of master and replica serviceaccount when using sentinel

Signed-off-by: wilmarguida <[email protected]>

* chore(redis): bump the chart version

Co-authored-by: Ibone González Mauraza <[email protected]>
Signed-off-by: Wilmar den Ouden <[email protected]>

---------

Signed-off-by: wilmarguida <[email protected]>
Signed-off-by: Wilmar den Ouden <[email protected]>
Signed-off-by: Ibone González Mauraza <[email protected]>
Co-authored-by: Ibone González Mauraza <[email protected]>
Co-authored-by: Ibone González Mauraza <[email protected]>
Signed-off-by: Jose Antonio Carmona <[email protected]>
joancafom pushed a commit to dalbani/charts that referenced this pull request Feb 22, 2024
Since 18.8.3 (bitnami#22716) no service account is created with architecture set to standalone.

In this case the serviceaccount and application condition didn't match for master (instead the condition for replicas was used for the master service account).

Signed-off-by: Wout Van De Wiel <[email protected]>
Signed-off-by: Jose Antonio Carmona <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
redis solved verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants