cert provisioner #22

mengqiy · 2018-06-12T07:08:31Z

This PR is migrating from mengqiy/WebhookCertManager#1

GoDoc: http://35.192.136.38:12346/pkg/sigs.k8s.io/controller-runtime/pkg/admission/

droot

lets discuss this in person today to go over it.

droot · 2018-06-12T11:01:14Z

pkg/admission/certoutput/certoutput.go

+limitations under the License.
+*/
+
+package certoutput


mengqiy · 2018-06-12T21:35:40Z

PTAL

pwittrock · 2018-06-12T22:11:36Z

pkg/admission/doc.go

+The package will write self-signed certificates to secrets.
+
+	// Build a CertProvisioner with empty options
+	cp := &CertProvisioner{Client: getClientOrDie(), Options: Options{}}


They shouldn't have to specify Options. I don't think the getClientOrDie() function is correct. Use the one from the config package

pwittrock · 2018-06-12T22:12:04Z

pkg/admission/doc.go

+		// handler error
+	}
+
+If the above mutatingWebhookConfiguration is processed, the cert provisioner will provision


mutatingWebhookConfiguration

cap?

pwittrock · 2018-06-12T22:13:33Z

pkg/admission/doc.go

+
+If the above mutatingWebhookConfiguration is processed, the cert provisioner will provision
+the certificate and create an secret named "secret-foo" in namespace "namespace-bar" for webhook "webhook-1".
+Similarly, it will create an secret named "secret-baz" in namespace "default" for webhook "webhook-2".


Also mention that it will write the CA back to the WebhookConfiguration

pwittrock · 2018-06-12T22:14:19Z

pkg/admission/certoutput/certoutput.go

+	ServerCertName = "cert.pem"
+)
+
+// Options are options for configure a CertOutput.


for configuring

pwittrock · 2018-06-12T22:14:28Z

pkg/admission/certoutput/certoutput.go

+
+// Options are options for configure a CertOutput.
+type Options struct {
+	Type *Type


Document Type

pwittrock · 2018-06-12T22:15:09Z

pkg/admission/controller.go

+}
+
+// getClientOrDie get the default client for the CertProvisioner.
+func getClientOrDie() client.Client {


This already exists in the config package

I can't find it.
There is no method returning client.Client in https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/client/config
The only relevant thing I can find is client.New() method at https://godoc.org/github.com/kubernetes-sigs/controller-runtime/pkg/client#New

pwittrock · 2018-06-12T22:16:06Z

pkg/admission/controller.go

+
+func (bc *CertProvisioner) init() {
+	if bc.Client == nil {
+		bc.Client = getClientOrDie()


We probably want to return an error from the calling method instead of panic-ing.

pwittrock · 2018-06-12T22:17:25Z

pkg/admission/controller.go

+	"github.com/kubernetes-sigs/controller-runtime/pkg/client/config"
+)
+
+// CertProvisioner provisions certificates for webhook configurations.


and writes them to an output destination - such as a Secret or local file. CertProvisioner can update the CA field of
certain resources with the CA of the certs.

pwittrock · 2018-06-12T22:26:51Z

pkg/admission/certoutput/certoutput.go

+}
+
+// New builds a new CertsOutput instance for the provided webhook configuration.
+func New(webhookConfig runtime.Object, client client.Client, cp certinput.CertInput, op Options) (CertsOutput, error) {


can the Input be an option tool? Can we default it to selfsigned?

pwittrock · 2018-06-12T22:27:44Z

pkg/admission/certoutput/certoutput.go

+}
+
+// New builds a new CertsOutput instance for the provided webhook configuration.
+func New(webhookConfig runtime.Object, client client.Client, cp certinput.CertInput, op Options) (CertsOutput, error) {


For client, WDYT of injecting it?

What will it look like? Some pseudo code will be helpful.

mengqiy · 2018-06-13T20:24:57Z

PTAL

droot

Looks good, have a couple of comments.

droot · 2018-06-13T21:21:56Z

pkg/admission/certgenerator/doc.go


-Create a implementation instance of certprovisioner.
+Create a implementation instance of certgenerator.


Create an instance of CertGenerator ?

droot · 2018-06-13T21:22:15Z

pkg/admission/certgenerator/doc.go

-	cp := SelfSignedCertProvisioner{
-		CommonName: "foo.bar.com"
-	}
+	cp := SelfSignedCertGenerator{}


nit: cp --> cg :)

droot · 2018-06-13T21:22:35Z

pkg/admission/certgenerator/doc.go

-	cp := SelfSignedCertProvisioner{
-		CommonName: "foo.bar.com"
-	}
+	cp := SelfSignedCertGenerator{}

 Provision the certificates.


Provision --> Generate

droot · 2018-06-13T21:30:39Z

pkg/admission/certgenerator/certgenerator.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package certprovisioner
+package certgenerator


Want to throw it out there from caller's code readability perspective:

We can probably have a package name cert under admission

caller's code will look like

cg, err := cert.Generator() cw, err := cert.Writer()

droot · 2018-06-13T21:33:58Z

pkg/admission/certgenerator/certgenerator.go

@@ -24,8 +24,8 @@ type Certs struct {
 	CACert []byte
 }


Certs is giving an impression of multiple certificates and it is not.

some options.Cert or Certificate or CertArtifacts or CertContents

droot · 2018-06-13T21:37:51Z

pkg/admission/certwriter/certwriter.go

+// CertWriter provides method to handle webhooks.
+type CertWriter interface {
+	// Create a new CertWriter with the given runtime.Object
+	New(runtime.Object) (CertWriter, error)


Are we sure this method belongs in this interface ? its more like a constructor for CertWriter itself ?

droot · 2018-06-13T21:42:34Z

pkg/admission/certwriter/certwriter.go

+	// Create a new CertWriter with the given runtime.Object
+	New(runtime.Object) (CertWriter, error)
+	// Handle ensures that the webhooks it manages have the right certificates.
+	Handle() error


Should this be called WriteCert or EnsureCert or ManageCert or PopulateCert ?

droot · 2018-06-13T21:43:52Z

pkg/admission/certwriter/certwriter.go

+}
+
+// certio provides methods for handling certificates for webhook.
+type certio interface {


certReaderWriter is probably better name ?

droot · 2018-06-13T21:47:43Z

pkg/admission/controller.go

+		if err != nil {
+			return err
+		}
+	}


This doesn't appear to be thread-safe ?

Also, once cp.CertWriter is assigned and webhookConfiguration changes, cp.CertWriter won't get updated .. right..? wouldn't that be a problem ?

pwittrock · 2018-06-19T01:28:38Z

pkg/admission/certwriter/certwriter.go

+	EnsureCert() error
+}
+
+func handleCommon(webhook *admissionregistrationv1beta1.Webhook, ch certReadWriter) error {


document this function

pwittrock · 2018-06-19T19:12:08Z

pkg/admission/certwriter/suite_test.go

+var _ = BeforeSuite(func(done Done) {
+	logf.SetLogger(logf.ZapLogger(false))
+
+	testenv = &test.Environment{}


Don't we need to start the te if we are going to use it?

pwittrock · 2018-06-19T19:13:21Z

pkg/admission/certwriter/provider.go

+}
+
+// New builds a new CertWriter for the provided webhook configuration.
+func NewProvider(ops Options) (CertWriterProvider, error) {


This writes to both locations? What does this do by default?

pwittrock · 2018-06-19T19:13:55Z

pkg/admission/certwriter/multiple.go

+	"k8s.io/apimachinery/pkg/runtime"
+)
+
+// MultiCertWriterProvider composes a slice of CertWriterProviders.


Add a comment about what use cases this is good for.

mengqiy · 2018-06-21T00:39:22Z

PTAL

mengqiy · 2018-06-21T00:40:36Z

Coverage report:
sigs.k8s.io/controller-runtime/pkg/admission/cert/generator/selfsigned.go:27: ServiceToCommonName 100.0%
sigs.k8s.io/controller-runtime/pkg/admission/cert/generator/selfsigned.go:42: Generate 69.2%
sigs.k8s.io/controller-runtime/pkg/admission/cert/writer/certwriter.go:57: NewCertWriter 100.0%
sigs.k8s.io/controller-runtime/pkg/admission/cert/writer/certwriter.go:82: handleCommon 100.0%
sigs.k8s.io/controller-runtime/pkg/admission/cert/writer/certwriter.go:113: createIfNotExists 100.0%
sigs.k8s.io/controller-runtime/pkg/admission/cert/writer/certwriter.go:145: validCert 100.0%
sigs.k8s.io/controller-runtime/pkg/admission/cert/writer/certwriter.go:160: getWebhooksFromObject 100.0%
sigs.k8s.io/controller-runtime/pkg/admission/cert/writer/certwriter.go:173: dnsNameForWebhook 100.0%
sigs.k8s.io/controller-runtime/pkg/admission/cert/writer/multiple.go:32: EnsureCerts 100.0%
sigs.k8s.io/controller-runtime/pkg/admission/cert/writer/secret.go:53: EnsureCerts 95.5%
sigs.k8s.io/controller-runtime/pkg/admission/cert/writer/secret.go:98: parseAnnotations 100.0%
sigs.k8s.io/controller-runtime/pkg/admission/cert/writer/secret.go:109: ensureCert 80.0%
sigs.k8s.io/controller-runtime/pkg/admission/cert/writer/secret.go:135: write 78.6%
sigs.k8s.io/controller-runtime/pkg/admission/cert/writer/secret.go:158: overwrite 78.6%
sigs.k8s.io/controller-runtime/pkg/admission/cert/writer/secret.go:181: read 100.0%
sigs.k8s.io/controller-runtime/pkg/admission/cert/writer/secret.go:189: setOwnerRef 78.6%
sigs.k8s.io/controller-runtime/pkg/admission/cert/writer/secret.go:219: secretToCerts 100.0%
sigs.k8s.io/controller-runtime/pkg/admission/cert/writer/secret.go:230: certsToSecret 100.0%
total: (statements) 90.1%

pwittrock · 2018-06-21T01:06:50Z

pkg/admission/cert/writer/certwriter.go

+		Client:        ops.Client,
+		CertGenerator: ops.CertGenerator,
+	}
+	//f := &FSCertWriter{


Why is this commented out?

It has not been implemented ans tested completely :)

pwittrock · 2018-06-21T01:07:07Z

pkg/admission/cert/writer/certwriter.go

+}
+
+// NewCertWriter builds a new CertWriter using the provided options.
+// By default, it builds a MultiCertWriter that is composed of a SecretCertWriter and a FSCertWriter.


This document is not correct, it doesn't do the file writer

This will happen in one week.
I can remove it for now.

pwittrock · 2018-06-21T01:10:04Z

pkg/admission/cert/writer/fs.go

+
+package writer
+
+//import (


Can you remove this file from the PR?

pwittrock · 2018-06-21T01:13:28Z

pkg/admission/cert/writer/secret.go

+	// Due to
+	// https://github.com/kubernetes/kubernetes/blob/5da925ad4fd070e687dc5255c177d5e7d542edd7/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/controller_ref.go#L35
+	isController := true
+	ownerRef := metav1.OwnerReference{


Why not use this:

https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/controller/controllerutil/controllerutil.go#L31

pwittrock · 2018-06-21T01:17:40Z

pkg/admission/cert/writer/secret.go

+	*generator.Artifacts, error) {
+	v := s.webhookMap[webhookName]
+
+	webhook := v.webhook


Most of this code is duplicated in write(). Please pull it out into a function.

pwittrock · 2018-06-21T01:18:32Z

pkg/admission/cert/writer/secret.go

+
+func certsToSecret(certs *generator.Artifacts, sec apitypes.NamespacedName) *corev1.Secret {
+	return &corev1.Secret{
+		TypeMeta: metav1.TypeMeta{


FYI: you shouldn't need to fill in the TypeMeta.

Why?
Does it mean the client.Client will set the TypeMeta fields for me if I pass it a *corev1.Secret?

pwittrock · 2018-06-21T01:20:45Z

pkg/admission/controller.go

+
+import (
+	"reflect"
+	"sync"


cleanup these imports

pwittrock · 2018-06-21T01:23:01Z

pkg/admission/cert/writer/multiple_test.go

+	return nil
+}
+
+var _ = Describe("MultipleCertWriterProvider", func() {


I think this has been renamed.

pwittrock · 2018-06-21T01:24:33Z

pkg/admission/cert/writer/doc.go

+Provision the certificates using the CertWriter.
+
+	// writer can be either one of the CertWriters created above
+	err = writer.EnsureCerts(webhookConfiguration) // webhookConfiguration is an existing runtime.Object


How does this get the certs out? Add an example of actually starting an http server.

pwittrock · 2018-06-21T01:25:27Z

pkg/admission/cert/writer/secret_test.go

+			ObjectMeta: metav1.ObjectMeta{
+				Name:        "test-mwc",
+				Namespace:   "test-ns",
+				UID:         types.UID("123456"),


I think you can just use a string.

pwittrock · 2018-06-21T01:27:26Z

/lgtm

pwittrock · 2018-06-21T01:27:31Z

/approve

address the remaining comments from #22

cert provisioner

address the remaining comments from kubernetes-sigs#22

mengqiy requested a review from pwittrock June 12, 2018 07:08

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 12, 2018

mengqiy requested a review from droot June 12, 2018 07:15

droot reviewed Jun 12, 2018

View reviewed changes

pkg/admission/certoutput/certoutput.go Outdated

limitations under the License.

*/

package certoutput

Copy link

Contributor

droot Jun 12, 2018

Choose a reason for hiding this comment

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

certio ?

mengqiy force-pushed the webhook branch 2 times, most recently from cd79900 to 62cad9b Compare June 12, 2018 21:33

mengqiy force-pushed the webhook branch from 62cad9b to 52c95da Compare June 12, 2018 21:52

pwittrock reviewed Jun 12, 2018

View reviewed changes

mengqiy force-pushed the webhook branch 2 times, most recently from fa1ac99 to 952db43 Compare June 13, 2018 20:23

droot reviewed Jun 13, 2018

View reviewed changes

mengqiy mentioned this pull request Jun 14, 2018

Cert manager mengqiy/WebhookCertManager#1

Closed

mengqiy force-pushed the webhook branch 2 times, most recently from 8d0492b to 2994c60 Compare June 19, 2018 00:57

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 19, 2018

This comment has been minimized.

Sign in to view

pwittrock reviewed Jun 19, 2018

View reviewed changes

Mengqi Yu added 3 commits June 20, 2018 17:07

cert provisioner

96156ac

add some tests

43f54a4

address comments

c745b80

mengqiy force-pushed the webhook branch from 2994c60 to c745b80 Compare June 21, 2018 00:32

pwittrock reviewed Jun 21, 2018

View reviewed changes

pkg/admission/controller.go

import (

"reflect"

"sync"

Copy link

Contributor

pwittrock Jun 21, 2018

Choose a reason for hiding this comment

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

cleanup these imports

pwittrock reviewed Jun 21, 2018

View reviewed changes

k8s-ci-robot assigned pwittrock Jun 21, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2018

pwittrock added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2018

k8s-ci-robot merged commit 365c04b into kubernetes-sigs:master Jun 21, 2018

mengqiy deleted the webhook branch June 21, 2018 21:36

mengqiy mentioned this pull request Jun 21, 2018

address the remaining comments from #22 #56

Merged

k8s-ci-robot added a commit that referenced this pull request Jun 22, 2018

Merge pull request #56 from mengqiy/webhook

bf51669

address the remaining comments from #22

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018

Merge pull request kubernetes-sigs#22 from mengqiy/webhook

315ba15

cert provisioner

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018

Merge pull request kubernetes-sigs#56 from mengqiy/webhook

33347c8

address the remaining comments from kubernetes-sigs#22


		Create a implementation instance of certprovisioner.
		Create a implementation instance of certgenerator.

cert provisioner #22

cert provisioner #22

Uh oh!

Conversation

mengqiy commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

droot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mengqiy commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mengqiy commented Jun 13, 2018

Uh oh!

droot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mengqiy commented Jun 21, 2018

Uh oh!

mengqiy commented Jun 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mengqiy commented Jun 12, 2018 •

edited

Loading