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

[helm] Fix reverse proxy for builtin registry #4711

Merged
merged 1 commit into from
Jul 14, 2021

Conversation

corneliusludmann
Copy link
Contributor

With the new proxy implementation with Caddy, the access to the built-in registry is over Caddy which serves the registry with the main proxy certificates instead of the registry certificates (SSL termination). That has the following implications:

  • Caddy accesses the registry via https://registry.default.svc.cluster.local. That means the self-signed cert of the registry needs this domain instead of https://registry.my-gitpod.example.com.
  • Caddy needs to trust the self-singed certs of the registry (needs to know the CA).
  • The dind of the image builder does not need the registry certs since the certs that Caddy uses are (need to be) trusted certs and not self-signed certs (as long as bypassProxy is not set).

I tested this on k3s with this setting and verified that the preview env still works.

Alternatives: The alternative would be to change the behavior of Caddy so that the proxy does not terminate SSL but does a raw redirect to the registry as it was in nginx (when this is possible with Caddy).

See also #4710

/cc @solomonope due to self-hosted
/cc @aledbf since you implemented the Caddy config

/werft with-installer
/werft with-retag
@corneliusludmann corneliusludmann requested a review from a team as a code owner July 6, 2021 09:35
@csweichel csweichel requested a review from aledbf July 6, 2021 09:35
@csweichel
Copy link
Contributor

Also assigning @aledbf here due to upcoming changes on self-hosted internal cert management

@@ -2,7 +2,7 @@
# Licensed under the MIT License. See License-MIT.txt in the project root for license information.

{{ if (index .Values "docker-registry" "enabled") }}
{{- $regName := include "gitpod.builtinRegistry.name" . -}}
{{- $regName := include "gitpod.builtinRegistry.internal_name" . -}}
{{ $cm := (index .Values "cert-manager") }}
{{- if $cm.enabled }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wulfthimm: The (self-signed) cert of the registry has always the internal domain registry.default.svc.cluster.local as domain, never the public registry.my-gitpod.example.com domain. Don't know how the cert-manager works but could this lead to problems with the cert-manager configuration here? Do we need to drop this?

@corneliusludmann corneliusludmann removed the request for review from a team July 6, 2021 09:39
@aledbf
Copy link
Member

aledbf commented Jul 6, 2021

@corneliusludmann I have a draft PR that improves the self-signed certificate generation and do not uses proxy to access the docker-registry. Please check #4592

@corneliusludmann
Copy link
Contributor Author

I have a draft PR that improves the self-signed certificate generation and do not uses proxy to access the docker-registry. Please check #4592

Thanks for the hint, @aledbf! Big PR, couldn't go through everything. 😇 Does it mean that your PR solves the issues I posted in our slack channel self-hosted a few days ago [here and here (internal)]? Is there an ETA for your PR?

@aledbf
Copy link
Member

aledbf commented Jul 6, 2021

Does it mean that your PR solves the issues I posted in our slack channel self-hosted a few days ago

Yes. I need to finish the PR before next Monday.

@aledbf
Copy link
Member

aledbf commented Jul 6, 2021

@corneliusludmann I updated the PR and split the commits to be more clear about what is being changed

@corneliusludmann corneliusludmann marked this pull request as draft July 7, 2021 07:25
@geropl geropl mentioned this pull request Jul 9, 2021
8 tasks
@geropl
Copy link
Member

geropl commented Jul 14, 2021

/werft run

👍 started the job as gitpod-build-clu-fix-registry-reverse-proxy.1

@corneliusludmann corneliusludmann marked this pull request as ready for review July 14, 2021 09:01
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

👍

Quick reasoning summary:

pull

  • each registry-facade installs a re-direct in it's local /etc/hosts: reg.${domain}:port -> registry-facade node port
  • this works because we issue publicly verifiable certs (Let's Encrypt) which include reg.

push:

  • prod/staging/devstaging: imagebuilder get's the properly configured GCP creds and the GCP registry has publicly verifiable certs
  • builtin: our chart has an exception that makes the registry available at registry.domain
  • the builtin case is broken because we never test it (neither staging nor devstaging)
  • the fix works as advertised

@geropl
Copy link
Member

geropl commented Jul 14, 2021

/werft run with-clean-slate-deployment

👍 started the job as gitpod-build-clu-fix-registry-reverse-proxy.2

@corneliusludmann corneliusludmann merged commit f55f4fc into main Jul 14, 2021
@corneliusludmann corneliusludmann deleted the clu/fix-registry-reverse-proxy branch July 14, 2021 14:05
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.

None yet

4 participants