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

fix: add insecure-skip-tls-verify on helm pull when Creds.InsecureSkipVerify is set to true #6458

Merged
merged 2 commits into from
Jul 20, 2021

Conversation

elucidator
Copy link
Contributor

@elucidator elucidator commented Jun 11, 2021

When using an insecure helm repository you are allowed to add that on including the "--insecure-skip..." see (https://github.com/argoproj/argo-cd/blob/master/util/helm/cmd.go#L160)

But when doing an actual fetch for the helm chart this parameter gets omitted.

This PR adds this to the Fetch method (https://github.com/argoproj/argo-cd/blob/master/util/helm/cmd.go#L211)
Fixes:
#4258
#3693
#6376

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

@elucidator elucidator changed the title fix: add "--insecure-skip-tls-verify" on helm pull when Creds.InsecureSkipVerify is set to true fix: add insecure-skip-tls-verify on helm pull when Creds.InsecureSkipVerify is set to true Jun 11, 2021
@elucidator elucidator marked this pull request as ready for review June 11, 2021 15:19
Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

Thanks a lot @elucidator ! Added one minor comment

util/helm/cmd.go Outdated
@@ -219,6 +219,9 @@ func (c *Cmd) Fetch(repo, chartName, version, destination string, creds Creds) (
if creds.Password != "" {
args = append(args, "--password", creds.Password)
}
if creds.InsecureSkipVerify {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change to creds.InsecureSkipVerify && c.insecureSkipVerifySupported, simiar to:

if opts.InsecureSkipVerify && c.insecureSkipVerifySupported {

The --insecure-skip-tls-verify was added recently to helm client and not supported by older versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be needed for the login command as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry @elucidator, I did not notice your update. For consistency is good to add the same check to Login but not must have: Login command is only available in helm3 which supports skip verify so we can leave it as is.

@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #6458 (6071d8f) into master (6f95950) will increase coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head 6071d8f differs from pull request most recent head a8e1d28. Consider uploading reports for the commit a8e1d28 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6458   +/-   ##
=======================================
  Coverage   41.07%   41.07%           
=======================================
  Files         152      152           
  Lines       20144    20146    +2     
=======================================
+ Hits         8274     8275    +1     
  Misses      10734    10734           
- Partials     1136     1137    +1     
Impacted Files Coverage Δ
util/helm/cmd.go 29.44% <0.00%> (-0.37%) ⬇️
util/helm/client.go 45.97% <0.00%> (+0.57%) ⬆️

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 6f95950...a8e1d28. Read the comment docs.

@elucidator elucidator requested a review from alexmt June 14, 2021 20:25
Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-verification PR requires pre-release verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants