-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 an ability to use TLS with K8s infra and helm chart #13946
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jul 22, 2019
benoitf
reviewed
Jul 24, 2019
sleshchenko
force-pushed
the
helm-tls
branch
3 times, most recently
from
July 25, 2019 08:21
6445b27
to
7605ab6
Compare
sleshchenko
requested review from
amisevsk,
ibuziuk,
metlos,
nickboldt and
rhopp
as code owners
July 25, 2019 08:56
ci-build |
sleshchenko
changed the title
Change TLS configuring in helm chart
Fix an ability to use TLS with K8s infra and helm chart
Jul 26, 2019
This was referenced Jul 26, 2019
I'm on PTO until monday |
amisevsk
approved these changes
Jul 26, 2019
assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/che/che.properties
Outdated
Show resolved
Hide resolved
benoitf
reviewed
Jul 29, 2019
benoitf
approved these changes
Jul 29, 2019
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on GCP
I've updated document for GCP / TLS installation
Signed-off-by: Sergii Leshchenko <[email protected]>
Signed-off-by: Sergii Leshchenko <[email protected]>
Signed-off-by: Sergii Leshchenko <[email protected]>
Signed-off-by: Sergii Leshchenko <[email protected]>
Signed-off-by: Sergii Leshchenko <[email protected]>
Signed-off-by: Sergii Leshchenko <[email protected]>
Signed-off-by: Sergii Leshchenko <[email protected]>
Signed-off-by: Sergii Leshchenko <[email protected]>
ci-build |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
It's ready to review but note that I'm still testing these changes for different combinations of configuration.
The corresponding pull request will be created to chectl before merging this.
It inlcudes the following changes that are separated in different commits to make review easier:
useCertManager
,useStaging
,email
was related toClusterIssuer
but I discovered thatsingle
anddefault
host server strategies are not working at all, but only that configurations allows to set up certificate manager easily.With
multi-host
server strategy, wildcard certificates are required, which require DNS challenge to be done and it is really specific to DNS that is used on installation. With some DNS providers, we can quite easily configure automated way of DNS challenge (like Cloud DNS), on some user has to do it manually. So, I do not see a straightforward and clean way how we can write Helm Chart to handle it for all installations. This definitely can be improved and automated, but I believe that requiring a secret with certificate ready-to-use and provide instruction on how to get it on some installation is a good working start point.So, creating of certificate and clusterissuer and related values are removed from chart.
There is a default strategy for workspaces namespaces, to create a workspace in a dedicated namespace. In such configuration, user would need to set up replication for TLS certificate whole all Che namespaces
OR (it's implemented here) Che Server may propagate certificate for workspaces, it just needs to know private key and certificate. It includes Che Server + Helm Chart changes
During testing I've found that
cheNamespace
is not used anywhere, then investigated a bit when it was used, and discovered that it's the outdated name ofcheWorkspacesNamespace
.See a48d4b4#diff-7e81184b59cb2730e14842271ecf4261R34
19ecb7d
So, it's removed. To get namespace where Che is located,
.Release.Namespace
can be used in templates.When TLS is configured, Che Server must be provided with
che-tls
certificate and if this cert is self-signed there is no need to requestself-signed
secret with the same content as we have. So, nowche-tls
secret is reused forCHE_SELF_SIGNED_CERT
env var.Example command to test my changes:
I've tested the following scenario:
I tried it with the following configurations:
With self-signed certificate, I've got successfully deployed Che but even after import of self-signed certificate into browser, I tried to open Che and got this error
This is quite a known issue, but I guess some extra actions need to be done during certificate generating but I'm not sure that I should investigate it more now.
After accepting each Che host as trusted, and tried to start a workspace I got another error on Plugin Broker side. A dedicated issue is created to test self-signed certificates on Kubernetes infrastructure Test self-signed certificates for Che on Kubernetes/OS deployed with chectl (helm/operator installers) #14035
What issues does this PR fix or reference?
These issues were found during #13869
It solves #13986, #13996, #13997
Release Notes
N/A
Docs PR
N/A