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

certgen: add cert-manager certificate compatibility #2547

Merged
merged 1 commit into from
May 26, 2020

Conversation

jpeach
Copy link
Contributor

@jpeach jpeach commented May 21, 2020

This change adds new options to certgen that will allow it to overwrite
existing Kubernetes Secrets, and to generate Secrets in a format that
is compatible with cert-manager (we call this "compact" format).

The --overwrite flag can be used to let certgen update existing secrets,
and this will enable both the transition to the new format and certificate
rotation on a per-release granularity.

The --secrets-format flag switches the Secrets output format from the
current format to the compatible "compact" format. Once this is enabled
in the deployment YAML, operators will easily be able to substitute
cert-manager certificates for certgen certificates.

Finally, the bootstrap command is updated to ensure that the specified
certificate files are actually present. This prevents Envoy starting up
and rejecting the static configuration, which is unrecoverable.

This fixes #2494.

Signed-off-by: James Peach [email protected]

@jpeach jpeach requested review from stevesloka and youngnick May 21, 2020 07:22
@jpeach
Copy link
Contributor Author

jpeach commented May 21, 2020

@tsaarni This PR adds all the machinery for xDS certificate rotation and transitioning to xDS Secrets that are compatible with cert-manager. Would be great if you could do a review pass.

@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

Merging #2547 into master will increase coverage by 0.92%.
The diff coverage is 36.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2547      +/-   ##
==========================================
+ Coverage   76.84%   77.77%   +0.92%     
==========================================
  Files          72       71       -1     
  Lines        5892     5893       +1     
==========================================
+ Hits         4528     4583      +55     
+ Misses       1264     1206      -58     
- Partials      100      104       +4     
Impacted Files Coverage Δ
cmd/contour/certgen.go 31.94% <18.51%> (+31.94%) ⬆️
internal/certgen/certgen.go 34.44% <40.78%> (+34.44%) ⬆️
internal/envoy/bootstrap.go 87.66% <50.00%> (-1.53%) ⬇️

internal/certgen/certgen.go Show resolved Hide resolved
cmd/contour/bootstrap.go Outdated Show resolved Hide resolved
internal/certgen/certgen.go Outdated Show resolved Hide resolved
internal/certgen/certgen.go Show resolved Hide resolved
internal/certgen/k8s.go Outdated Show resolved Hide resolved
@jpeach
Copy link
Contributor Author

jpeach commented May 22, 2020

@stevesloka @youngnick With the "compact" secrets format, should we still create the cacert secret with a single ca.crt data element?

@jpeach jpeach force-pushed the certgen-compact-secrets branch from 379b8b7 to d8cc485 Compare May 24, 2020 23:56
@youngnick
Copy link
Member

@stevesloka @youngnick With the "compact" secrets format, should we still create the cacert secret with a single ca.crt data element?

If that will help with migration, yes, otherwise, no.

Copy link
Member

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM aside from changing CheckFiles default to false, probably with a name change.

cmd/contour/bootstrap.go Outdated Show resolved Hide resolved
@jpeach
Copy link
Contributor Author

jpeach commented May 26, 2020

@stevesloka @youngnick With the "compact" secrets format, should we still create the cacert secret with a single ca.crt data element?

If that will help with migration, yes, otherwise, no.

Pretty sure it's not needed for migration.

@jpeach jpeach force-pushed the certgen-compact-secrets branch from d8cc485 to fabdad1 Compare May 26, 2020 05:41
This change adds new options to certgen that will allow it to overwrite
existing Kubernetes Secrets, and to generate Secrets in a format that
is compatible with cert-manager (we call this "compact" format).

The `--overwrite` flag can be used to let certgen update existing secrets,
and this will enable both the transition to the new format and certificate
rotation on a per-release granularity.

The `--secrets-format` flag switches the Secrets output format from the
current format to the compatible "compact" format. Once this is enabled
in the deployment YAML, operators will easily be able to substitute
cert-manager certificates for certgen certificates.

Finally, the bootstrap command is updated to ensure that the specified
certificate files are actually present. This prevents Envoy starting up
and rejecting the static configuration, which is unrecoverable.

This fixes projectcontour#2494.

Signed-off-by: James Peach <[email protected]>
@jpeach jpeach force-pushed the certgen-compact-secrets branch from fabdad1 to 9034a0f Compare May 26, 2020 05:47
@youngnick youngnick self-requested a review May 26, 2020 06:52
Copy link
Member

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

LGTM

@jpeach jpeach merged commit 96de431 into projectcontour:master May 26, 2020
@jpeach jpeach deleted the certgen-compact-secrets branch May 26, 2020 07:25
@jpeach jpeach added this to the 1.5.0 milestone May 28, 2020
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.

Certgen should generate secrets that are compatible with cert-manager
4 participants