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

Bug 1809555: Retrieve the list of Helm charts via chart repo proxy endpoint #4389

Merged

Conversation

pedjak
Copy link
Contributor

@pedjak pedjak commented Feb 20, 2020

The reverse proxy obtained via httputil.NewSingleHostReverseProxy does not follow redirects,
and returns such requests to UI causing CORS issue, failing to load index.yaml from UI.

Our default repo redhat-developer.github.com redirects to redhat-developer.github.io and in order
to fix index.yaml loading, we started to use redhat-developer.github.io

@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. component/backend Related to backend component/core Related to console core functionality labels Feb 20, 2020
@sbose78
Copy link

sbose78 commented Feb 20, 2020

@rohitkrai03 @pedjak ,
Could you locally apply these changes to your branch and test it,
or maybe make a single PR with both changes in ? (I can deploy the PR onto to cluster to validate it)

@sbose78
Copy link

sbose78 commented Feb 20, 2020

/test frontend

@pedjak
Copy link
Contributor Author

pedjak commented Feb 20, 2020

/hold let's test it first on some clusters

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 20, 2020
@christianvogt
Copy link
Contributor

There's a lint error in the front end code.
You can run yarn lint --fix

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 21, 2020
@pedjak
Copy link
Contributor Author

pedjak commented Feb 21, 2020

@christianvogt thanks, fixed.

@pedjak
Copy link
Contributor Author

pedjak commented Feb 21, 2020

/retest

@@ -27,7 +27,7 @@ func RegisterFlags(fs *flag.FlagSet) *config {

cfg := new(config)

fs.StringVar(&cfg.repoUrl, "helm-chart-repo-url", "https://redhat-developer.github.com/redhat-helm-charts", "Helm chart repository URL")
fs.StringVar(&cfg.repoUrl, "helm-chart-repo-url", "https://redhat-developer.github.io/redhat-helm-charts", "Helm chart repository URL")
Copy link
Contributor

Choose a reason for hiding this comment

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

https://redhat-developer.github.io/redhat-helm-charts/ is an HTML page. I'm guessing you don't want that as the default value here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we do. HTML page is for humans, but UI fetches https://redhat-developer.github.io/redhat-helm-charts/index.yaml

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't proxy any HTML since we don't want to run any JS on the console domain. We should restrict the API to only proxy to the endpoint we want.

@pedjak
Copy link
Contributor Author

pedjak commented Feb 24, 2020

/retest

@christianvogt
Copy link
Contributor

@rohitkrai03 please review and test the front end

@rohitkrai03
Copy link
Contributor

/assign

@rohitkrai03
Copy link
Contributor

rohitkrai03 commented Feb 25, 2020

Tested it locally. Fetching of charts through /api/helm/charts seems to work fine now. Although I did find some issue with installation which seems unrelated to this issue -

Screenshot from 2020-02-25 11-21-58

@pedjak
Copy link
Contributor Author

pedjak commented Feb 25, 2020

@rohitkrai03 we should also test it on a cluster

@sbose78
Copy link

sbose78 commented Feb 25, 2020

Let me deploy this on a cluster..

@pedjak
Copy link
Contributor Author

pedjak commented Feb 26, 2020

@christianvogt @rohitkrai03 Did we validate it on a cluster? Can we remove hold?

@sbose78
Copy link

sbose78 commented Feb 27, 2020

Rohit on slack: "Okay, so I just tested the PR on the above cluster deployment and everything works fine"

We'll need a confirmation on the local testing.

@sbose78
Copy link

sbose78 commented Feb 27, 2020

Predrag on slack
"ok, after going deeper into helm code.. I think it is not our code, it is local dev configuration... @Rohit check if you have any helm repos defined in your env.. helm repo list and if you do, please remove them with helm repo remove XXX Afterthat, chart installation via UI should work (tested on master)"

@rohitkrai03
Copy link
Contributor

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2020
Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

/lgtm

Verified locally as well as on cluster. Works fine.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2020
@pedjak
Copy link
Contributor Author

pedjak commented Feb 28, 2020

/assign @benjaminapetersen

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/lgtm cancel

@@ -27,7 +27,7 @@ func RegisterFlags(fs *flag.FlagSet) *config {

cfg := new(config)

fs.StringVar(&cfg.repoUrl, "helm-chart-repo-url", "https://redhat-developer.github.com/redhat-helm-charts", "Helm chart repository URL")
fs.StringVar(&cfg.repoUrl, "helm-chart-repo-url", "https://redhat-developer.github.io/redhat-helm-charts", "Helm chart repository URL")
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't proxy any HTML since we don't want to run any JS on the console domain. We should restrict the API to only proxy to the endpoint we want.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 2, 2020
The reverse proxy obtained via httputil.NewSingleHostReverseProxy does not follow redirects,
and returns such requests to UI causing CORS issue, failing to load index.yaml from UI.

Our default repo redhat-developer.github.com redirects to redhat-developer.github.io and in order
to fix index.yaml loading, we started to use redhat-developer.github.io
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 2, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pedjak, rohitkrai03, spadgett

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 2, 2020
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Mar 2, 2020

@pedjak: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/verify 67580b4 link /test verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 7e59754 into openshift:master Mar 2, 2020
@rohitkrai03
Copy link
Contributor

/retitle Bug 1809555: Retrieve the list of Helm charts via chart repo proxy endpoint

@openshift-ci-robot openshift-ci-robot changed the title Retrieve the list of Helm charts via chart repo proxy endpoint Bug 1809555: Retrieve the list of Helm charts via chart repo proxy endpoint Mar 3, 2020
@openshift-ci-robot
Copy link
Contributor

@pedjak: All pull requests linked via external trackers have merged. Bugzilla bug 1809555 has been moved to the MODIFIED state.

In response to this:

Bug 1809555: Retrieve the list of Helm charts via chart repo proxy endpoint

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@rohitkrai03
Copy link
Contributor

/cherry-pick release-4.4

@openshift-cherrypick-robot

@rohitkrai03: new pull request created: #4605

In response to this:

/cherry-pick release-4.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/backend Related to backend component/core Related to console core functionality lgtm Indicates that a PR is ready to be merged. 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.

None yet

10 participants