k8s-infra: routing, TLS (rebased) #9329

guydc · 2018-04-04T13:29:15Z

Signed-off-by: Guy Daich guy.daich@sap.com

What does this PR do?

Replaces stale PR: #8822 . All previous commits are squashed.

Introduce an External Server Exposer Strategy, responsible for exposing service ports associated with external servers, making them accessible from outside the cluster.

Move server exposure to shared k8s infra level:

Extract Openshift External Server Exposer to dedicated class.
Consolidate Openshift Server Exposer and Server converter with Kubernetes Infra.

Provide three options for exposing external (and secondary) servers in k8s infra:

multi-host: unique hostname for each component, like Che Openshift infrastructure.
single-host: single hostname for all components. Can be used in conjunction with TLS.
default-host: default ingress hostname. Can be used for local development without dynamic DNS (based on ingress IP).

Add basic TLS support:

Integrate with cert-manager in single-host configuration, to issue a LetsEncrypt certificate. Use cluster-wide issuers and ingress-shim annotations.
Add Ingress TLS provisioning.
Move TLS_ENABLED configuration to Kubernetes Infra Level.

Update Docs:

Add example value files for more convenient helm deployment of multi-user, default-host and TLS configurations.
Update readme deployment instructions.
update readme minikube setup (add RBAC)

Multiuser:

Che waits for Keycloak as well as postgres in helm deployment.
Keycloak uses a service account to poll postgres endpoint availability.

Test PR

Follow instructions to set up minikube, helm, tiller, cert-manager.

Follow specific instructions for single/multi-user, default-host installation on minikube.

Routing strategies tested locally on minikube & minishift.

What issues does this PR fix or reference?

This PR fixes #8694.
This PR is part of kubernetes infrastructure epic #5908.

Release Notes

Docs PR

Currently, documented only in the Che Kubernetes Helm deployment instructions.

Signed-off-by: Guy Daich <guy.daich@sap.com> Adapt OS infra to use external server exposer Signed-off-by: Guy Daich <guy.daich@sap.com> reuse k8s ServersConverter in OS infra begin tls for path-based k8s infra Signed-off-by: Guy Daich <guy.daich@sap.com> continue k8s tls support Signed-off-by: Guy Daich <guy.daich@sap.com> Add single-host strategy, cert-manager support Signed-off-by: Guy Daich <guy.daich@sap.com> fix charts, add single-host strategy, add TLS test Signed-off-by: Guy Daich <guy.daich@sap.com> TLS: changed cert-manager certficiate issuer to be ClusterIssuer Signed-off-by: Eyal Barlev <perspectivus@gmail.com> add resolved values.yaml Signed-off-by: Guy Daich <guy.daich@sap.com> Adapt to k8s Multiuser * Add sample values files * Fix merge issues Signed-off-by: Guy Daich <guy.daich@sap.com> Cleanup OpenShift Infra * rename TLS default value in che.env * remove openshift server exposer (consolidated with k8s) Signed-off-by: Guy Daich <guy.daich@sap.com> remove uintended automation changes Signed-off-by: Guy Daich <guy.daich@sap.com> cloud deployment helm changes Signed-off-by: Guy Daich <guy.daich@sap.com>

Signed-off-by: Guy Daich <guy.daich@sap.com>

codenvy-ci · 2018-04-04T13:31:02Z

Can one of the admins verify this patch?

codenvy-ci · 2018-04-04T13:34:07Z

Can one of the admins verify this patch?

codenvy-ci · 2018-04-04T13:34:20Z

Can one of the admins verify this patch?

ghost · 2018-04-05T10:13:31Z

I have tested the PR and can confirm #8694 is fixed. No more wrong redirects. Thanks @guydaichs

guydc · 2018-04-05T10:40:33Z

@eivantsov multi-host should work. it's on by default.

l0rd

Thanks @guydaichs

sleshchenko

Well done. Looks GOOD to me 👍

Please take a look my minor comments

sleshchenko · 2018-04-04T13:31:12Z

deploy/kubernetes/helm/che/templates/configmap.yaml

+  CHE_INFRA_KUBERNETES_SERVER__STRATEGY: {{ .Values.global.serverStrategy }}
+
+
+


Please remove extra lines

sleshchenko · 2018-04-06T05:40:43Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties

-che.infra.openshift.tls_enabled=false
+# Creates Ingresses with Transport Layer Security (TLS) enabled
+# In OpenShift infrastructure, Routes will be TLS-enabled
+che.infra.kubernetes.tls_enabled=false


Please move new Kubernetes properties above to Kubernetes section.

sleshchenko · 2018-04-06T05:49:16Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties

+che.infra.kubernetes.tls_enabled=false
+
+# Name of a secret that should be used when creating workspace ingresses with TLS
+che.infra.kubernetes.tls_secret=


Please add a note that it is ignored by OpenShift infrastructure, isn't it?

correct. done.

sleshchenko · 2018-04-06T05:50:23Z

dockerfiles/init/manifests/che.env

@@ -506,6 +507,10 @@ CHE_SINGLE_PORT=false
 # OpenShift infrastructure ignores this property because it uses Routes instead of ingresses.
 #CHE_INFRA_KUBERNETES_INGRESS_ANNOTATIONS__JSON=NULL

+# Creates Ingresses with Transport Layer Security (TLS) enabled
+# In OpenShift infrastructure, Routes will be TLS-enabled
+CHE_INFRA_KUBERNETES_TLS_ENABLED=false


Maybe it makes sense to add CHE_INFRA_KUBERNETES_TLS__SECRET here.

sleshchenko · 2018-04-06T05:54:55Z

...se/che/workspace/infrastructure/kubernetes/server/ExternalServerExposerStrategyProvider.java

+    if (externalServerExposerStrategy != null) {
+      this.externalServerExposerStrategy = externalServerExposerStrategy;
+    } else {
+      throw new IllegalArgumentException(


Maybe it would be better to throw org.eclipse.che.inject.ConfigurationException here

Signed-off-by: Guy Daich <guy.daich@sap.com>

garagatyi · 2018-04-06T13:49:48Z

@guydaichs
Git is complicated stuff but, please, do not recreate PRs unless you really have to because reviewers have to review the whole PR all over again after that. The same stuff about force pushing to the PR branch. We can always squash everything on the merge. Or you can wait until all the reviews are in place and the squash or rebase the PR.

garagatyi · 2018-04-10T06:54:07Z

@guydaichs cool contribution to the project! Congrats!

Introduce an External Server Exposer Strategy, responsible for exposing service ports associated with external servers, making them accessible from outside the cluster. Move server exposure to shared k8s infra level: - multi-host: unique hostname for each component, like Che Openshift infrastructure. - single-host: single hostname for all components. Can be used in conjunction with TLS. - default-host: default ingress hostname. Can be used for local development without dynamic DNS (based on ingress IP). Add basic TLS support. Signed-off-by: Guy Daich <guy.daich@sap.com>

sleshchenko · 2018-06-22T06:12:14Z

deploy/kubernetes/helm/che/templates/_hostHelper.tpl

@@ -1,7 +1,9 @@
 {{- define "cheHost" }}
-{{- if .Values.global.isHostBased }}
-{{- printf "master.%s" .Values.global.cheDomain }}
+{{- if eq .Values.global.serverStrategy "default-host" }}


@guydaichs Looks like after your changes readme.md is outdated. I mean the following line

If you must use an ip address (e.g. your corporate policy prevents you from using nip.io), you would also have to set isHostBased to false.

Could you please review readme.md and update information about servers strategies there?

@sleshchenko #10152

👍 Thanks

guydc added 3 commits April 4, 2018 15:06

Apply helm changes to moved files

7948d6f

Signed-off-by: Guy Daich <guy.daich@sap.com>

Fix Code Review Issues

c11e540

Signed-off-by: Guy Daich <guy.daich@sap.com>

guydc requested a review from benoitf as a code owner April 4, 2018 13:29

guydc requested a review from a user April 4, 2018 13:29

guydc requested review from garagatyi, l0rd, riuvshin, skabashnyuk and sleshchenko as code owners April 4, 2018 13:29

guydc mentioned this pull request Apr 4, 2018

k8s-infra: server routing strategies & basic TLS #8822

Closed

l0rd approved these changes Apr 5, 2018

View reviewed changes

sleshchenko approved these changes Apr 6, 2018

View reviewed changes

skabashnyuk approved these changes Apr 6, 2018

View reviewed changes

guydc added 3 commits April 6, 2018 14:41

Merge remote-tracking branch 'upstream/master' into CHE-8694-rebased

b43fcbe

Signed-off-by: Guy Daich <guy.daich@sap.com>

code review fixes

3700b94

Signed-off-by: Guy Daich <guy.daich@sap.com>

pom.xml fix

3f0b491

Signed-off-by: Guy Daich <guy.daich@sap.com>

slemeur removed the kind/epic A long-lived, PM-driven feature request. Must include a checklist of items that must be completed. label Apr 6, 2018

garagatyi approved these changes Apr 6, 2018

View reviewed changes

riuvshin approved these changes Apr 6, 2018

View reviewed changes

garagatyi merged commit a48d4b4 into eclipse-che:master Apr 10, 2018

benoitf added this to the 6.4.0 milestone Apr 10, 2018

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Apr 10, 2018

garagatyi mentioned this pull request Apr 10, 2018

Fix Openshift infrastructure #9366

Merged

sleshchenko mentioned this pull request Apr 24, 2018

Eclipse Che Always points To Localhost instead of Docker Machine #9315

Closed

sleshchenko reviewed Jun 22, 2018

View reviewed changes

guydc mentioned this pull request Jun 22, 2018

Fix Routing Strategy Docs #10152

Merged

metlos mentioned this pull request Dec 3, 2018

Introducing Jaeger tracing support to the Che Helm chart. #12049

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

k8s-infra: routing, TLS (rebased) #9329

k8s-infra: routing, TLS (rebased) #9329

guydc commented Apr 4, 2018

codenvy-ci commented Apr 4, 2018

codenvy-ci commented Apr 4, 2018

codenvy-ci commented Apr 4, 2018

ghost commented Apr 5, 2018 •

edited by ghost

Loading

guydc commented Apr 5, 2018

l0rd left a comment

sleshchenko left a comment

sleshchenko Apr 4, 2018

guydc Apr 6, 2018

sleshchenko Apr 6, 2018

guydc Apr 6, 2018

sleshchenko Apr 6, 2018

guydc Apr 6, 2018

sleshchenko Apr 6, 2018

guydc Apr 6, 2018

sleshchenko Apr 6, 2018

guydc Apr 6, 2018

garagatyi commented Apr 6, 2018

garagatyi commented Apr 10, 2018

sleshchenko Jun 22, 2018

guydc Jun 22, 2018

sleshchenko Jun 22, 2018

		CHE_INFRA_KUBERNETES_SERVER__STRATEGY: {{ .Values.global.serverStrategy }}

k8s-infra: routing, TLS (rebased) #9329

k8s-infra: routing, TLS (rebased) #9329

Conversation

guydc commented Apr 4, 2018

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

codenvy-ci commented Apr 4, 2018

codenvy-ci commented Apr 4, 2018

codenvy-ci commented Apr 4, 2018

ghost commented Apr 5, 2018 • edited by ghost Loading

guydc commented Apr 5, 2018

l0rd left a comment

Choose a reason for hiding this comment

sleshchenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garagatyi commented Apr 6, 2018

garagatyi commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 5, 2018 •

edited by ghost

Loading