Skip to content

Make host calculation more short and concise for che master #10509

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.

Sign up for GitHub

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

Merged
guydc merged 3 commits into from
Aug 15, 2018
Merged

Make host calculation more short and concise for che master #10509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
124a8f1
Make host calculation more short and concise for che master
sergeykuperman Jul 23, 2018
ccd4601
Merge branch 'master' of https://github.com/eclipse/che into helmChan…
sergeykuperman Aug 7, 2018
107c550
fix hardcoded secret name in certificate.yaml
sergeykuperman Aug 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion deploy/kubernetes/helm/che/charts/che-keycloak/templates/ingress.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ spec:
tls:
- hosts:
- {{ template "keycloakHost" . }}
secretName: che-tls
secretName: {{ .Values.global.tls.secretName }}
Copy link
Copy Markdown
Member

@sleshchenko sleshchenko Aug 7, 2018

Choose a reason for hiding this comment

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

Good catch.

Looks like one more hardcoded usage of che-tls is missing: certificate.yaml

Copy link
Copy Markdown
Member

@sleshchenko sleshchenko Aug 14, 2018

Choose a reason for hiding this comment

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

@i300543 Should cerificate.yaml be updated with secretName from configuration instead of hardcoded one?

Copy link
Copy Markdown
Contributor Author

@i300543 i300543 Aug 14, 2018

Choose a reason for hiding this comment

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

well, yes it probably should :)
the certificate.yaml only relevant to letsencrypt tls flow, and we dont use it anymore
so it never came up.
i guess i will upload 1 more commit.
this shouldn't require the whole selenium checkup again does it ?

Copy link
Copy Markdown
Member

@sleshchenko sleshchenko Aug 14, 2018

Choose a reason for hiding this comment

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

I think no, it doesn't require the whole selenium checkup again

{{- end }}
rules:
{{- if eq .Values.global.serverStrategy "default-host" }}
Expand Down
4 changes: 1 addition & 3 deletions deploy/kubernetes/helm/che/templates/_hostHelper.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
{{- define "cheHost" }}
{{- if eq .Values.global.serverStrategy "default-host" }}
{{- if or (eq .Values.global.serverStrategy "default-host") (eq .Values.global.serverStrategy "single-host") }}
{{- printf "%s" .Values.global.ingressDomain }}
{{- else if eq .Values.global.serverStrategy "single-host" }}
{{- printf "che-%s.%s" .Release.Namespace .Values.global.ingressDomain }}
{{- else }}
{{- printf "che-%s.%s" .Release.Namespace .Values.global.ingressDomain }}
{{- end }}
Expand Down
8 changes: 1 addition & 7 deletions deploy/kubernetes/helm/che/templates/_keycloakAuthUrlHelper.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,10 @@
{{- define "keycloakAuthUrl" }}
{{- if eq .Values.global.serverStrategy "default-host" }}
{{- if or (eq .Values.global.serverStrategy "default-host") (eq .Values.global.serverStrategy "single-host") }}
{{- if .Values.global.tls.enabled }}
{{- printf "https://%s/auth" .Values.global.ingressDomain }}
{{- else }}
{{- printf "http://%s/auth" .Values.global.ingressDomain }}
{{- end }}
{{- else if eq .Values.global.serverStrategy "single-host" }}
{{- if .Values.global.tls.enabled }}
{{- printf "https://che-%s.%s/auth" .Release.Namespace .Values.global.ingressDomain }}
{{- else }}
{{- printf "http://che-%s.%s/auth" .Release.Namespace .Values.global.ingressDomain }}
{{- end }}
{{- else }}
{{- if .Values.global.tls.enabled }}
{{- printf "https://keycloak-%s.%s/auth" .Release.Namespace .Values.global.ingressDomain }}
Expand Down
4 changes: 1 addition & 3 deletions deploy/kubernetes/helm/che/templates/_keycloakHostHelper.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
{{- define "keycloakHost" }}
{{- if eq .Values.global.serverStrategy "default-host" }}
{{- if or (eq .Values.global.serverStrategy "default-host") (eq .Values.global.serverStrategy "single-host") }}
Copy link
Copy Markdown

@garagatyi garagatyi Jul 24, 2018

Choose a reason for hiding this comment

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

@i300543 Can you elaborate on why removing che-<namepsace> prefix from keycloakHost and cheHost would not change the behavior?

Copy link
Copy Markdown
Contributor Author

@i300543 i300543 Jul 24, 2018

Choose a reason for hiding this comment

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

The behavior would change:
lets say our ingressDomain is my-dev.myhost.com, and the namespace is my-dev
with the old logic the che wsmaster host then would be http(s)://che-my-dev.my-dev.myhost.com
and keycloak would be : http(s)://che-my-dev.my-dev.myhost.com/auth
with the new logic the host would be:
http(s)://my-dev.myhost.com (for wsmaster)
http(s)://my-dev.myhost.com/auth (for keycloak)
there is no real reason (as far as i understand) to include the namespace into the host name in case of single-host server strategy

Copy link
Copy Markdown
Contributor Author

@i300543 i300543 Jul 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

regarding the cheHost change - (in kc_realm_user.sh)
this affects the CORS headers (allowed origins) and allowed redirect hosts for keycloak, and this value should be the same as the che host, which is calculated by the _keycloakHostHelper.tpl, and not hard coded as the same logic which runs in one of the if's in the tpl script right ?

{{- printf "%s" .Values.global.ingressDomain }}
{{- else if eq .Values.global.serverStrategy "single-host" }}
{{- printf "che-%s.%s" .Release.Namespace .Values.global.ingressDomain }}
{{- else }}
{{- printf "keycloak-%s.%s" .Release.Namespace .Values.global.ingressDomain }}
{{- end }}
Expand Down
2 changes: 1 addition & 1 deletion deploy/kubernetes/helm/che/templates/certificate.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ kind: Certificate
metadata:
name: che-host-cert
spec:
secretName: che-tls
secretName: {{ .Values.global.tls.secretName }}
issuerRef:
name: letsencrypt
commonName: {{ .Values.global.cheDomain }}
Expand Down
2 changes: 1 addition & 1 deletion deploy/kubernetes/helm/che/templates/ingress.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ spec:
tls:
- hosts:
- {{ template "cheHost" . }}
secretName: che-tls
secretName: {{ .Values.global.tls.secretName }}
{{- end }}
rules:
{{- if ne .Values.global.serverStrategy "default-host" }}
Expand Down
2 changes: 1 addition & 1 deletion dockerfiles/keycloak/kc_realm_user.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ if [ "${CHE_KEYCLOAK_ADMIN_REQUIRE_UPDATE_PASSWORD}" == "false" ]; then
fi

cat /scripts/che-realm.json.erb | \
sed -e "s@<%= scope\.lookupvar('che::che_server_url') %>@${PROTOCOL}://che-${NAMESPACE}.${ROUTING_SUFFIX}@" \
sed -e "s@<%= scope\.lookupvar('che::che_server_url') %>@${PROTOCOL}://${CHE_HOST}@" \
> /scripts/che-realm.json

echo "Creating Admin user..."
Expand Down