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

Respect openshift cluster wide proxy #272

Merged
merged 5 commits into from
Jul 8, 2020
Merged

Respect openshift cluster wide proxy #272

merged 5 commits into from
Jul 8, 2020

Conversation

tolusha
Copy link
Contributor

@tolusha tolusha commented May 21, 2020

Signed-off-by: Anatoliy Bazko abazko@redhat.com

Reference issues

https://issues.redhat.com/browse/CRW-732

What does this PR do

Tested on OpenShift with proxy configured. Workspace was successfully created and project was cloned.

@tolusha tolusha requested review from davidfestal and mmorhun May 21, 2020 14:35
@tolusha tolusha mentioned this pull request Jun 1, 2020
34 tasks
@tolusha tolusha force-pushed the certs branch 2 times, most recently from 5127622 to dff4266 Compare June 5, 2020 14:35
@tolusha tolusha marked this pull request as ready for review June 10, 2020 09:24
cheWorkspaceNoProxy := proxy.NoProxy
if proxy.HttpProxy != "" {
if proxy.NoProxy == "" {
cheWorkspaceNoProxy = os.Getenv("KUBERNETES_SERVICE_HOST")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure if the external K8S API URL is by default in the cluster-wide proxy settings, but I don't think so.
And in a number of cases it is required to add it in the non-proxy hosts, to allow the Keycloak server to contact the OpenShift ApiServer through this external URL that is configured in Keycloak for the openshift-v4 provider in case OpenShift OAuth in enabled.

This external URL is already retrieved by the operator by this function to configure Keycloak with Openshift OAuth. So it could be added by default here since it is usually required in typical proxy-based installations.
Of course maybe a boolean field to disable this automatic addition of the external API host in the noproxy field could certainly be useful.

@che-bot
Copy link
Contributor

che-bot commented Jun 11, 2020

Latest version of Eclipse installed and tested successfully on minikube.

2 similar comments
@che-bot
Copy link
Contributor

che-bot commented Jun 11, 2020

Latest version of Eclipse installed and tested successfully on minikube.

@che-bot
Copy link
Contributor

che-bot commented Jun 11, 2020

Latest version of Eclipse installed and tested successfully on minikube.

@nickboldt
Copy link
Contributor

If the intent is to include this fix in CRW 2.2, please let me know by the end of the day 06-24, and provide a PR against the 7.14.x branch.

@nickboldt
Copy link
Contributor

Slip to 2.3.

Will this be merged in time for 7.16?

I could merge it now but pr check failing :D

@tolusha
Copy link
Contributor Author

tolusha commented Jul 7, 2020

@nickboldt
Pls pls pls never merge ;)

@nickboldt
Copy link
Contributor

Pls pls pls never merge ;)

I assume you mean "never merge if tests are failing". If so, no problem!

tolusha added 5 commits July 8, 2020 14:55
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
@che-bot
Copy link
Contributor

che-bot commented Jul 8, 2020

Latest version of Eclipse Che installed and tested successfully on minikube.

Copy link
Contributor

@davidfestal davidfestal left a comment

Choose a reason for hiding this comment

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

Seems great.
Approved, assuming that you will create a corresponding docs PR to update related documentation.
Thanks !

@tolusha tolusha merged commit 110149a into master Jul 8, 2020
@tolusha tolusha deleted the certs branch July 8, 2020 13:08
@tolusha
Copy link
Contributor Author

tolusha commented Jul 9, 2020

PR to update CRD doc
#333

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants