Skip to content
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

Propagate custom public certificate to env variable #171

Merged
merged 12 commits into from
Mar 4, 2020
Merged

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented Feb 14, 2020

Add a property to configure propagation of custom public certificate from config-map to environment variable

eclipse-che/che#16002

@che-bot
Copy link
Contributor

che-bot commented Feb 14, 2020

Can one of the admins verify this patch?

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
@vinokurig
Copy link
Contributor Author

Docs PR: eclipse-che/che-docs#1078

@tolusha
Copy link
Contributor

tolusha commented Feb 14, 2020

@vinokurig
You shouldn't manually update cr/crd file.
If you update che_types.go then follow the instruction:

// Important: You should regenerate some generated code after modifying this file. At the root o fthe project:
// - Run "operator-sdk generate k8s": this will perform required changes in the "pkg/apis/org/v1/zz_generatedxxx" files
// - Run "operator-sdk generate openapi": this will generate the "deploy/crds/org_v1_checluster_crd.yaml" file
// - In the updated "deploy/crds/org_v1_checluster_crd.yaml": Delete all the `required:` openAPI rules in the CRD OpenApi schema.
// - Rename the new "deploy/crds/org_v1_checluster_crd.yaml" to "deploy/crds/org_v1_che_crd.yaml" to override it.
// IMPORTANT These 2 last steps are important to ensure backward compatibility with already existing `CheCluster` CRs that were created when no schema was provided.

@tolusha
Copy link
Contributor

tolusha commented Feb 14, 2020

/cc @mmorhun

Copy link
Contributor

@tolusha tolusha left a comment

Choose a reason for hiding this comment

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

If there is no any specific reason to merge it before 7.9.0 I would like to postpone it.

@vinokurig
Copy link
Contributor Author

@tolusha

You shouldn't manually update cr/crd file.
If you update che_types.go then follow the instruction:

Fixed

@vinokurig
Copy link
Contributor Author

@tolusha

You shouldn't manually update cr/crd file.
If you update che_types.go then follow the instruction:

Done

pkg/apis/org/v1/che_types.go Show resolved Hide resolved
deploy/crds/org_v1_che_cr.yaml Outdated Show resolved Hide resolved
pkg/deploy/deployment_che.go Outdated Show resolved Hide resolved
@vinokurig vinokurig force-pushed the che-16002 branch 3 times, most recently from a888fc5 to 036955d Compare February 21, 2020 15:10
@@ -158,6 +179,9 @@ func NewCheDeployment(cr *orgv1.CheCluster, cheImageAndTag string, cmRevision st
},
},
},
VolumeMounts: []corev1.VolumeMount{
customPublicCertsVolumeMount,
Copy link
Member

Choose a reason for hiding this comment

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

It seems to be wrong formatted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed the formatting

@tolusha
Copy link
Contributor

tolusha commented Feb 25, 2020

@vinokurig
One more step is required

cd olm
./update-nightly-olm-files.sh

And include the generated files into PR

@vinokurig
Copy link
Contributor Author

@tolusha

One more step is required

cd olm
./update-nightly-olm-files.sh

And include the generated files into PR

Done

Copy link
Contributor

@davidfestal davidfestal left a comment

Choose a reason for hiding this comment

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

Mainly OK with the changes, but let one comment about automatic rolling update of the Che server deployment when the custom certs are changed.

effectiveGitSelfSignedCert := r.GetDeploymentEnvVarSource(effectiveCheDeployment, "CHE_GIT_SELF__SIGNED__CERT") != nil
if desiredMemRequest.Cmp(effectiveMemRequest) != 0 ||
desiredMemLimit.Cmp(effectiveMemLimit) != 0 ||
effectiveImagePullPolicy != desiredImagePullPolicy ||
effectiveSelfSignedCert != desiredSelfSignedCert ||
effectiveCustomPublicCerts != desiredCustomPublicCerts ||
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that a change in the content of the config map will not trigger a redeploy of the impacted components (mainly the Che server).
You might want to also support this case by doing the same way as what is done for the che config map (with the current config map resource version being added as a metadata in the Che deployment itself).

@tolusha Do we want to support automatic rolling update of the Che server when the custom public certs config map content is changed ?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes. it make sense.

Copy link
Contributor Author

@vinokurig vinokurig Feb 28, 2020

Choose a reason for hiding this comment

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

I have no idea how to check the certificates config-map changed state. In case of the che config-map there is a predefined list of values which is compared with the actual values, but the certificates config map hasn't any predefined values. @tolusha @AndrienkoAleksandr @mmorhun @davidfestal WDYT?

@vinokurig
Copy link
Contributor Author

[ci-test]

@flacatus
Copy link
Contributor

flacatus commented Mar 3, 2020

[test]

@vinokurig vinokurig merged commit f288238 into master Mar 4, 2020
@vinokurig vinokurig deleted the che-16002 branch March 4, 2020 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants