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

feature(kuma-cp): allow custom files in chart cm #3671

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

wjrbetts
Copy link
Contributor

@wjrbetts wjrbetts commented Jan 7, 2022

The initial motivation for this is to allow the appropriate
CA file for the Postgres instance to be specified when
running kuma-cp in universal mode and then configured with
KUMA_STORE_POSTGRES_TLS_CA_PATH

Signed-off-by: Will Betts will.betts@equalexperts.com

@wjrbetts wjrbetts requested a review from a team as a code owner January 7, 2022 19:00
@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2022

Codecov Report

Merging #3671 (fc8e4a5) into master (999b7a8) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3671   +/-   ##
=======================================
  Coverage   50.90%   50.90%           
=======================================
  Files         927      927           
  Lines       56508    56508           
=======================================
+ Hits        28766    28767    +1     
+ Misses      25453    25452    -1     
  Partials     2289     2289           
Impacted Files Coverage Δ
pkg/plugins/leader/postgres/leader_elector.go 93.61% <0.00%> (-6.39%) ⬇️
pkg/defaults/components.go 86.95% <0.00%> (-4.35%) ⬇️
pkg/core/resources/manager/cache.go 85.71% <0.00%> (ø)
pkg/plugins/runtime/gateway/route/sorter.go 66.66% <0.00%> (+5.12%) ⬆️
pkg/core/runtime/component/component.go 88.67% <0.00%> (+7.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 999b7a8...fc8e4a5. Read the comment docs.

@lahabana
Copy link
Contributor

Would it be preferable to add the possibility to mount extra volumes to the cp rather than inlining a bunch of files in the configMap?

Something akin to this that would work for secrets and configMap?

@wjrbetts
Copy link
Contributor Author

Would it be preferable to add the possibility to mount extra volumes to the cp rather than inlining a bunch of files in the configMap?

Something akin to this that would work for secrets and configMap?

Thanks for your feedback @lahabana. I don't have a strong view. It would be more flexible to do what you suggest so that you can mount config to any path, rather than just files under /etc/kuma.io/kuma-control-plane. Either approach would address our current need. Do you have a preference?

@wjrbetts
Copy link
Contributor Author

Possibly the latest change is not quite what you had in mind @lahabana, but is maybe somewhere in between the two approaches. How does that seem to you?

@lahabana
Copy link
Contributor

@wjrbetts it's getting close I believe. However, what's the point of having the configMap inline in your values.yaml rather than just creating it separately and reference it here?

@wjrbetts
Copy link
Contributor Author

@wjrbetts it's getting close I believe. However, what's the point of having the configMap inline in your values.yaml rather than just creating it separately and reference it here?

Are you suggesting creating extra config maps outside of helm and just referencing them in the chart @lahabana? It seems potentially convenient to me not to have to create/destroy extra config maps separately from the rest of the resources in the chart. Very happy to change that though. Do you have a concern about having it inline?

@lahabana
Copy link
Contributor

@wjrbetts how about making both work if values is not set don't create the configMap and only mount it?
Also if we do configMaps should we do Secrets at the same time? Usually when you need configMap you often need Secrets...

Btw the tests are failing because you haven't updated the golden files see: make test UPDATE_GOLDEN_FILES=true (You can use PKG_LIST=<path to specific packages> to run only subsets of the tests.

@wjrbetts
Copy link
Contributor Author

@wjrbetts how about making both work if values is not set don't create the configMap and only mount it? Also if we do configMaps should we do Secrets at the same time? Usually when you need configMap you often need Secrets...

Btw the tests are failing because you haven't updated the golden files see: make test UPDATE_GOLDEN_FILES=true (You can use PKG_LIST=<path to specific packages> to run only subsets of the tests.

Sounds good to me, I'll give that a try.

Ah ok, thanks for the pointer

The initial motivation for this is to allow the appropriate
CA file for the Postgres instance to be specified when
running kuma-cp in universal mode and then configured with
KUMA_STORE_POSTGRES_TLS_CA_PATH

Signed-off-by: Will Betts <will.betts@equalexperts.com>
Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

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

LGTM thanks for the contribution!

@lahabana lahabana merged commit 12afcb6 into kumahq:master Jan 12, 2022
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.

3 participants