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

Optionally pass cortex config as configmap. #280

Merged
merged 3 commits into from
Dec 6, 2021

Conversation

jmcarp
Copy link
Contributor

@jmcarp jmcarp commented Dec 1, 2021

What this PR does:
Optionally pass cortex config as configmap.

Note: I only updated the ingestor templates so far, to verify that the approach makes sense. If it does, I'll copy and paste the updated volume to the other templates.

Which issue(s) this PR fixes:
Fixes #235.

Checklist

  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@jmcarp jmcarp force-pushed the jmcarp/configmap branch 8 times, most recently from dfc2be0 to d2caf18 Compare December 2, 2021 01:55
@nschad
Copy link
Collaborator

nschad commented Dec 3, 2021

Nice!

with that approach however, we should really have the s3 credentials (or similiar) not in the configMap. I'm not saying Secrets are "secure" but atleast you can install some KMS provider where you can actually encrypt the data.

But with extraVolumes, extraVolumeMounts, extraEnvs and extraArgs this should already be possible. I would have to test that but I think you can already have sensitive information separately. Use environment variables in the configuration

@jmcarp jmcarp marked this pull request as ready for review December 3, 2021 16:52
values.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@nschad nschad left a comment

Choose a reason for hiding this comment

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

Hey, thank you for your contribution.

Can you please add the CHANGELOG.md entry?
Also we would need to add a mechanism similiar to: This
We should also have ci-tests setup and documentation guide for proper usage so nobody puts their secrets in the configmap

So we can restart components when configmap changes, right now we can't use something like configmap reloader AFAIK

@jmcarp
Copy link
Contributor Author

jmcarp commented Dec 5, 2021

Updated, lmk if I missed anything.

Copy link
Collaborator

@nschad nschad left a comment

Choose a reason for hiding this comment

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

I will have another glance tomorrow but this looks really good. Thanks!

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@nschad nschad left a comment

Choose a reason for hiding this comment

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

I re-edited the guide, because inline code blocks completely broke the the text unfortunately. We will have to adjust the markdown processor or something like that in the future

@nschad nschad requested a review from kd7lxl December 6, 2021 10:28
templates/configmap.yaml Outdated Show resolved Hide resolved
@nschad
Copy link
Collaborator

nschad commented Dec 6, 2021

@jmcarp Please be careful when rebasing, my changes to the guide were lost 😢

@jmcarp
Copy link
Contributor Author

jmcarp commented Dec 6, 2021

Ack, sorry, not used to collaborating on a branch 😓

@nschad nschad merged commit 731c145 into cortexproject:master Dec 6, 2021
@nschad nschad mentioned this pull request Dec 29, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deploy the config as a configMap rather than a secret
3 participants