Certificate handling improvements for preview environments #9686
Conversation
This file contains hidden or 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
mads-hartmann
commented
May 2, 2022
| @@ -142,7 +142,7 @@ export async function deployToPreviewEnvironment(werft: Werft, jobConfig: JobCon | |||
|
|
|||
| try { | |||
| werft.log(vmSlices.COPY_CERT_MANAGER_RESOURCES, 'Copy over CertManager resources from core-dev') | |||
| installMetaCertificates(werft, jobConfig.repository.branch, withVM, 'default', PREVIEW_K3S_KUBECONFIG_PATH, vmSlices.COPY_CERT_MANAGER_RESOURCES) | |||
There was a problem hiding this comment.
I believe we want this to be blocking and not fire-and-forget. The certificates has previously been issues earlier in the job, so this is waiting for them to be ready and then copying them over.
mads-hartmann
commented
May 2, 2022
| @@ -24,7 +23,7 @@ export async function prepare(werft: Werft, config: JobConfig) { | |||
| configureStaticClustersAccess() | |||
| werft.done(prepareSlices.CONFIGURE_CORE_DEV) | |||
|
|
|||
| issueCertificate(werft, config) | |||
| await issueCertificate(werft, config) | |||
There was a problem hiding this comment.
This is just creating the Certificate CRD resources that CertManager uses. I don't believe we want to do so as a "fire-and-forget" promise
There was a problem hiding this comment.
agreed - assuming we only create the CR and don't actually wait for the cert to be issued.
mads-hartmann
commented
May 2, 2022
| const certName = config.withVM ? `harvester-${previewNameFromBranchName(config.repository.branch)}` : `staging-${previewNameFromBranchName(config.repository.branch)}` | ||
| const domain = config.withVM ? `${config.previewEnvironment.destname}.preview.gitpod-dev.com` : `${config.previewEnvironment.destname}.staging.gitpod-dev.com`; | ||
|
|
||
| werft.log(prepareSlices.ISSUE_CERTIFICATES, prepareSlices.ISSUE_CERTIFICATES) | ||
| issueMetaCerts(werft, certName, "certs", domain, config.withVM, prepareSlices.ISSUE_CERTIFICATES) | ||
| await issueMetaCerts(werft, certName, "certs", domain, config.withVM, prepareSlices.ISSUE_CERTIFICATES) |
There was a problem hiding this comment.
Same as the comment above, this is just creating the Certificate, no need to use "fire-and-forget"
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.
Description
We have seen sporadic problem when issuing certificate for our preview environments. It appears to be a problem with CertManager and that upgrading to a newer version might reduce the chances of getting into this problem, but from reports it doesn't seem to eliminate it completely.
This PR:
Additionally the PR improves a few things
Related Issue(s)
Fixes https://github.com/gitpod-io/ops/issues/2066
How to test
I haven't been able to validate the error-handling code for when the certificates fail due to the nature of the issues.
Release Notes
Documentation
N/A