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

Add support for secrets for helm v2 #79

Merged

Conversation

laghoule
Copy link
Contributor

It's possible to use helm v2 with added security mesure (disable tiller svc & secrets instead of configmaps):

helm init --history-max 5 --service-account tiller --override 'spec.template.spec.containers[0].command'='{/tiller,--listen=127.0.0.1:44134,--storage=secret}

In order to support this feature, I have add a flag --helm-store to the detect-helm command. I have tested it with my EKS sandbox cluster (1.15.x) with installed helm 2.14.2.

@laghoule
Copy link
Contributor Author

I forgot to update the readme.md. If you're interresed with this PR, I will update it.

Copy link
Contributor

@lucasreed lucasreed left a comment

Choose a reason for hiding this comment

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

Hey @laghoule, thanks so much for this PR! I've commented inline for some change requests as well as a couple questions I had.

.gitignore Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
cmd/root.go Outdated Show resolved Hide resolved
pkg/helm/helm.go Outdated Show resolved Hide resolved
pkg/helm/helm.go Outdated Show resolved Hide resolved
pkg/helm/helm.go Outdated Show resolved Hide resolved
@lucasreed
Copy link
Contributor

I forgot to update the readme.md. If you're interresed with this PR, I will update it.

An update to the README would be great! Fine to add that at the end when the other review items are updated. Thanks!

Copy link
Member

@sudermanjr sudermanjr left a comment

Choose a reason for hiding this comment

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

Love the addition, thanks!

I think Luke covered most of the things in here, I just added a couple little things that I noticed.

cmd/root.go Outdated Show resolved Hide resolved
pkg/helm/helm.go Outdated Show resolved Hide resolved
@sudermanjr
Copy link
Member

The venom tests aren't running right now due to Quay outage. I don't think your changes will break them, but you might consider adding some to cover the new functionality

@laghoule
Copy link
Contributor Author

I have already made a test coverage of the new functionality ;)

@codecov
Copy link

codecov bot commented May 28, 2020

Codecov Report

Merging #79 into master will increase coverage by 0.25%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #79      +/-   ##
==========================================
+ Coverage   78.23%   78.49%   +0.25%     
==========================================
  Files           6        6              
  Lines         363      372       +9     
==========================================
+ Hits          284      292       +8     
- Misses         52       53       +1     
  Partials       27       27              
Impacted Files Coverage Δ
pkg/helm/helm.go 65.95% <83.33%> (+2.42%) ⬆️

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 f7e3a06...c526c8e. Read the comment docs.

@sudermanjr
Copy link
Member

I have already made a test coverage of the new functionality ;)

Sorry, I wasn't clear. We have two sets of tests, functional and unit. You got the unit tests covered (which is awesome, thank you).

We also have functional testing with Venom in the e2e directory. Those are the ones I was referring to.

Copy link
Member

@sudermanjr sudermanjr 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 quick changes!

Copy link
Contributor

@lucasreed lucasreed left a comment

Choose a reason for hiding this comment

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

I did add one more minor nitpick change, if you wouldn't mind accepting my suggested modification.

Otherwise this looks great, thank you so much!! 🎉

e2e/tests/02_helm-detect-2.yaml Outdated Show resolved Hide resolved
@lucasreed lucasreed merged commit 81c8d32 into FairwindsOps:master May 29, 2020
@pwhitehead00
Copy link

Any chance this could be cut in a new release? thanks!!

@lucasreed
Copy link
Contributor

Any chance this could be cut in a new release? thanks!!

Yep! We have something else we want to get in before the new release, but it should be soon.

@sudermanjr
Copy link
Member

Just released v2.3.0 which contains that change. Thanks again!

@pwhitehead00
Copy link

thank you!!!

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.

None yet

4 participants