Update the documentation about importing external certificates #1598

davidfestal · 2020-09-18T16:28:39Z

What does this PR do?

This PR Updates the documentation about importing additional certificates in a Ce installation, to reflect the current status of Che on this Topic.

What issues does this PR fix or reference?

This PR refers to issue https://issues.redhat.com/browse/RHDEVDOCS-2089

Specify the version of the product this PR applies to.

This PR applies to the last Upstream release (7.18.2) and to the 2.4 downstream release.

PR Checklist

As the author of this Pull Request I made sure that:

vale has been run successfully against the PR branch
Link checker has been run successfully against the PR branch
Documentation describes a scenario that is already covered by QE tests, otherwise an issue has been created and acknowledged by Che QE team
Changed article references are updated where they are used (or a redirect has been set up on the docs side):
- Dashboard branding.constant.ts + product.json
- Chectl constants.ts

themr0c · 2020-09-21T08:28:08Z

...stallation-guide/partials/proc_importing-tls-certificates-to-che-server-java-truststore.adoc

@@ -1,92 +1,174 @@


 [id="importing-tls-certificates-to-{prod-id-short}-server-java-trustore_{context}"]


Can you also update the id and all xref statements pointing to this id?

tolusha · 2020-09-21T08:46:17Z

...stallation-guide/partials/proc_importing-tls-certificates-to-che-server-java-truststore.adoc

 ----
 +
- Certificates are mounted in folder `/public-certs/` of the {prod-short} server container. This command returns the list of files in that folder:
+- {prod-short} mounts certificates in folder `/public-certs/` of the {prod-short} server container. This command returns the list of files in that folder:


I think that checking /public-certs folder is not enough.
when ca-certs configmap is updated then k8s automatically refreshes /public-certs.
but those certificates won't be added to java trust store of the che-server and keycloak.

themr0c · 2020-09-21T08:48:31Z

It seems this procedure is turning into a collection of 2 distinct procedures depending on the method used to install the Che instance.

We have a similar case Upgrading Che. We make there the distinction between these 2 deployment flows: using OperatorHub vs. using the CLI management tool.

For consistency purposes, using the same presentation here would be awesome, but is it relevant?

...stallation-guide/partials/proc_importing-tls-certificates-to-che-server-java-truststore.adoc

according to whether the certificates are added; - at installation time - on an already-running installation.

davidfestal · 2020-10-13T08:19:17Z

@themr0c I don't know why the vale tests fail during CI while I have no vale error on my file in the Che workspace. Maybe rules have changed ?

@l0rd Since the PR comes from my fork, the result automatically pushed on netlify is not the right one.
So to ease the review I added the resulting page for preview as a PDF printed from the Web page

davidfestal · 2020-10-13T08:23:06Z

It seems this procedure is turning into a collection of 2 distinct procedures depending on the method used to install the Che instance.

We have a similar case Upgrading Che. We make there the distinction between these 2 deployment flows: using OperatorHub vs. using the CLI management tool.

For consistency purposes, using the same presentation here would be awesome, but is it relevant?

@themr0c

In fact we thought if would be more beneficial to split the documentation into 2 sub-section according to the use-case:

Configuration at installation time
Configuration on already-running installation.

So I assume we would not also split according to the underlying installation method (operator or helm).

l0rd

The document is much clearer now. Good job @davidfestal. But there are still some points to review.

modules/installation-guide/partials/proc_importing-external-or-additional-tls-certificates.adoc

l0rd · 2020-10-15T07:35:59Z

modules/installation-guide/partials/proc_importing-external-or-additional-tls-certificates.adoc

+* {prod-short} already uses some reserved file names to automatically inject certificates into the ConfigMap, so you should avoid using the following reserved file names to save your certificates:
+  ** `ca-bundle.crt`
+  ** `ca.crt`
+====


The paragraph above is repeated multiple times in this article. Can we move it to a different doc component and reference it multiple times?

Fixed in commit 3f481ae

modules/installation-guide/partials/proc_importing-external-or-additional-tls-certificates.adoc

themr0c · 2020-10-27T12:23:49Z

@davidfestal Any chance you can include the proposed changes and rebase soon?

davidfestal · 2020-10-27T14:41:38Z

@davidfestal Any chance you can include the proposed changes and rebase soon?

I'll try to work on it tomorrow

Co-authored-by: Mario Loriedo <mario.loriedo@gmail.com>

davidfestal · 2020-11-30T18:59:11Z

@l0rd Could you please review again after the last commit: 96614f0 ?

l0rd

LGTM Great job @davidfestal

yhontyk

LGTM

@l0rd

* First changes * Split the procedure into 2 sections according to whether the certificates are added: - at installation time - on an already-running installation. * Add changes proposed by @l0rd * Change the title according to @l0rd review comment * Fix comment #1598 (review) * Add verification steps at the workspace level * Fix some of the vale errors Co-authored-by: Mario Loriedo <mario.loriedo@gmail.com> Co-authored-by: Yana Hontyk <yhontyk@redhat.com>

@l0rd

* First changes * Split the procedure into 2 sections according to whether the certificates are added: - at installation time - on an already-running installation. * Add changes proposed by @l0rd * Change the title according to @l0rd review comment * Fix comment #1598 (review) * Add verification steps at the workspace level * Fix some of the vale errors Co-authored-by: Mario Loriedo <mario.loriedo@gmail.com> Co-authored-by: Yana Hontyk <yhontyk@redhat.com>

che-bot added the status/code-review label Sep 18, 2020

davidfestal requested review from l0rd and tolusha September 18, 2020 18:13

davidfestal changed the title ~~[WIP] Update the documentation about importing external certificates~~ Update the documentation about importing external certificates Sep 18, 2020

davidfestal marked this pull request as ready for review September 18, 2020 20:14

davidfestal requested review from yhontyk, MichalMaler, rkratky and themr0c as code owners September 18, 2020 20:14

rkratky added the need-cherry-pick/7.34.x need cherry-pick to 7.18.x branch label Sep 21, 2020

themr0c reviewed Sep 21, 2020

View reviewed changes

che-bot added the kind/enhancement label Sep 21, 2020

tolusha reviewed Sep 21, 2020

View reviewed changes

rkratky self-assigned this Sep 21, 2020

l0rd reviewed Sep 28, 2020

View reviewed changes

...stallation-guide/partials/proc_importing-tls-certificates-to-che-server-java-truststore.adoc Outdated Show resolved Hide resolved

l0rd reviewed Sep 28, 2020

View reviewed changes

...stallation-guide/partials/proc_importing-tls-certificates-to-che-server-java-truststore.adoc Outdated Show resolved Hide resolved

l0rd reviewed Sep 28, 2020

View reviewed changes

...stallation-guide/partials/proc_importing-tls-certificates-to-che-server-java-truststore.adoc Outdated Show resolved Hide resolved

l0rd reviewed Sep 28, 2020

View reviewed changes

...stallation-guide/partials/proc_importing-tls-certificates-to-che-server-java-truststore.adoc Outdated Show resolved Hide resolved

davidfestal added 5 commits October 12, 2020 17:02

first changes

9899344

Deeper update

3c35728

Cleanup

ffaf568

Add changes proposed by @l0rd

2031a3d

Split the procedure into 2 sections:

49b4702

according to whether the certificates are added; - at installation time - on an already-running installation.

davidfestal force-pushed the rhdevdocs-2089-reintegrate-external-ca-doc branch from aa64568 to 49b4702 Compare October 12, 2020 17:21

l0rd requested changes Oct 15, 2020

View reviewed changes

l0rd reviewed Oct 15, 2020

View reviewed changes

modules/installation-guide/partials/proc_importing-external-or-additional-tls-certificates.adoc Outdated Show resolved Hide resolved

l0rd reviewed Oct 15, 2020

View reviewed changes

modules/installation-guide/partials/proc_importing-external-or-additional-tls-certificates.adoc Outdated Show resolved Hide resolved

themr0c added the need-cherry-pick/7.32.x need cherry-pick to 7.32.x label Oct 20, 2020

themr0c added this to the 7.20.x milestone Oct 27, 2020

davidfestal and others added 7 commits October 28, 2020 14:45

Suggested change by @l0rd

d661e13

Co-authored-by: Mario Loriedo <mario.loriedo@gmail.com>

@l0rd Suggested change by @l0rd

6c4b132

Co-authored-by: Mario Loriedo <mario.loriedo@gmail.com>

Change the title according to @l0rd review comment

ea6c439

Additional changes requested by @l0rd

3f481ae

Small change asked by @l0rd

6d75512

2 additional changes requested by @l0rd

3a2f052

Fix comment eclipse-che#1598 (review)

89c462e

davidfestal mentioned this pull request Oct 29, 2020

Incomplete propagation of the trusted bundle CA certificates to the workspaces eclipse-che/che#18245

Closed

22 tasks

yhontyk assigned yhontyk and unassigned rkratky Nov 10, 2020

Add verification steps at the workspace level

96614f0

davidfestal requested a review from l0rd November 30, 2020 18:58

Fix some of the vale errors

28e8790

l0rd approved these changes Dec 1, 2020

View reviewed changes

Merge branch 'master' into rhdevdocs-2089-reintegrate-external-ca-doc

7076a32

yhontyk approved these changes Dec 1, 2020

View reviewed changes

davidfestal merged commit c1921c5 into eclipse-che:master Dec 2, 2020

che-bot removed the status/code-review label Dec 2, 2020

yhontyk added cherry-picked to 7.30.x and removed need-cherry-pick/7.34.x need cherry-pick to 7.18.x branch labels Dec 2, 2020

yhontyk added cherry-picked to 7.32.x cherry-picked to 7.32.x and removed need-cherry-pick/7.32.x need cherry-pick to 7.32.x labels Dec 2, 2020

themr0c removed the cherry-picked to 7.30.x label Jun 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the documentation about importing external certificates #1598

Update the documentation about importing external certificates #1598

davidfestal commented Sep 18, 2020 •

edited

Loading

themr0c Sep 21, 2020

davidfestal Oct 13, 2020

tolusha Sep 21, 2020

themr0c commented Sep 21, 2020

davidfestal commented Oct 13, 2020

davidfestal commented Oct 13, 2020 •

edited

Loading

l0rd left a comment

l0rd Oct 15, 2020

davidfestal Oct 28, 2020

themr0c commented Oct 27, 2020

davidfestal commented Oct 27, 2020

davidfestal commented Nov 30, 2020

l0rd left a comment

yhontyk left a comment

		@@ -1,92 +1,174 @@


		[id="importing-tls-certificates-to-{prod-id-short}-server-java-trustore_{context}"]

Update the documentation about importing external certificates #1598

Update the documentation about importing external certificates #1598

Conversation

davidfestal commented Sep 18, 2020 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Specify the version of the product this PR applies to.

PR Checklist

themr0c Sep 21, 2020

Choose a reason for hiding this comment

davidfestal Oct 13, 2020

Choose a reason for hiding this comment

tolusha Sep 21, 2020

Choose a reason for hiding this comment

themr0c commented Sep 21, 2020

davidfestal commented Oct 13, 2020

davidfestal commented Oct 13, 2020 • edited Loading

l0rd left a comment

Choose a reason for hiding this comment

l0rd Oct 15, 2020

Choose a reason for hiding this comment

davidfestal Oct 28, 2020

Choose a reason for hiding this comment

themr0c commented Oct 27, 2020

davidfestal commented Oct 27, 2020

davidfestal commented Nov 30, 2020

l0rd left a comment

Choose a reason for hiding this comment

yhontyk left a comment

Choose a reason for hiding this comment

davidfestal commented Sep 18, 2020 •

edited

Loading

davidfestal commented Oct 13, 2020 •

edited

Loading