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(operations/helm) add support for extraObjects #6213

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

timtalbot
Copy link

@timtalbot timtalbot commented Jan 22, 2024

PR Description

Adds support for an extraObjects field in chart values to add additional manifests with chart installation.

Which issue(s) this PR fixes

Notes to the Reviewer

Following the model used by many charts including other Grafana ones to deploy extra objects with the chart. Thanks for your time!

PR Checklist

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

@CLAassistant
Copy link

CLAassistant commented Jan 22, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@tpaschalis tpaschalis left a comment

Choose a reason for hiding this comment

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

Could you describe a couple of use cases where this could be useful?

This has come up in the past, but I'd like for us to pinpoint some real-world problems this is solving before introducing it simply because other charts do the same.

IMHO, this might be a footgun to users. How do we debug issues arising from people reusing values.yaml files they found online, or how do we deal with security concerns around having random objects reuse the agent's RBAC resources?

@tpaschalis tpaschalis self-requested a review January 22, 2024 15:11
@timtalbot
Copy link
Author

timtalbot commented Jan 22, 2024

In my case, I'd like to deploy the agent to forward Prometheus metrics to an endpoint via basic auth using a configuration that mounts the password from a volume on the pod. I do see this chart already provides a way to mount extra volumes, but I'm using secrets-store-csi-driver-provider-aws to fetch a secret from AWS and mount it into a volume. I'm hoping to define the SecretProviderClass that will know how to retrieve my AWS secret as part of the agent installation itself so that one operation installs a totally working agent in my cluster. I'm deploying the agent into multiple clusters via Cluster API so it takes some workarounds to provide extra configuration to Helm installs that isn't provided via their charts.

I'll associate the service account to an AWS IAM role with an explicit list of permissions. That said I understand your security concerns. With this change, the user needs to understand and be responsible for what they're deploying with extraObjects. I sympathize that this complicates debugging if users are copy-paste deploying things they find online.

@tpaschalis tpaschalis dismissed their stale review January 23, 2024 08:24

My own review?

@timtalbot
Copy link
Author

A more straight forward example could be the ability to deploy an additional config map or secret to use with the agent.envFrom option that already exists as part of chart installation. Has that ever come up in past discussions?

@timtalbot
Copy link
Author

@tpaschalis If there's little desire for this feature, I won't argue strongly otherwise. I've worked around it on my end. I appreciate your time looking at this regardless!

@timtalbot timtalbot changed the title feat(operations/helm) add support for extraValues feat(operations/helm) add support for extraObjects Jan 25, 2024
@tpaschalis
Copy link
Member

Hey there, apologies for the belated response. For the time being, I'd like to hold off on this and reconsider if some other use case with no workaround.

Copy link
Contributor

github-actions bot commented Mar 5, 2024

This PR has not had any activity in the past 30 days, so the needs-attention label has been added to it.
If you do not have enough time to follow up on this PR or you think it's no longer relevant, consider closing it.
The needs-attention label signals to maintainers that something has fallen through the cracks. No action is needed by you; your PR will be kept open and you do not have to respond to this comment. The label will be removed the next time this job runs if there is new activity.
Thank you for your contributions!

@github-actions github-actions bot added the needs-attention An issue or PR has been sitting around and needs attention. label Mar 5, 2024
@QuentinBisson
Copy link
Contributor

@tpaschalis The simplest use case I have is this:
How can I deploy a secret with this chart for let's say, connect to a secured endpoint like let's say grafana cloud?
Without creating the secret outside of the chart, how would you create this secret? Without this or another mechanism, then the chart is not self-sufficient

@github-actions github-actions bot removed the needs-attention An issue or PR has been sitting around and needs attention. label Mar 17, 2024
@rfratto rfratto added variant/static Related to Grafana Agent Static. variant/flow Relatd to Grafana Agent Flow. enhancement New feature or request labels Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request variant/flow Relatd to Grafana Agent Flow. variant/static Related to Grafana Agent Static.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants