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 URL with encoded path support for ChartDownloader #9824

Merged

Conversation

sathieu
Copy link
Contributor

@sathieu sathieu commented Jun 17, 2021

What this PR does / why we need it:

When the repo url contains URL-encoded parts, and doesn't end with /, relative downloads are not properly resolved.

Example, with current main (3d1bc72):

$ bin/helm repo add kubitus-ingress-operator https://gitlab.com/api/v4/projects/kubitus-project%2Fkubitus-ingress-operator/packages/helm/stable
"kubitus-ingress-operator" has been added to your repositories

$ show.go:173: [debug] Original chart version: ""
Error: failed to fetch https://gitlab.com/api/v4/projects/kubitus-project/kubitus-ingress-operator/packages/helm/stable/charts/kubitus-ingress-operator-v0.2.0.tgz : 404 Not Found
helm.go:81: [debug] failed to fetch https://gitlab.com/api/v4/projects/kubitus-project/kubitus-ingress-operator/packages/helm/stable/charts/kubitus-ingress-operator-v0.2.0.tgz : 404 Not Found
helm.sh/helm/v3/pkg/getter.(*HTTPGetter).get
        /home/circleci/helm.sh/helm/pkg/getter/httpgetter.go:73
helm.sh/helm/v3/pkg/getter.(*HTTPGetter).Get
        /home/circleci/helm.sh/helm/pkg/getter/httpgetter.go:41
helm.sh/helm/v3/pkg/downloader.(*ChartDownloader).DownloadTo
        /home/circleci/helm.sh/helm/pkg/downloader/chart_downloader.go:99
helm.sh/helm/v3/pkg/action.(*ChartPathOptions).LocateChart
        /home/circleci/helm.sh/helm/pkg/action/install.go:675
main.runShow
        /home/circleci/helm.sh/helm/cmd/helm/show.go:179
main.newShowCmd.func4
        /home/circleci/helm.sh/helm/cmd/helm/show.go:116
github.com/spf13/cobra.(*Command).execute
        /go/pkg/mod/github.com/spf13/cobra@v1.1.1/command.go:850
github.com/spf13/cobra.(*Command).ExecuteC
        /go/pkg/mod/github.com/spf13/cobra@v1.1.1/command.go:958
github.com/spf13/cobra.(*Command).Execute
        /go/pkg/mod/github.com/spf13/cobra@v1.1.1/command.go:895
main.main
        /home/circleci/helm.sh/helm/cmd/helm/helm.go:80
runtime.main
        /usr/local/go/src/runtime/proc.go:204
runtime.goexit
        /usr/local/go/src/runtime/asm_amd64.s:1374

As you can see, %2F was replaced by / which leads to 404 Not found.

We this patch applied, it works:

$ bin/helm show chart  kubitus-ingress-operator/kubitus-ingress-operator --debug
show.go:195: [debug] Original chart version: ""
apiVersion: v2
appVersion: main
dependencies:
- condition: metacontroller.enabled
  name: metacontroller
  repository: ""
description: Kubitus Ingress Operator
home: https://gitlab.com/kubitus-project/kubitus-ingress-operator
name: kubitus-ingress-operator
sources:
- https://gitlab.com/kubitus-project/kubitus-ingress-operator
type: application
version: v0.2.0

Special notes for your reviewer:

Fixes: #9977.

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

@helm-bot helm-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 17, 2021
@sathieu
Copy link
Contributor Author

sathieu commented Jun 17, 2021

@cndoit18 Please review this one (split out of #9763).

@helm-bot helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 17, 2021
@sathieu sathieu force-pushed the fix_helm_chart_download_without_trailing_slash branch from 4b91500 to 09dbc05 Compare June 17, 2021 16:03
@helm-bot helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 17, 2021
@sathieu sathieu force-pushed the fix_helm_chart_download_without_trailing_slash branch from 09dbc05 to 2ff3c98 Compare September 6, 2021 14:03
@sathieu
Copy link
Contributor Author

sathieu commented Sep 6, 2021

@mattfarina Please review this other fix about encoded slashes (similar to PR #9822).

@ghost
Copy link

ghost commented Sep 14, 2021

LGTM. Thank you for the fix!

@sathieu
Copy link
Contributor Author

sathieu commented Oct 28, 2021

@cndoit18 Please review again.

@sathieu
Copy link
Contributor Author

sathieu commented Oct 28, 2021

@bacongobbler Please review 🙏.

@sathieu
Copy link
Contributor Author

sathieu commented Dec 1, 2021

@hickeyma Please review 🙏.

@sathieu sathieu force-pushed the fix_helm_chart_download_without_trailing_slash branch from 2ff3c98 to b09fa69 Compare January 4, 2022 09:25
@sathieu
Copy link
Contributor Author

sathieu commented Jan 4, 2022

@cndoit18 @bacongobbler Please review again. I've rebased and fixed conflicts.

@sathieu sathieu force-pushed the fix_helm_chart_download_without_trailing_slash branch from b09fa69 to c2eb4bb Compare February 2, 2022 10:30
@sathieu
Copy link
Contributor Author

sathieu commented Feb 2, 2022

@cndoit18 @bacongobbler Could you please take some time to review this PR?

Signed-off-by: Mathieu Parent <math.parent@gmail.com>
@sathieu sathieu force-pushed the fix_helm_chart_download_without_trailing_slash branch from c2eb4bb to d9e5bbc Compare April 29, 2022 03:18
@sathieu
Copy link
Contributor Author

sathieu commented Apr 29, 2022

This PR is now 10 months old. Could it be added to the 3.9 milestone to ensure it is reviewed? 🙏

@hickeyma hickeyma added this to the 3.9.0 milestone Apr 29, 2022
@mattfarina mattfarina modified the milestones: 3.9.0, 3.10.0 May 18, 2022
@sathieu
Copy link
Contributor Author

sathieu commented Jun 24, 2022

This PR is now 1 year old. Any chance to get it reviewed and hopefully merged 🍀 ?

Copy link
Contributor

@hickeyma hickeyma 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 @sathieu

@hickeyma
Copy link
Contributor

This will need another Helm maintainer to review before merging.

@sathieu
Copy link
Contributor Author

sathieu commented Aug 23, 2022

Thanks @hickeyma Would you please ping an available maintainer?

@hickeyma hickeyma added the bug Categorizes issue or PR as related to a bug. label Sep 23, 2022
@hickeyma hickeyma modified the milestones: 3.10.0, 3.10.1 Sep 23, 2022
@mattfarina mattfarina merged commit 7924501 into helm:main Oct 5, 2022
@sathieu sathieu deleted the fix_helm_chart_download_without_trailing_slash branch October 5, 2022 14:47
@sathieu
Copy link
Contributor Author

sathieu commented Oct 5, 2022

Thanks @mattfarina 🙏 !

@mattfarina mattfarina added the picked Indicates that a PR has been cherry-picked into the next release candidate. label Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Categorizes issue or PR as related to a bug. picked Indicates that a PR has been cherry-picked into the next release candidate. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't use repo URLs that contain URL-encoded characters (e.g., Gitlab)
5 participants