Remove selfSignedCertSecretName property #15878

mmorhun · 2020-01-30T10:07:30Z

Signed-off-by: Mykola Morhun mmorhun@redhat.com

What does this PR do?

Reduces number of tls secrets in Che installation process.
Actually we may use the same certificate, but treat it differently depending on self-signed flag.

What issues does this PR fix or reference?

#15810
Should be merged together with che-incubator/chectl#476

che-bot · 2020-01-30T10:37:27Z

✅ E2E Happy path tests succeed 🎉

See Details

Jenkins job
logs and configs
Happy path tests DevFile
image: maxura/che-server:15878

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

che-bot · 2020-01-30T11:08:02Z

E2E tests of Eclipse Che Multiuser on OCP has been successful:

Build details
Test report
docker image: eclipseche/che-server:15878

sleshchenko

Good simplification! 👍

che-bot · 2020-01-31T15:24:16Z

Can one of the admins verify this patch?

mmorhun · 2020-02-04T16:08:48Z

Rebased

che-bot · 2020-02-04T16:37:28Z

✅ E2E Happy path tests succeed 🎉

See Details

Jenkins job
logs and configs
Happy path tests DevFile
image: maxura/che-server:15878

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

che-bot · 2020-02-04T18:51:04Z

E2E tests of Eclipse Che Multiuser on OCP has failed:

Build details
Test report
docker image: eclipseche/che-server:15878
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.
Use command "ci-test" to rerun the test.

sleshchenko · 2020-02-05T09:10:45Z

@metlos Please review the latest commit 4adf9a9

sleshchenko

Still LGTM after my commit ;)

che-bot · 2020-02-05T09:30:07Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:15878

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

che-bot · 2020-02-05T10:44:08Z

E2E tests of Eclipse Che Multiuser on OCP has failed:

Build details
Test report
docker image: eclipseche/che-server:15878
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.
Use command "ci-test" to rerun the test.

sleshchenko · 2020-02-05T10:51:34Z

ci-build

metlos

I think the change on the condition for propagating the TLS secret is not correct.

metlos · 2020-02-05T11:16:28Z

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

              optional: false
        {{- end }}

        # If workspaces are created in a separate precreated namespace
        # then configure Che Server to propagate TLS secret to workspaces' namespaces
-        {{- if not (contains "<" .Values.global.cheWorkspacesNamespace) }}
+        {{- if contains "<" .Values.global.cheWorkspacesNamespace }}


So originally, before we had placeholders in namespace names, this test was merely "if the namespace is different than the default one". With the introduction of placeholders, we changed the condition to "if this is a static, precreated, namespace". Reverting that condition as you suggest, would actually mean "if this is either a precreated namespace using a placeholder (e.g. <username>-che) or a namespace we create (e.g. <workspaceid>-che)".

So I don't think this change is actually valid. At the same time I agree the test is insufficient :) We need to capture the "precreatedness" of the namespaces here and I am afraid we cannot capture that by merely detecting the placeholders in the name.

Maybe just actually give up and introduce global.namespacePrecreated?

I wrote a lot of words and thought about the behavior we need here, but then just removed everything because I got to a conclusion that Che Server must be configured with TLS certs to propagate always if it's different than Che Server one...

We introduce placeholders and some conditions in helm chart and they are valid for ServiceAccounts, Roles, and RoleBindings... But what is the reason not to propagate TLS secret for WorkspaceIngresses?... At this stage, I believe that they must be propagated always if workspaces are not deployed in the same namespace as Che is because only in that case workspaces are not able to reuse Che Server's one...
Hope I described my thought quite clearly. Let me fix condition as I described and then we'll continue discussing it

mmorhun · 2020-02-05T12:29:33Z

I've just tested the latest changes and everything worked as supposed to.
At least for my deployment case.

che-bot · 2020-02-05T12:33:06Z

✅ E2E Happy path tests succeed 🎉

See Details

Jenkins job
logs and configs
Happy path tests DevFile
image: maxura/che-server:15878

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>

che-bot · 2020-02-05T13:03:56Z

✅ E2E Happy path tests succeed 🎉

See Details

Jenkins job
logs and configs
Happy path tests DevFile
image: maxura/che-server:15878

Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1)

che-bot · 2020-02-05T13:40:10Z

E2E tests of Eclipse Che Multiuser on OCP has failed:

Build details
Test report
docker image: eclipseche/che-server:15878
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.
Use command "ci-test" to rerun the test.

SkorikSergey · 2020-02-05T13:55:37Z

Selenium tests execution on Eclipse Che Multiuser on OCP (https://ci.codenvycorp.com/job/che-pullrequests-test-ocp/3558//Selenium_20tests_20report/) doesn't show any regression against this Pull request.

tolusha · 2020-02-17T07:36:46Z

@mmorhun
Can we merge?

mmorhun · 2020-02-17T07:56:48Z

@tolusha no, we cannot, because this should be merged together with che-incubator/chectl#476, which is better to merge after the upcoming release.

SkorikSergey · 2020-02-24T09:32:37Z

[ci-test]

mmorhun requested review from skabashnyuk, tolusha, mshaposhnik, sleshchenko and AndrienkoAleksandr January 30, 2020 10:07

mmorhun requested review from l0rd, nickboldt and rhopp as code owners January 30, 2020 10:07

mmorhun self-assigned this Jan 30, 2020

che-bot added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Jan 30, 2020

mmorhun mentioned this pull request Jan 30, 2020

feat: Implement TLS by default for Minikube + Helm installer che-incubator/chectl#476

Merged

sleshchenko approved these changes Jan 30, 2020

View reviewed changes

AndrienkoAleksandr approved these changes Jan 30, 2020

View reviewed changes

tolusha mentioned this pull request Feb 2, 2020

Che Deploy Sprint 179 #15839

Closed

35 tasks

mmorhun force-pushed the che-15810 branch from 5a1f251 to e783c62 Compare February 4, 2020 16:08

sleshchenko requested a review from metlos February 5, 2020 09:10

sleshchenko approved these changes Feb 5, 2020

View reviewed changes

metlos reviewed Feb 5, 2020

View reviewed changes

mmorhun and others added 2 commits February 5, 2020 14:40

Remove selfSignedCertSecretName property

8da3276

Signed-off-by: Mykola Morhun <mmorhun@redhat.com>

Fix propagating of TLS certificate for workspaces ingresses

378c28b

Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>

sleshchenko force-pushed the che-15810 branch from 73bcaf8 to 378c28b Compare February 5, 2020 12:40

metlos approved these changes Feb 5, 2020

View reviewed changes

SkorikSergey approved these changes Feb 5, 2020

View reviewed changes

tolusha mentioned this pull request Feb 17, 2020

Che Deploy Sprint #180 #16049

Closed

46 tasks

mmorhun merged commit 579cb67 into master Feb 26, 2020

mmorhun deleted the che-15810 branch February 26, 2020 09:26

che-bot added this to the 7.10.0 milestone Feb 26, 2020

che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Feb 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove selfSignedCertSecretName property #15878

Remove selfSignedCertSecretName property #15878

mmorhun commented Jan 30, 2020 •

edited

Loading

che-bot commented Jan 30, 2020

che-bot commented Jan 30, 2020

sleshchenko left a comment

che-bot commented Jan 31, 2020

mmorhun commented Feb 4, 2020

che-bot commented Feb 4, 2020

che-bot commented Feb 4, 2020

sleshchenko commented Feb 5, 2020

sleshchenko left a comment

che-bot commented Feb 5, 2020

che-bot commented Feb 5, 2020

sleshchenko commented Feb 5, 2020

metlos left a comment

metlos Feb 5, 2020

sleshchenko Feb 5, 2020

mmorhun commented Feb 5, 2020 •

edited

Loading

che-bot commented Feb 5, 2020

che-bot commented Feb 5, 2020

che-bot commented Feb 5, 2020

SkorikSergey commented Feb 5, 2020

tolusha commented Feb 17, 2020

mmorhun commented Feb 17, 2020

SkorikSergey commented Feb 24, 2020

Remove selfSignedCertSecretName property #15878

Remove selfSignedCertSecretName property #15878

Conversation

mmorhun commented Jan 30, 2020 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

che-bot commented Jan 30, 2020

che-bot commented Jan 30, 2020

sleshchenko left a comment

Choose a reason for hiding this comment

che-bot commented Jan 31, 2020

mmorhun commented Feb 4, 2020

che-bot commented Feb 4, 2020

che-bot commented Feb 4, 2020

sleshchenko commented Feb 5, 2020

sleshchenko left a comment

Choose a reason for hiding this comment

che-bot commented Feb 5, 2020

che-bot commented Feb 5, 2020

sleshchenko commented Feb 5, 2020

metlos left a comment

Choose a reason for hiding this comment

metlos Feb 5, 2020

Choose a reason for hiding this comment

sleshchenko Feb 5, 2020

Choose a reason for hiding this comment

mmorhun commented Feb 5, 2020 • edited Loading

che-bot commented Feb 5, 2020

che-bot commented Feb 5, 2020

che-bot commented Feb 5, 2020

SkorikSergey commented Feb 5, 2020

tolusha commented Feb 17, 2020

mmorhun commented Feb 17, 2020

SkorikSergey commented Feb 24, 2020

mmorhun commented Jan 30, 2020 •

edited

Loading

mmorhun commented Feb 5, 2020 •

edited

Loading