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

improve support for persistent configurations #1915

Merged
merged 14 commits into from
Dec 10, 2022
Merged

Conversation

mmetc
Copy link
Contributor

@mmetc mmetc commented Dec 7, 2022

Some people like to configure completely from environment variables, others prefer persistent configuration in bind mount or persistent volumes. Since we have data as well in etc (the hub) and don't support all the possible options in envvars, persistent configurations are sometimes necessary.

  • fix a bug with parsing the BOUNCER_KEY variables
  • set all defaults in config.yaml and leave environment variables empty. This way when they are set we know that we must override the values in config.yaml.
  • ignore tainted objects when calling install/upgrade/remove
  • less verbose docker build
  • pinned yq
  • use_wal is false by default (what's the threshold to observe performance issues?)

@mmetc mmetc added docker Docker related issues area/configuration labels Dec 7, 2022
@github-actions
Copy link

github-actions bot commented Dec 7, 2022

@mmetc: There are no 'kind' label on this PR. You need a 'kind' label to generate the release automatically.

  • /kind feature
  • /kind enhancement
  • /kind fix
  • /kind chore
  • /kind dependencies
Details

I am a bot created to help the crowdsecurity developers manage community feedback and contributions. You can check out my manifest file to understand my behavior and what I can do. If you want to use this for your project, you can check out the BirthdayResearch/oss-governance-bot repository.

@LaurenceJJones
Copy link
Contributor

/kind fix

@mmetc mmetc marked this pull request as draft December 7, 2022 11:23
@mmetc mmetc force-pushed the docker-local-credentials branch 2 times, most recently from c07e60c to 7e5da3f Compare December 7, 2022 12:16
@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2022

Codecov Report

Merging #1915 (b205504) into master (10ee07c) will increase coverage by 0.20%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1915      +/-   ##
==========================================
+ Coverage   53.59%   53.79%   +0.20%     
==========================================
  Files         147      151       +4     
  Lines       19862    20243     +381     
==========================================
+ Hits        10645    10890     +245     
- Misses       8082     8178      +96     
- Partials     1135     1175      +40     
Flag Coverage Δ
func-crowdsec 47.07% <ø> (ø)
func-cscli 44.57% <ø> (-0.05%) ⬇️
unit-linux 58.43% <ø> (+0.03%) ⬆️
unit-windows 53.63% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/crowdsec-cli/machines.go 50.17% <0.00%> (-1.07%) ⬇️
pkg/cwhub/path_separator_windows.go 53.84% <0.00%> (ø)
pkg/csplugin/utils_windows.go 53.54% <0.00%> (ø)
pkg/acquisition/modules/file/tailline_windows.go 100.00% <0.00%> (ø)
...isition/modules/wineventlog/wineventlog_windows.go 71.56% <0.00%> (ø)
pkg/acquisition/modules/cloudwatch/cloudwatch.go 78.82% <0.00%> (+0.98%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mmetc
Copy link
Contributor Author

mmetc commented Dec 8, 2022

@LaurenceJJones thanks for testing but it's not finished, I'm missing a few things :)

@mmetc mmetc marked this pull request as ready for review December 8, 2022 15:50
@mmetc mmetc changed the title docker: don't overwrite persistent local_api_credentials.yaml improve support for persistent configurations Dec 9, 2022
Copy link
Contributor

@LaurenceJJones LaurenceJJones left a comment

Choose a reason for hiding this comment

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

LGTM tested via the set of user scenarios that were presented over the last couples of days.

@mmetc
Copy link
Contributor Author

mmetc commented Dec 9, 2022

LGTM tested via the set of user scenarios that were presented over the last couples of days.

I need to fix an issue with k8s and I'll merge to master. Monday we should release 1.4.4rc1

@mmetc mmetc merged commit 4097214 into master Dec 10, 2022
@mmetc mmetc deleted the docker-local-credentials branch December 10, 2022 21:09
@neilmfrench
Copy link

Does this do anything to fix crowdsecurity/helm-charts#67?

mmetc added a commit that referenced this pull request Dec 11, 2022
set all defaults in config.yaml and leave environment variables empty. This way when they are set we know that we must override the values in config.yaml.
ignore tainted objects when calling install/upgrade/remove
use_wal is false by default
@mmetc
Copy link
Contributor Author

mmetc commented Dec 11, 2022

Does this do anything to fix crowdsecurity/helm-charts#67?

Yes and no. The container should not crash, but your multi-node setup still sees a single agent, although it's mostly an aesthetic issue, all agents work correctly.
This PR should be backported in 1.4.4-rc1, likely tomorrow.

mmetc added a commit that referenced this pull request Dec 12, 2022
)

set all defaults in config.yaml and leave environment variables empty. This way when they are set we know that we must override the values in config.yaml.
ignore tainted objects when calling install/upgrade/remove
use_wal is false by default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants