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

[local-preview] Remove cert-manager dependency #11412

Merged
merged 1 commit into from
Jul 21, 2022
Merged

Conversation

Pothulapati
Copy link
Contributor

Description

This PR removes the dependency of cert-manager and thus
reducing resource usage. This is replaced by the usage of
mkcert instead.

Each certificate is induvidually generated on those domains
and stored into specific files for k3s to apply them which
is then used by Gitpod.

Signed-off-by: Tarun Pothulapati tarun@gitpod.io

Related Issue(s)

Part of #11303

How to test

Release Notes

[local-preview] Remove `cert-manager` dependency

Documentation

Werft options:

  • /werft with-preview

@Pothulapati Pothulapati force-pushed the tar/lp-cert-manager branch 2 times, most recently from 298a5c5 to 863b3d8 Compare July 15, 2022 11:41
@Pothulapati Pothulapati marked this pull request as ready for review July 15, 2022 11:41
@Pothulapati Pothulapati requested a review from a team July 15, 2022 11:41
@github-actions github-actions bot added the team: delivery Issue belongs to the self-hosted team label Jul 15, 2022
@Pothulapati Pothulapati added do-not-merge/work-in-progress and removed team: delivery Issue belongs to the self-hosted team labels Jul 15, 2022
This PR removes the dependency of `cert-manager` and thus
reducing resource usage. This is replaced by the usage of
`mkcert` instead

Signed-off-by: Tarun Pothulapati <tarun@gitpod.io>
@github-actions github-actions bot added the team: delivery Issue belongs to the self-hosted team label Jul 18, 2022
cat "${HOME}"/.local/share/mkcert/rootCA.pem > "$FN_CACERT"
mkcert -cert-file "$FN_SSLCERT" \
-key-file "$FN_SSLKEY" \
"*.ws.${DOMAIN}" "*.${DOMAIN}" "${DOMAIN}" "reg.${DOMAIN}" "registry.default.svc.cluster.local" "gitpod.default" "ws-manager.default.svc" "ws-manager" "ws-manager-dev" "registry-facade" "server" "ws-manager-bridge" "ws-proxy" "ws-manager" "ws-daemon.default.svc" "ws-daemon" "wsdaemon"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this is enough, and we don't have to generate induvidual certs for each purpose.

@mrsimonemms
Copy link
Contributor

I'm using the command docker run -p 443:443 --privileged --name gitpod --rm -it -v gitpod:/var/gitpod eu.gcr.io/gitpod-core-dev/build/local-preview:tar-lp-cert-manager.11 to test - I believe this is the correct command

@Pothulapati
Copy link
Contributor Author

Correct @mrsimonemms

@mrsimonemms
Copy link
Contributor

It's more a bit of branding than a problem, but is there any way we can make the certificate name be "Gitpod Local Preview" (or something similar)? (This in Firefox, but I guess it'll be the same everywhere)

image

@mrsimonemms
Copy link
Contributor

/hold

mrsimonemms
mrsimonemms previously approved these changes Jul 18, 2022
Copy link
Contributor

@mrsimonemms mrsimonemms left a comment

Choose a reason for hiding this comment

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

Works beautifully. Only question is about the certificate name - if it's not possible or want to do later, feel free to remove the hold

@mrsimonemms mrsimonemms self-requested a review July 18, 2022 13:38
@Pothulapati
Copy link
Contributor Author

Pothulapati commented Jul 18, 2022

It's a valid question, and looks like there is no direct mkcert way of setting the CA name as per FiloSottile/mkcert#260.

I just remembered I used https://github.com/smallstep/cli for similar tasks, and can try moving to that unless there is some concern using it. 🤔

cc: @lucasvaltl

@mrsimonemms
Copy link
Contributor

Let's do as a future PR

I've removed my approval as cannot build image at moment. Investigating reasons
image

@Pothulapati
Copy link
Contributor Author

Pothulapati commented Jul 18, 2022

So, I saw a similar image build error initially on this same change, but couldn't repro it. It could be a real issue. As this is just an improvement, I'd err on making sure if its a real problem or not before merging.

@adrienthebo Can you also PTAL once? 👀 😄

@adrienthebo
Copy link
Contributor

I got a successful build when creating an environment against https://github.com/mrzarquon/gitpod-aws-toolkit (as that's where I'm doing most of my work right now). This should be good to go, but I'm running a test against gitpod-io/gitpod to see if I can reproduce the failure that you're seeing @mrsimonemms.

Copy link
Contributor

@adrienthebo adrienthebo left a comment

Choose a reason for hiding this comment

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

LGTM!

@mrsimonemms
Copy link
Contributor

mrsimonemms commented Jul 21, 2022

I'm getting the same issue on both this PR and latest, so it's almost certainly not a problem introduced by this PR (I've also upgraded from Ubuntu 21.10 to 22.04, so it's probably my computer that's broken it)

Remove the hold at-will

@Pothulapati
Copy link
Contributor Author

Removing the hold, but will continue testing and raise a separate issue if its something that we might have to fix.

/unhold

@roboquat roboquat merged commit af9da1c into main Jul 21, 2022
@roboquat roboquat deleted the tar/lp-cert-manager branch July 21, 2022 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note size/L team: delivery Issue belongs to the self-hosted team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants