PR feedback

- simpler returns in getExistingCA()
- add logging to decodeCerts()

helper/cert/source_gen.go

@@ -110,7 +110,7 @@ func (s *GenSource) Certificate(ctx context.Context, last *Bundle) (Bundle, erro
 		// Check if there's an existing caBundle on the webhook config
 		oldCAs := s.getExistingCA(ctx)
 		if len(oldCAs) > 0 {
-			bothCerts, err := prependLastCA(s.caCert, oldCAs)
+			bothCerts, err := prependLastCA(s.caCert, oldCAs, s.Log)
 			if err != nil {
 				// If there's an error, don't set s.caCert; just log a warning
 				// and continue on, so that the caCert will be replaced
@@ -236,8 +236,6 @@ func (s *GenSource) getBundleFromSecret() (Bundle, error) {
 }
 
 func (s *GenSource) getExistingCA(ctx context.Context) []byte {
-	var caBundle []byte
-
 	switch s.AdminAPIVersion {
 	case adminv1.SchemeGroupVersion.Version:
 		cfg, err := s.K8sClient.AdmissionregistrationV1().
@@ -248,9 +246,8 @@ func (s *GenSource) getExistingCA(ctx context.Context) []byte {
 			return []byte{}
 		}
 		if len(cfg.Webhooks) > 0 {
-			caBundle = cfg.Webhooks[0].ClientConfig.CABundle
+			return cfg.Webhooks[0].ClientConfig.CABundle
 		}
-		return caBundle
 	case adminv1beta.SchemeGroupVersion.Version:
 		cfg, err := s.K8sClient.AdmissionregistrationV1beta1().
 			MutatingWebhookConfigurations().
@@ -260,13 +257,13 @@ func (s *GenSource) getExistingCA(ctx context.Context) []byte {
 			return []byte{}
 		}
 		if len(cfg.Webhooks) > 0 {
-			caBundle = cfg.Webhooks[0].ClientConfig.CABundle
+			return cfg.Webhooks[0].ClientConfig.CABundle
 		}
-		return caBundle
-	default:
-		// unknown Admin API version, so don't even try to fetch caBundle
-		return []byte{}
 	}
+
+	// At this point either the AdminAPIVersion was unknown, or the CABundle was
+	// empty, so just return an empty slice
+	return []byte{}
 }
 
 func (s *GenSource) expiry() time.Duration {
@@ -447,7 +444,7 @@ func parseCert(pemValue []byte) (*x509.Certificate, error) {
 }
 
 // decodeCerts decodes a caBundle ([]byte) into a list of certs ([][]byte)
-func decodeCerts(caBundle []byte) tls.Certificate {
+func decodeCerts(caBundle []byte, log hclog.Logger) tls.Certificate {
 	certs := tls.Certificate{}
 	remainder := caBundle
 	var next *pem.Block
@@ -458,6 +455,8 @@ func decodeCerts(caBundle []byte) tls.Certificate {
 		}
 		if next.Type == "CERTIFICATE" {
 			certs.Certificate = append(certs.Certificate, next.Bytes)
+		} else {
+			log.Warn("unexpected pem block type in caBundle, ignoring", "type", next.Type)
 		}
 	}
 	return certs
@@ -466,9 +465,9 @@ func decodeCerts(caBundle []byte) tls.Certificate {
 // prependLastCA returns a new CA bundle:
 // [0] last CA cert from oldCABundle
 // [1] newCACert
-func prependLastCA(newCACert, oldCABundle []byte) ([]byte, error) {
+func prependLastCA(newCACert, oldCABundle []byte, log hclog.Logger) ([]byte, error) {
 	// Decode the certs from the old CA bundle
-	oldCerts := decodeCerts(oldCABundle)
+	oldCerts := decodeCerts(oldCABundle, log)
 	// Append the old CA if it exists
 	newBundle := []byte{}
 	if len(oldCerts.Certificate) > 0 {

helper/cert/source_gen_test.go

@@ -281,12 +281,12 @@ func TestGensource_prependLastCA(t *testing.T) {
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			result, err := prependLastCA(newBundle1.CACert, tc.oldCAs)
+			result, err := prependLastCA(newBundle1.CACert, tc.oldCAs, hclog.Default())
 			require.NoError(t, err)
 			assert.Equal(t, tc.expected, result)
 
 			// Run again with the output from previous
-			result2, err := prependLastCA(newBundle2.CACert, result)
+			result2, err := prependLastCA(newBundle2.CACert, result, hclog.Default())
 			require.NoError(t, err)
 			assert.Equal(t, append(newBundle1.CACert, newBundle2.CACert...), result2)
 		})