address comments

Author: Mengqi Yu

pkg/admission/certgenerator/certgenerator.go renamed to pkg/admission/cert/generator/certgenerator.go

@@ -14,18 +14,18 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package certgenerator
+package generator
 
-// CertArtifacts hosts a private key, its corresponding serving certificate and
+// Artifacts hosts a private key, its corresponding serving certificate and
 // the CA certificate that signs the serving certificate.
-type CertArtifacts struct {
+type Artifacts struct {
 	Key    []byte
 	Cert   []byte
 	CACert []byte
 }
 
 // CertGenerator is an interface to provision the serving certificate.
 type CertGenerator interface {
-	// Generate returns a CertArtifacts struct.
-	Generate(CommonName string) (*CertArtifacts, error)
+	// Generate returns a Artifacts struct.
+	Generate(CommonName string) (*Artifacts, error)
 }

pkg/admission/certgenerator/certgenerator_test.go renamed to pkg/admission/cert/generator/certgenerator_test.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package certgenerator
+package generator
 
 import "fmt"
 

pkg/admission/certgenerator/doc.go renamed to pkg/admission/cert/generator/doc.go

@@ -15,16 +15,16 @@ limitations under the License.
 */
 
 /*
-Package certgenerator provides an interface and implementation to provision certificates.
+Package generator provides an interface and implementation to provision certificates.
 
 Create an instance of CertGenerator.
 
 	cg := SelfSignedCertGenerator{}
 
 Generate the certificates.
-	certs, err := cp.Generate("foo.bar.com")
+	certs, err := cg.Generate("foo.bar.com")
 	if err != nil {
 		// handle error
 	}
 */
-package certgenerator
+package generator

pkg/admission/certgenerator/fake/certgenerator.go renamed to pkg/admission/cert/generator/fake/certgenerator.go

@@ -19,20 +19,21 @@ package fake
 import (
 	"fmt"
 
-	"sigs.k8s.io/controller-runtime/pkg/admission/certgenerator"
+	"sigs.k8s.io/controller-runtime/pkg/admission/cert/generator"
 )
 
-// FakeCertGenerator is a CertGenerator for testing.
-type FakeCertGenerator struct {
-	DnsNameToCertArtifacts map[string]*certgenerator.CertArtifacts
+// CertGenerator is a CertGenerator for testing.
+type CertGenerator struct {
+	DNSNameToCertArtifacts map[string]*generator.Artifacts
 }
 
-var _ certgenerator.CertGenerator = &FakeCertGenerator{}
+var _ generator.CertGenerator = &CertGenerator{}
 
-func (cp *FakeCertGenerator) Generate(commonName string) (*certgenerator.CertArtifacts, error) {
-	certs, found := cp.DnsNameToCertArtifacts[commonName]
+// Generate generates certificates by matching a common name.
+func (cp *CertGenerator) Generate(commonName string) (*generator.Artifacts, error) {
+	certs, found := cp.DNSNameToCertArtifacts[commonName]
 	if !found {
-		return nil, fmt.Errorf("failed to find common name %q in the FakeCertGenerator", commonName)
+		return nil, fmt.Errorf("failed to find common name %q in the CertGenerator", commonName)
 	}
 	return certs, nil
 }

pkg/admission/certgenerator/selfsigned.go renamed to pkg/admission/cert/generator/selfsigned.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package certgenerator
+package generator
 
 import (
 	"crypto/x509"
@@ -39,7 +39,7 @@ var _ CertGenerator = &SelfSignedCertGenerator{}
 // to establish trust for clients, CA certificate is used by the
 // client to verify the server authentication chain.
 // The cert will be valid for 365 days.
-func (cp *SelfSignedCertGenerator) Generate(commonName string) (*CertArtifacts, error) {
+func (cp *SelfSignedCertGenerator) Generate(commonName string) (*Artifacts, error) {
 	signingKey, err := cert.NewPrivateKey()
 	if err != nil {
 		return nil, fmt.Errorf("failed to create the CA private key: %v", err)
@@ -62,7 +62,7 @@ func (cp *SelfSignedCertGenerator) Generate(commonName string) (*CertArtifacts,
 	if err != nil {
 		return nil, fmt.Errorf("failed to create the cert: %v", err)
 	}
-	return &CertArtifacts{
+	return &Artifacts{
 		Key:    cert.EncodePrivateKeyPEM(key),
 		Cert:   cert.EncodeCertPEM(signedCert),
 		CACert: cert.EncodeCertPEM(signingCert),

pkg/admission/certgenerator/selfsigned_test.go renamed to pkg/admission/cert/generator/selfsigned_test.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package certgenerator
+package generator
 
 import (
 	"crypto/x509"

pkg/admission/certwriter/certwriter.go renamed to pkg/admission/cert/writer/certwriter.go

@@ -14,20 +14,21 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package certwriter
+package writer
 
 import (
 	"bytes"
+	"crypto/tls"
+	"errors"
 	"fmt"
+	"log"
 	"net/url"
 
 	admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
-
-	"crypto/tls"
-
-	"sigs.k8s.io/controller-runtime/pkg/admission/certgenerator"
+	"sigs.k8s.io/controller-runtime/pkg/admission/cert/generator"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 const (
@@ -41,31 +42,60 @@ const (
 
 // CertWriter provides method to handle webhooks.
 type CertWriter interface {
-	// EnsureCert ensures that the webhooks it manages have the right certificates.
-	EnsureCert() error
+	// EnsureCert ensures that the webhooks have proper certificates.
+	EnsureCerts(runtime.Object) error
+}
+
+// Options are options for configuring a CertWriter.
+type Options struct {
+	Client        client.Client
+	CertGenerator generator.CertGenerator
+}
+
+// NewCertWriter builds a new CertWriter using the provided options.
+// By default, it builds a MultiCertWriter that is composed of a SecretCertWriter and a FSCertWriter.
+func NewCertWriter(ops Options) (CertWriter, error) {
+	if ops.CertGenerator == nil {
+		ops.CertGenerator = &generator.SelfSignedCertGenerator{}
+	}
+	if ops.Client == nil {
+		// TODO: default the client if possible
+		return nil, errors.New("Options.Client is required")
+	}
+	s := &SecretCertWriter{
+		Client:        ops.Client,
+		CertGenerator: ops.CertGenerator,
+	}
+	//f := &FSCertWriter{
+	//	CertGenerator: ops.CertGenerator,
+	//}
+	return &MultiCertWriter{
+		CertWriters: []CertWriter{
+			s,
+			//f,
+		},
+	}, nil
 }
 
+// handleCommon ensures the given webhook has a proper certificate.
+// It uses the given certReadWriter to read and (or) write the certificate.
 func handleCommon(webhook *admissionregistrationv1beta1.Webhook, ch certReadWriter) error {
-	webhookName := webhook.Name
-	certs, err := ch.read(webhookName)
-	if apierrors.IsNotFound(err) {
-		certs, err = ch.write(webhookName)
-		switch {
-		case apierrors.IsAlreadyExists(err):
-			certs, err = ch.read(webhookName)
-			if err != nil {
-				return err
-			}
-		case err != nil:
-			return err
-		}
-	} else if err != nil {
+	if webhook == nil {
+		return nil
+	}
+	if ch == nil {
+		return errors.New("certReaderWriter should not be nil")
+	}
+
+	certs, err := createIfNotExists(webhook.Name, ch)
+	if err != nil {
 		return err
 	}
 
 	// Recreate the cert if it's invalid.
 	if !validCert(certs) {
-		certs, err = ch.overwrite(webhookName)
+		log.Printf("cert is invalid or expiring, regenerating a new one")
+		certs, err = ch.overwrite(webhook.Name)
 		if err != nil {
 			return err
 		}
@@ -80,17 +110,39 @@ func handleCommon(webhook *admissionregistrationv1beta1.Webhook, ch certReadWrit
 	return nil
 }
 
-// certReadWriter provides methods for handling certificates for webhook.
+func createIfNotExists(webhookName string, ch certReadWriter) (*generator.Artifacts, error) {
+	// Try to read first
+	certs, err := ch.read(webhookName)
+	if apierrors.IsNotFound(err) {
+		// Create if not exists
+		certs, err = ch.write(webhookName)
+		switch {
+		// This may happen if there is another racer.
+		case apierrors.IsAlreadyExists(err):
+			certs, err = ch.read(webhookName)
+			if err != nil {
+				return certs, err
+			}
+		case err != nil:
+			return certs, err
+		}
+	} else if err != nil {
+		return certs, err
+	}
+	return certs, nil
+}
+
+// certReadWriter provides methods for reading and writing certificates.
 type certReadWriter interface {
 	// read reads a wehbook name and returns the certs for it.
-	read(webhookName string) (*certgenerator.CertArtifacts, error)
+	read(webhookName string) (*generator.Artifacts, error)
 	// write writes the certs and return the certs it wrote.
-	write(webhookName string) (*certgenerator.CertArtifacts, error)
+	write(webhookName string) (*generator.Artifacts, error)
 	// overwrite overwrites the existing certs and return the certs it wrote.
-	overwrite(webhookName string) (*certgenerator.CertArtifacts, error)
+	overwrite(webhookName string) (*generator.Artifacts, error)
 }
 
-func validCert(certs *certgenerator.CertArtifacts) bool {
+func validCert(certs *generator.Artifacts) bool {
 	// TODO:
 	// 1) validate the key and the cert are valid pair e.g. call crypto/tls.X509KeyPair()
 	// 2) validate the cert with the CA cert
@@ -118,19 +170,18 @@ func getWebhooksFromObject(obj runtime.Object) ([]admissionregistrationv1beta1.W
 	}
 }
 
-func webhookClientConfigToCommonName(config *admissionregistrationv1beta1.WebhookClientConfig) (string, error) {
+func dnsNameForWebhook(config *admissionregistrationv1beta1.WebhookClientConfig) (string, error) {
 	if config.Service != nil && config.URL != nil {
 		return "", fmt.Errorf("service and URL can't be set at the same time in a webhook: %v", config)
 	}
 	if config.Service == nil && config.URL == nil {
 		return "", fmt.Errorf("one of service and URL need to be set in a webhook: %v", config)
 	}
 	if config.Service != nil {
-		return certgenerator.ServiceToCommonName(config.Service.Namespace, config.Service.Name), nil
+		return generator.ServiceToCommonName(config.Service.Namespace, config.Service.Name), nil
 	}
-	if config.URL != nil {
-		u, err := url.Parse(*config.URL)
-		return u.Host, err
-	}
-	return "", nil
+	// config.URL != nil
+	u, err := url.Parse(*config.URL)
+	return u.Host, err
+
 }