From 7b01a8aa0d8626478d3a220404f96ebbecb024bc Mon Sep 17 00:00:00 2001
From: Vincent Desjardins <vdesjardins@gmail.com>
Date: Thu, 11 Oct 2018 02:17:03 +0000
Subject: [PATCH] update code review #2

Signed-off-by: Vincent Desjardins <vdesjardins@gmail.com>
---
 pkg/apis/certmanager/validation/issuer.go      | 12 ++++++++++++
 pkg/apis/certmanager/validation/issuer_test.go | 12 +++++++++++-
 pkg/issuer/vault/setup.go                      | 14 --------------
 3 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/pkg/apis/certmanager/validation/issuer.go b/pkg/apis/certmanager/validation/issuer.go
index 001ff362a..36accad53 100644
--- a/pkg/apis/certmanager/validation/issuer.go
+++ b/pkg/apis/certmanager/validation/issuer.go
@@ -17,6 +17,7 @@ limitations under the License.
 package validation
 
 import (
+	"crypto/x509"
 	"fmt"
 	"strings"
 
@@ -122,6 +123,17 @@ func ValidateVaultIssuerConfig(iss *v1alpha1.VaultIssuer, fldPath *field.Path) f
 	if len(iss.Path) == 0 {
 		el = append(el, field.Required(fldPath.Child("path"), ""))
 	}
+
+	// check if caBundle is valid
+	certs := iss.CABundle
+	if len(certs) > 0 {
+		caCertPool := x509.NewCertPool()
+		ok := caCertPool.AppendCertsFromPEM(certs)
+		if !ok {
+			el = append(el, field.Invalid(fldPath.Child("caBundle"), "", "Specified CA bundle is invalid"))
+		}
+	}
+
 	return el
 	// TODO: add validation for Vault authentication types
 }
diff --git a/pkg/apis/certmanager/validation/issuer_test.go b/pkg/apis/certmanager/validation/issuer_test.go
index 13c4a38f1..c4d00249b 100644
--- a/pkg/apis/certmanager/validation/issuer_test.go
+++ b/pkg/apis/certmanager/validation/issuer_test.go
@@ -72,6 +72,16 @@ func TestValidateVaultIssuerConfig(t *testing.T) {
 				field.Required(fldPath.Child("path"), ""),
 			},
 		},
+		"vault issuer with invalid fields": {
+			spec: &v1alpha1.VaultIssuer{
+				Server:   "something",
+				Path:     "a/b/c",
+				CABundle: []byte("invalid"),
+			},
+			errs: []*field.Error{
+				field.Invalid(fldPath.Child("caBundle"), "", "Specified CA bundle is invalid"),
+			},
+		},
 	}
 	for n, s := range scenarios {
 		t.Run(n, func(t *testing.T) {
@@ -619,7 +629,7 @@ func TestValidateACMEIssuerDNS01Config(t *testing.T) {
 }
 
 func TestValidateSecretKeySelector(t *testing.T) {
-	validName := v1alpha1.LocalObjectReference{"name"}
+	validName := v1alpha1.LocalObjectReference{Name: "name"}
 	validKey := "key"
 	// invalidName := v1alpha1.LocalObjectReference{"-name-"}
 	// invalidKey := "-key-"
diff --git a/pkg/issuer/vault/setup.go b/pkg/issuer/vault/setup.go
index 5204eebdf..3248f82b9 100644
--- a/pkg/issuer/vault/setup.go
+++ b/pkg/issuer/vault/setup.go
@@ -18,7 +18,6 @@ package vault
 
 import (
 	"context"
-	"crypto/x509"
 	"fmt"
 
 	"github.com/golang/glog"
@@ -38,7 +37,6 @@ const (
 	messageServerAndPathRequired         = "Vault server and path are required fields"
 	messsageAuthFieldsRequired           = "Vault tokenSecretRef or appRole is required"
 	messageAuthFieldRequired             = "Vault tokenSecretRef and appRole cannot be set on the same issuer"
-	messageVaultCABundleInvalid          = "Specified CA bundle is invalid"
 )
 
 func (v *Vault) Setup(ctx context.Context) error {
@@ -83,18 +81,6 @@ func (v *Vault) Setup(ctx context.Context) error {
 		return fmt.Errorf(messageAuthFieldRequired)
 	}
 
-	// check if caBundle is valid
-	certs := v.issuer.GetSpec().Vault.CABundle
-	if len(certs) > 0 {
-		caCertPool := x509.NewCertPool()
-		ok := caCertPool.AppendCertsFromPEM(certs)
-		if !ok {
-			glog.V(4).Infof("%s: %s", v.issuer.GetObjectMeta().Name, messageVaultCABundleInvalid)
-			v.issuer.UpdateStatusCondition(v1alpha1.IssuerConditionReady, v1alpha1.ConditionFalse, errorVault, messageVaultCABundleInvalid)
-			return fmt.Errorf(messageVaultCABundleInvalid)
-		}
-	}
-
 	client, err := v.initVaultClient()
 	if err != nil {
 		s := messageVaultClientInitFailed + err.Error()