pkg/certs: Move certgen into a public package so other projects can consume #3135

stevesloka · 2020-11-17T17:26:19Z

Some projects like the Contour Operator, need ways to create certificates for
Contour deployments. Currently, this is done via a Kubernetes Job which executes
the Contour command certgen which generates self-signed certificates used for
communication between Contour<>Envoy.

This moves the generation logic into a public package (e.g. pkg/certs) which
allows projects to consume this same code to generate certificates by other
means other than the Contour certgen command.

Fixes #3130

Signed-off-by: Steve Sloka slokas@vmware.com

stevesloka · 2020-11-17T17:28:14Z

@danehans @Miciah

// ref projectcontour/contour-operator#34

codecov · 2020-11-17T17:34:11Z

Codecov Report

Merging #3135 (cc2c5f2) into main (ca29391) will decrease coverage by 0.08%.
The diff coverage is 69.56%.

@@            Coverage Diff             @@
##             main    #3135      +/-   ##
==========================================
- Coverage   75.39%   75.30%   -0.09%     
==========================================
  Files          97       96       -1     
  Lines        6023     5925      -98     
==========================================
- Hits         4541     4462      -79     
+ Misses       1372     1363       -9     
+ Partials      110      100      -10

Impacted Files	Coverage Δ
cmd/contour/certgen.go	`26.00% <0.00%> (-21.30%)`	⬇️
internal/certgen/certgen.go	`72.83% <100.00%> (ø)`
internal/dag/httpproxy_processor.go	`94.15% <0.00%> (-0.02%)`	⬇️
internal/k8s/log.go	`69.56% <0.00%> (+6.52%)`	⬆️

pkg/certs/certgen.go

skriss · 2020-12-07T17:39:11Z

pkg/certs/certgen.go

+}
+
+// GenerateCerts performs the actual cert generation steps and then returns the certs for the output function.
+func GenerateCerts(certConfig *CertgenConfig) (map[string][]byte, error) {


any reason not to combine GenerateCerts and OutputCerts into a single exported function, since they need to be called one after the other? It'd make for a slightly cleaner public API IMHO.

Yeah I'll do that, less API exposed is better. =)

In looking into this a bit more, it seems like we should have 3 public methods outlined below. The confusing part about how it's built today is that if you request a cert in yaml or pem format, you still need to pass a kubeclient which isn't used at all in those two situations.

Kubernetes Secret (--kube), GenerateKubeCertificates(format string, kubeclient *kubernetes.Clientset, overwrite bool)

Yaml (--yaml), GenerateYamlCertificates(format, outputDirectory string, force bool)

PEM (--pem), GeneratePemCertificates(format, outputDirectory string, force bool)

Note: We could combine the yaml & pem into a single one, but I'd probably keep separate? /shrug

This means then that the doCertgen() inside cmd/contour will look at the flags passed and call each public method appropriately, but external callers have a clean API to use to code against.

I propose just one API:

type CertificateSet struct { Contour []byte Envoy []byte CA []byte } func GenerateCertificates(options ...) (CertificateSet, error)

The caller can deal with emitting the certificates in the way they need (i.e. contour-operator probably can't directly use any of the specific APIs above). We probably need the options so the caller can do things like set unique SAN entries.

The operator can use the apis above as-is.

The work to serialize them to pem or yaml or secrets is a benefit of using a library as this is intended so the caller doesn't need to reinvent that work.

There could be further customizations to add name, etc, etc, but could come as well as needed.

The operator can use the apis above as-is.

Are you sure? The operator doesn't need GenerateYamlCertificates or GenerateYamlCertificates. It may need GenerateKubeCertificates, but (1) depending on the Kubernetes framework it uses, it may not have easy access to the right client struct (2) it may need to specify the name or namespace of the secret (3) it may need to set owner references or other object meta information (4) it may combine the certificates into secrets in way this function does not anticipate.

Requirements / needs will change over time, 💯 .

I like the sound of @jpeach's suggestion, it seems like a nice, clean API. I think it's probably better to have separate Output functions for each type though, so that we can ensure that things consuming this package output their certs in the right way (ie files have the correct permissions etc, certificates are in the right formats, and so on). Callers don't have to use the Output functions, but supplying them makes keeping things in sync easier.

github-actions · 2021-01-01T01:09:29Z

Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 90 days.

stevesloka · 2021-01-05T17:08:40Z

Ok folks, I just did a big refactor to this, please take a look.

pkg/certs/certgen.go

…onsume Some projects like the Contour Operator, need ways to create certificates for Contour deployments. Currently, this is done via a Kubernetes Job which executes the Contour command `certgen` which generates self-signed certificates used for communication between Contour<>Envoy. This moves the generation logic into a public package (e.g. pkg/certs) which allows projects to consume this same code to generate certificates by other means other than the Contour `certgen` command. Fixes projectcontour#3130 Signed-off-by: Steve Sloka <slokas@vmware.com>

Signed-off-by: Steve Sloka <slokas@vmware.com>

pkg/certs/certgen.go

sunjayBhatia

LGTM, looks like a solid move over of the existing code and kept to the pared down interface, with some configurability

skriss

It'd be great to get @danehans to take a look at this too and validate that the public API will work for the operator's needs.

pkg/certs/certgen.go

Signed-off-by: Steve Sloka <slokas@vmware.com>

skriss

LGTM assuming @danehans confirms this will meet the Operator's needs.

youngnick

LGTM, nice work @stevesloka

…onsume (projectcontour#3135) Some projects like the Contour Operator, need ways to create certificates for Contour deployments. Currently, this is done via a Kubernetes Job which executes the Contour command `certgen` which generates self-signed certificates used for communication between Contour<>Envoy. This moves the generation logic into a public package (e.g. pkg/certs) which allows projects to consume this same code to generate certificates by other means other than the Contour `certgen` command. Fixes projectcontour#3130

stevesloka added this to the 1.11.0 milestone Nov 17, 2020

stevesloka force-pushed the moveCertgen branch from 543a1aa to 4ddc9fa Compare November 17, 2020 17:29

stevesloka requested review from youngnick and skriss November 30, 2020 21:48

skriss reviewed Dec 7, 2020

View reviewed changes

skriss modified the milestones: 1.11.0, 1.12.0 Dec 17, 2020

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 1, 2021

stevesloka force-pushed the moveCertgen branch from 8a07d0f to faa4f89 Compare January 5, 2021 17:07

stevesloka removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 5, 2021

stevesloka requested review from jpeach and skriss January 5, 2021 17:08

stevesloka force-pushed the moveCertgen branch from faa4f89 to a97f914 Compare January 5, 2021 17:10

stevesloka commented Jan 5, 2021

View reviewed changes

pkg/certs/certgen.go Outdated Show resolved Hide resolved

stevesloka added 2 commits January 5, 2021 13:35

add ability to customize the dns suffix name from cluster.local

a440c9c

Signed-off-by: Steve Sloka <slokas@vmware.com>

stevesloka force-pushed the moveCertgen branch from e5d9e31 to a440c9c Compare January 5, 2021 18:36

sunjayBhatia reviewed Jan 5, 2021

View reviewed changes

pkg/certs/certgen.go Show resolved Hide resolved

sunjayBhatia approved these changes Jan 5, 2021

View reviewed changes

skriss requested changes Jan 5, 2021

View reviewed changes

pkg/certs/certgen.go Outdated Show resolved Hide resolved

pkg/certs/certgen.go Outdated Show resolved Hide resolved

pkg/certs/certgen.go Outdated Show resolved Hide resolved

pkg/certs/certgen.go Outdated Show resolved Hide resolved

skriss requested a review from danehans January 5, 2021 19:46

jpeach reviewed Jan 5, 2021

View reviewed changes

pkg/certs/certgen.go Outdated Show resolved Hide resolved

review comments

85999de

Signed-off-by: Steve Sloka <slokas@vmware.com>

stevesloka requested a review from a team as a code owner January 5, 2021 21:08

more review comments

cd0fa40

Signed-off-by: Steve Sloka <slokas@vmware.com>

skriss approved these changes Jan 7, 2021

View reviewed changes

Merge branch 'main' into moveCertgen

cc2c5f2

youngnick approved these changes Jan 11, 2021

View reviewed changes

sunjayBhatia added 2 commits January 13, 2021 11:49

Merge branch 'main' into moveCertgen

4388610

Merge branch 'main' into moveCertgen

1799fd5

sunjayBhatia merged commit 1a7ae70 into projectcontour:main Jan 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/certs: Move certgen into a public package so other projects can consume #3135

pkg/certs: Move certgen into a public package so other projects can consume #3135

stevesloka commented Nov 17, 2020

stevesloka commented Nov 17, 2020

codecov bot commented Nov 17, 2020 •

edited

Loading

skriss Dec 7, 2020

stevesloka Dec 8, 2020

stevesloka Dec 8, 2020

jpeach Dec 8, 2020

stevesloka Dec 8, 2020

stevesloka Dec 8, 2020

jpeach Dec 8, 2020

stevesloka Dec 8, 2020

youngnick Dec 14, 2020

github-actions bot commented Jan 1, 2021

stevesloka commented Jan 5, 2021

sunjayBhatia left a comment

skriss left a comment

skriss left a comment

youngnick left a comment

pkg/certs: Move certgen into a public package so other projects can consume #3135

pkg/certs: Move certgen into a public package so other projects can consume #3135

Conversation

stevesloka commented Nov 17, 2020

stevesloka commented Nov 17, 2020

codecov bot commented Nov 17, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 1, 2021

stevesloka commented Jan 5, 2021

sunjayBhatia left a comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

youngnick left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 17, 2020 •

edited

Loading