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

feat: Allow user to add service account labels #6022

Merged
merged 2 commits into from
Jan 5, 2024
Merged

Conversation

zopanix
Copy link
Contributor

@zopanix zopanix commented Dec 26, 2023

PR Description

Allow helm chart users to add service account labels

This can be useful to use OIDC required labels for grafana-agent to authenticate to services.

For example, Azure Workload Identity requires a labels to set additional labels on the service account for it to function properly.

Which issue(s) this PR fixes

n/a

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@CLAassistant
Copy link

CLAassistant commented Dec 26, 2023

CLA assistant check
All committers have signed the CLA.

@zopanix
Copy link
Contributor Author

zopanix commented Dec 26, 2023

Just to say I signed the CLA, should get updated soon enough

@@ -2,7 +2,7 @@ apiVersion: v2
name: grafana-agent
description: 'Grafana Agent'
type: application
version: 0.29.0
version: 0.30.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not increment the version quite yet. There is one other pending PR for helm, and then I'll make a new release shortly.

Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

Hi! Thanks for the contribution. I have a few comments but LGTM after that.

@@ -2,7 +2,7 @@ apiVersion: v2
name: grafana-agent
description: 'Grafana Agent'
type: application
version: 0.29.0
version: 0.30.0
Copy link
Member

Choose a reason for hiding this comment

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

Hi, can you roll this change back? We'll increment the version when the Helm chart is ready for a release; there's a small process to follow so I don't want to tie up the release with this PR.

@@ -14,6 +14,8 @@ Unreleased

- Update `rbac` to include necessary rules for the `otelcol.processor.k8sattributes` component. (@rlankfo)

- Allow helm chart users to set additional labels on the service account.
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see the name of the new field in the changelog entry:

Suggested change
- Allow helm chart users to set additional labels on the service account.
- Add `serviceAccount.additionalLabels` to values.yaml to enable setting additional labels on the created service account.

Comment on lines +8 to +10
{{- with .Values.serviceAccount.additionalLabels }}
{{- toYaml . | nindent 4 }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a corresponding test values.yaml file for this in operations/helm/charts/grafana-agent/ci and run make generate-helm-tests to generate the manifests for it? That way we can have regression tests in case this breaks.

@zopanix zopanix force-pushed the main branch 2 times, most recently from 310eb1c to aa6bb14 Compare January 5, 2024 13:13
This can be useful to use OIDC required labels for grafana-agent to
authenticate to services.

For example, Azure Workload Identity requires a labels to set additional
labels on the service account for it to function properly.
@christophe-scalepad
Copy link
Contributor

@rfratto: I've made the requested changes, I've also rebased on upstream since there were merge conflicts, should be good to go. Thanks for the feedback

Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

Thank you! Waiting for CI to pass and then I'll merge.

@rfratto rfratto merged commit c467eff into grafana:main Jan 5, 2024
10 checks passed
hainenber pushed a commit to hainenber/agent that referenced this pull request Jan 6, 2024
This can be useful to use OIDC required labels for grafana-agent to
authenticate to services.

For example, Azure Workload Identity requires a labels to set additional
labels on the service account for it to function properly.

Co-authored-by: christophe.vandekerchove <[email protected]>
BarunKGP pushed a commit to BarunKGP/grafana-agent that referenced this pull request Feb 20, 2024
This can be useful to use OIDC required labels for grafana-agent to
authenticate to services.

For example, Azure Workload Identity requires a labels to set additional
labels on the service account for it to function properly.

Co-authored-by: christophe.vandekerchove <[email protected]>
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants