Merge pull request #56 from mengqiy/webhook

address the remaining comments from #22

Author: k8s-ci-robot

pkg/admission/cert/generator/certgenerator.go

@@ -19,8 +19,11 @@ package generator
 // Artifacts hosts a private key, its corresponding serving certificate and
 // the CA certificate that signs the serving certificate.
 type Artifacts struct {
-	Key    []byte
-	Cert   []byte
+	// PEM encoded private key
+	Key []byte
+	// PEM encoded serving certificate
+	Cert []byte
+	// PEM encoded CA certificate
 	CACert []byte
 }
 

pkg/admission/cert/writer/doc.go

@@ -70,7 +70,8 @@ Create a SecretCertWriter
 		// handler error
 	}
 
-Provision the certificates using the CertWriter.
+Provision the certificates using the CertWriter. The certificate will be available in the desired secrets or
+the desired path.
 
 	// writer can be either one of the CertWriters created above
 	err = writer.EnsureCerts(webhookConfiguration) // webhookConfiguration is an existing runtime.Object

pkg/admission/cert/writer/multiple_test.go

@@ -41,7 +41,7 @@ func (f *fakeCertWriter) EnsureCerts(webhookConfig runtime.Object) error {
 	return nil
 }
 
-var _ = Describe("MultipleCertWriterProvider", func() {
+var _ = Describe("MultipleCertWriter", func() {
 	It("should invoke Provide method of each Provider", func() {
 		webhookConfig := &admissionregistration.MutatingWebhookConfiguration{}
 		f := &fakeCertWriter{}
@@ -53,7 +53,7 @@ var _ = Describe("MultipleCertWriterProvider", func() {
 		Expect(f.hasInvokedEnsureCerts).To(BeTrue())
 	})
 
-	It("should return the error from each Provider", func() {
+	It("should return the error from each CertWriter", func() {
 		webhookConfig := &admissionregistration.MutatingWebhookConfiguration{}
 		e := errors.New("this is an error")
 		f := &fakeCertWriter{err: e}
@@ -65,26 +65,3 @@ var _ = Describe("MultipleCertWriterProvider", func() {
 		Expect(f.hasInvokedEnsureCerts).To(BeTrue())
 	})
 })
-
-//var _ = Describe("multipleCertWriter", func() {
-//	It("should invoke EnsureCert method of each Provider", func() {
-//		f := &fakeCertWriter{}
-//		m := &multiCertWriter{
-//			certWriters: []CertWriter{f},
-//		}
-//		err := m.EnsureCert()
-//		Expect(err).NotTo(HaveOccurred())
-//		Expect(f.hasInvokedEnsureCert).To(BeTrue())
-//	})
-//
-//	It("should return error from each Provider", func() {
-//		e := errors.New("this is an error")
-//		f := &fakeCertWriter{err: e}
-//		m := &multiCertWriter{
-//			certWriters: []CertWriter{f},
-//		}
-//		err := m.EnsureCert()
-//		Expect(f.hasInvokedEnsureCert).To(BeTrue())
-//		Expect(err).To(MatchError(e))
-//	})
-//})

pkg/admission/cert/writer/secret.go

@@ -28,9 +28,10 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	apitypes "k8s.io/apimachinery/pkg/types"
-
+	"k8s.io/client-go/kubernetes/scheme"
 	"sigs.k8s.io/controller-runtime/pkg/admission/cert/generator"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 const (
@@ -132,22 +133,25 @@ type webhookAndSecret struct {
 
 var _ certReadWriter = &secretReadWriter{}
 
-func (s *secretReadWriter) write(webhookName string) (
-	*generator.Artifacts, error) {
+func (s *secretReadWriter) buildSecret(webhookName string) (*corev1.Secret, *generator.Artifacts, error) {
 	v := s.webhookMap[webhookName]
 
 	webhook := v.webhook
 	commonName, err := dnsNameForWebhook(&webhook.ClientConfig)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	certs, err := s.certGenerator.Generate(commonName)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	secret := certsToSecret(certs, v.secret)
-	// TODO: fix it: see the TODO in the method
-	err = setOwnerRef(secret, s.webhookConfig)
+	err = controllerutil.SetControllerReference(s.webhookConfig.(metav1.Object), secret, scheme.Scheme)
+	return secret, certs, err
+}
+
+func (s *secretReadWriter) write(webhookName string) (*generator.Artifacts, error) {
+	secret, certs, err := s.buildSecret(webhookName)
 	if err != nil {
 		return nil, err
 	}
@@ -157,20 +161,7 @@ func (s *secretReadWriter) write(webhookName string) (
 
 func (s *secretReadWriter) overwrite(webhookName string) (
 	*generator.Artifacts, error) {
-	v := s.webhookMap[webhookName]
-
-	webhook := v.webhook
-	commonName, err := dnsNameForWebhook(&webhook.ClientConfig)
-	if err != nil {
-		return nil, err
-	}
-	certs, err := s.certGenerator.Generate(commonName)
-	if err != nil {
-		return nil, err
-	}
-	secret := certsToSecret(certs, v.secret)
-	// TODO: fix it: see the TODO in the method
-	err = setOwnerRef(secret, s.webhookConfig)
+	secret, certs, err := s.buildSecret(webhookName)
 	if err != nil {
 		return nil, err
 	}
@@ -185,37 +176,6 @@ func (s *secretReadWriter) read(webhookName string) (*generator.Artifacts, error
 	return secretToCerts(secret), err
 }
 
-// setOwnerRef marks the webhook as the owner of the secret by setting the ownerReference in the secret.
-func setOwnerRef(secret, webhookConfig runtime.Object) error {
-	accessor, err := meta.Accessor(webhookConfig)
-	if err != nil {
-		return err
-	}
-	// TODO: typeAccessor.GetAPIVersion() and typeAccessor.GetKind() returns empty apiVersion and Kind, fix it.
-	typeAccessor, err := meta.TypeAccessor(webhookConfig)
-	if err != nil {
-		return err
-	}
-	blockOwnerDeletion := false
-	// 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{
-		APIVersion:         typeAccessor.GetAPIVersion(),
-		Kind:               typeAccessor.GetKind(),
-		Name:               accessor.GetName(),
-		UID:                accessor.GetUID(),
-		BlockOwnerDeletion: &blockOwnerDeletion,
-		Controller:         &isController,
-	}
-	secretAccessor, err := meta.Accessor(secret)
-	if err != nil {
-		return err
-	}
-	secretAccessor.SetOwnerReferences([]metav1.OwnerReference{ownerRef})
-	return nil
-}
-
 func secretToCerts(secret *corev1.Secret) *generator.Artifacts {
 	if secret.Data == nil {
 		return nil
@@ -229,10 +189,6 @@ func secretToCerts(secret *corev1.Secret) *generator.Artifacts {
 
 func certsToSecret(certs *generator.Artifacts, sec apitypes.NamespacedName) *corev1.Secret {
 	return &corev1.Secret{
-		TypeMeta: metav1.TypeMeta{
-			APIVersion: "v1",
-			Kind:       "Secret",
-		},
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: sec.Namespace,
 			Name:      sec.Name,

pkg/admission/cert/writer/secret_test.go

@@ -48,7 +48,7 @@ var _ = Describe("SecretCertProvider", func() {
 		url = "https://example.com/admission"
 		mwc = &admissionregistration.MutatingWebhookConfiguration{
 			TypeMeta: metav1.TypeMeta{
-				APIVersion: "admissionregistration/v1beta1",
+				APIVersion: "admissionregistration.k8s.io/v1beta1",
 				Kind:       "MutatingWebhookConfiguration",
 			},
 			ObjectMeta: metav1.ObjectMeta{
@@ -72,7 +72,7 @@ var _ = Describe("SecretCertProvider", func() {
 		}
 		vwc = &admissionregistration.ValidatingWebhookConfiguration{
 			TypeMeta: metav1.TypeMeta{
-				APIVersion: "admissionregistration/v1beta1",
+				APIVersion: "admissionregistration.k8s.io/v1beta1",
 				Kind:       "MutatingWebhookConfiguration",
 			},
 			ObjectMeta: metav1.ObjectMeta{
@@ -166,21 +166,17 @@ var _ = Describe("SecretCertProvider", func() {
 				certWriter = secretCertWriter
 
 				isController := true
-				blockOwnerDeletion := false
+				blockOwnerDeletion := true
 				secret = &corev1.Secret{
-					TypeMeta: metav1.TypeMeta{
-						APIVersion: "v1",
-						Kind:       "Secret",
-					},
 					ObjectMeta: metav1.ObjectMeta{
 						Namespace: "namespace-bar",
 						Name:      "secret-foo",
 						OwnerReferences: []metav1.OwnerReference{
 							{
-								APIVersion:         "admissionregistration/v1beta1",
+								APIVersion:         "admissionregistration.k8s.io/v1beta1",
 								Kind:               "MutatingWebhookConfiguration",
 								Name:               "test-mwc",
-								UID:                types.UID("123456"),
+								UID:                "123456",
 								BlockOwnerDeletion: &blockOwnerDeletion,
 								Controller:         &isController,
 							},

pkg/admission/controller.go

@@ -17,13 +17,11 @@ limitations under the License.
 package admission
 
 import (
+	"fmt"
 	"reflect"
 	"sync"
 
 	"k8s.io/apimachinery/pkg/runtime"
-
-	"fmt"
-
 	"sigs.k8s.io/controller-runtime/pkg/admission/cert/generator"
 	"sigs.k8s.io/controller-runtime/pkg/admission/cert/writer"
 	"sigs.k8s.io/controller-runtime/pkg/client"