Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/grafana] Allow secrets for sidecar (#13492) #15331

Merged
merged 4 commits into from
Jul 21, 2019
Merged

[stable/grafana] Allow secrets for sidecar (#13492) #15331

merged 4 commits into from
Jul 21, 2019

Conversation

fr-ser
Copy link
Contributor

@fr-ser fr-ser commented Jul 8, 2019

Updated the sidecar image to version 0.0.18
This allows also using secrets for volume mounts
This way the dashboard and data source import can be extended to those
datatypes

Signed-off-by: Sergej Herbert [email protected]

What this PR does / why we need it:

I pushed a PR into a used sidecar image (https://github.com/kiwigrid/k8s-sidecar) now we can use secrets for data source and dashboard provisioning for grafana.

Which issue this PR fixes

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • [ x] DCO signed
  • [ x] Chart Version bumped
  • [ x] Variables are documented in the README.md
  • [ x] Title of the PR starts with chart name (e.g. [stable/chart])

@helm-bot helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 8, 2019
@k8s-ci-robot k8s-ci-robot requested a review from maorfr July 8, 2019 19:42
@k8s-ci-robot
Copy link
Contributor

Hi @fr-ser. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 8, 2019
stable/grafana/templates/clusterrole.yaml Show resolved Hide resolved
stable/grafana/README.md Outdated Show resolved Hide resolved
stable/grafana/Chart.yaml Outdated Show resolved Hide resolved
@maorfr
Copy link
Member

maorfr commented Jul 9, 2019

hey @fr-ser,

thanks for this PR!
i like the idea of resourceType to define which resource type is to be used. very clever. but do we really have to support "both"?

i suggest to make this PR backwards compatible by having the default be configmap, and users can choose to use secrets if they specify it in their values.yaml.

wdyt?

at a later point we can add this change to other breaking changes and bump the major version of the chart.

@maorfr
Copy link
Member

maorfr commented Jul 9, 2019

/assign
/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 9, 2019
@fr-ser
Copy link
Contributor Author

fr-ser commented Jul 9, 2019

I would like to be backwards compatible, so I also prefer keeping configmap as default @maximbaz

But I would also like to keep the option both, as reading secrets (because of their base64 encoding or cluster permissions) is not as easy.
A cluster admin might want to keep datasources (because of the passwords that are included) as secrets but dashboards as configmaps..

@maximbaz
Copy link
Contributor

A cluster admin might want to keep datasources (because of the passwords that are included) as secrets but dashboards as configmaps..

This makes a lot of sense, and this is how I am planning to use this personally. I don't expect "configmaps" to go away in the next major version, I only want to be able to switch to "secrets" as soon as I see that I am about to put my password in there.

And in this case, if we are going to support "both" anyway, I still think introducing a new configuration option to control this isn't very useful, as opposed to just supporting both resource types out of the box.

fr-ser added 2 commits July 12, 2019 19:38
Updated the sidecar image to version 0.0.18
This allows also using secrets for volume mounts
This way the dashboard and datasource import can be extended to those
datatypes

Signed-off-by: Sergej Herbert <[email protected]>
- keep the appVersion the same
- bump minor version
- add missing apostrophe

Signed-off-by: Sergej Herbert <[email protected]>
@helm-bot helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 12, 2019
- enable sidecar resource secrets and configmaps by default
- update readme to indicate secret preference for datasources

Signed-off-by: Sergej Herbert <[email protected]>
@fr-ser
Copy link
Contributor Author

fr-ser commented Jul 12, 2019

Did a rebase to stay compatible with the current master (don't know what the rebase, merge policy is here...)

The big change is (aside from the Readme) the default watching for configmaps and secrets.

@fr-ser
Copy link
Contributor Author

fr-ser commented Jul 20, 2019

So is there anything I can do to help get this feature merged?

I can rebase/merge from master again of course, but it will be outdated within a couple of days I suppose.

@maorfr
Copy link
Member

maorfr commented Jul 21, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fr-ser, maorfr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2019
@k8s-ci-robot k8s-ci-robot merged commit 4644bc2 into helm:master Jul 21, 2019
ThoTischner pushed a commit to bitsbeats/charts that referenced this pull request Aug 13, 2019
* [stable/grafana] Allow secrets for sidecar (helm#13492)

Updated the sidecar image to version 0.0.18
This allows also using secrets for volume mounts
This way the dashboard and datasource import can be extended to those
datatypes

Signed-off-by: Sergej Herbert <[email protected]>

* [stable/grafana] Allow secret - review (helm#13492)

- keep the appVersion the same
- bump minor version
- add missing apostrophe

Signed-off-by: Sergej Herbert <[email protected]>

* [stable/grafana] Allow secret - review helm#2 (helm#13492)

- enable sidecar resource secrets and configmaps by default
- update readme to indicate secret preference for datasources

Signed-off-by: Sergej Herbert <[email protected]>
kengou pushed a commit to kengou/charts that referenced this pull request Sep 18, 2019
* [stable/grafana] Allow secrets for sidecar (helm#13492)

Updated the sidecar image to version 0.0.18
This allows also using secrets for volume mounts
This way the dashboard and datasource import can be extended to those
datatypes

Signed-off-by: Sergej Herbert <[email protected]>

* [stable/grafana] Allow secret - review (helm#13492)

- keep the appVersion the same
- bump minor version
- add missing apostrophe

Signed-off-by: Sergej Herbert <[email protected]>

* [stable/grafana] Allow secret - review #2 (helm#13492)

- enable sidecar resource secrets and configmaps by default
- update readme to indicate secret preference for datasources

Signed-off-by: Sergej Herbert <[email protected]>
ramkumarvs pushed a commit to yugabyte/charts-helm-fork that referenced this pull request Sep 30, 2019
* [stable/grafana] Allow secrets for sidecar (helm#13492)

Updated the sidecar image to version 0.0.18
This allows also using secrets for volume mounts
This way the dashboard and datasource import can be extended to those
datatypes

Signed-off-by: Sergej Herbert <[email protected]>

* [stable/grafana] Allow secret - review (helm#13492)

- keep the appVersion the same
- bump minor version
- add missing apostrophe

Signed-off-by: Sergej Herbert <[email protected]>

* [stable/grafana] Allow secret - review #2 (helm#13492)

- enable sidecar resource secrets and configmaps by default
- update readme to indicate secret preference for datasources

Signed-off-by: Sergej Herbert <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). lgtm Indicates that a PR is ready to be merged. ok-to-test size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[stable/grafana] datasource provisioning with sidecar should be done trough secrets
5 participants