From 298ceb3b2a72706097f03271ec0998d496610ca5 Mon Sep 17 00:00:00 2001 From: Vincent Sabatini Date: Thu, 19 Oct 2023 07:53:28 -0500 Subject: [PATCH] fix error message when setting up vault issuer * Ensure Vault URL can be parsed * Separate generic http errors from vault specific errors when checking health endpoint Signed-off-by: Vincent Sabatini --- internal/vault/vault.go | 9 +++++ internal/vault/vault_test.go | 9 ++++- .../certificaterequests/vault/vault_test.go | 6 +++- .../vault/vault_test.go | 1 + pkg/issuer/vault/setup.go | 12 +++++-- pkg/issuer/vault/setup_test.go | 36 +++++++++++++++++++ 6 files changed, 68 insertions(+), 5 deletions(-) diff --git a/internal/vault/vault.go b/internal/vault/vault.go index 4a5aaf2c8..6c51ec8c0 100644 --- a/internal/vault/vault.go +++ b/internal/vault/vault.go @@ -22,6 +22,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "path" "path/filepath" "strings" @@ -225,6 +226,14 @@ func (v *Vault) newConfig() (*vault.Config, error) { cfg := vault.DefaultConfig() cfg.Address = v.issuer.GetSpec().Vault.Server + urlParse, err := url.Parse(cfg.Address) + if err != nil { + return nil, fmt.Errorf("error parsing vault url: %w", err) + } + if urlParse.Host == "" { + return nil, fmt.Errorf("host not found in vault server url: %s", cfg.Address) + } + caBundle, err := v.caBundle() if err != nil { return nil, fmt.Errorf("failed to load vault CA bundle: %w", err) diff --git a/internal/vault/vault_test.go b/internal/vault/vault_test.go index 6e2aa31f6..5e457a203 100644 --- a/internal/vault/vault_test.go +++ b/internal/vault/vault_test.go @@ -989,6 +989,7 @@ func TestNewConfig(t *testing.T) { "a bad cert bundle should error": { issuer: gen.Issuer("vault-issuer", gen.SetIssuerVault(cmapi.VaultIssuer{ + Server: "https://vault.example.com", CABundle: []byte("a bad cert bundle"), }), ), @@ -998,6 +999,7 @@ func TestNewConfig(t *testing.T) { "a good cert bundle should be added to the config": { issuer: gen.Issuer("vault-issuer", gen.SetIssuerVault(cmapi.VaultIssuer{ + Server: "https://vault.example.com", CABundle: []byte(testLeafCertificate), }), ), @@ -1025,6 +1027,7 @@ func TestNewConfig(t *testing.T) { "a good bundle from a caBundleSecretRef should be added to the config": { issuer: gen.Issuer("vault-issuer", gen.SetIssuerVault(cmapi.VaultIssuer{ + Server: "https://vault.example.com", CABundleSecretRef: &cmmeta.SecretKeySelector{ Key: "my-bundle.crt", LocalObjectReference: cmmeta.LocalObjectReference{ @@ -1060,6 +1063,7 @@ func TestNewConfig(t *testing.T) { "a good bundle from a caBundleSecretRef with default key should be added to the config": { issuer: gen.Issuer("vault-issuer", gen.SetIssuerVault(cmapi.VaultIssuer{ + Server: "https://vault.example.com", CABundleSecretRef: &cmmeta.SecretKeySelector{ LocalObjectReference: cmmeta.LocalObjectReference{ Name: "bundle", @@ -1094,6 +1098,7 @@ func TestNewConfig(t *testing.T) { "a bad bundle from a caBundleSecretRef should error": { issuer: gen.Issuer("vault-issuer", gen.SetIssuerVault(cmapi.VaultIssuer{ + Server: "https://vault.example.com", CABundleSecretRef: &cmmeta.SecretKeySelector{ Key: "my-bundle.crt", LocalObjectReference: cmmeta.LocalObjectReference{ @@ -1108,7 +1113,8 @@ func TestNewConfig(t *testing.T) { "the tokenCreate func should be called with the correct namespace": { issuer: gen.Issuer("vault-issuer", gen.SetIssuerVault(cmapi.VaultIssuer{ - Path: "my-path", + Server: "https://vault.example.com", + Path: "my-path", Auth: cmapi.VaultAuth{ Kubernetes: &cmapi.VaultKubernetesAuth{ Role: "my-role", @@ -1320,6 +1326,7 @@ func TestNewWithVaultNamespaces(t *testing.T) { Spec: v1.IssuerSpec{ IssuerConfig: v1.IssuerConfig{ Vault: &v1.VaultIssuer{ + Server: "https://vault.example.com", Namespace: tc.vaultNS, Auth: cmapi.VaultAuth{ TokenSecretRef: &cmmeta.SecretKeySelector{ diff --git a/pkg/controller/certificaterequests/vault/vault_test.go b/pkg/controller/certificaterequests/vault/vault_test.go index 35dc0207b..0cb69b5fc 100644 --- a/pkg/controller/certificaterequests/vault/vault_test.go +++ b/pkg/controller/certificaterequests/vault/vault_test.go @@ -88,7 +88,9 @@ func generateSelfSignedCertFromCR(cr *cmapi.CertificateRequest, key crypto.Signe func TestSign(t *testing.T) { metaFixedClockStart := metav1.NewTime(fixedClockStart) baseIssuer := gen.Issuer("vault-issuer", - gen.SetIssuerVault(cmapi.VaultIssuer{}), + gen.SetIssuerVault(cmapi.VaultIssuer{ + Server: "https://example.vault.com", + }), gen.AddIssuerCondition(cmapi.IssuerCondition{ Type: cmapi.IssuerConditionReady, Status: cmmeta.ConditionTrue, @@ -234,6 +236,7 @@ func TestSign(t *testing.T) { }, }, }, + Server: "https://example.vault.com", })), }, ExpectedEvents: []string{ @@ -274,6 +277,7 @@ func TestSign(t *testing.T) { }, }, }, + Server: "https://example.vault.com", }), )}, ExpectedEvents: []string{ diff --git a/pkg/controller/certificatesigningrequests/vault/vault_test.go b/pkg/controller/certificatesigningrequests/vault/vault_test.go index 0d03c6547..f4b528977 100644 --- a/pkg/controller/certificatesigningrequests/vault/vault_test.go +++ b/pkg/controller/certificatesigningrequests/vault/vault_test.go @@ -70,6 +70,7 @@ func TestProcessItem(t *testing.T) { }, }, }, + Server: "https://example.vault.com", }), gen.AddIssuerCondition(cmapi.IssuerCondition{ Type: cmapi.IssuerConditionReady, diff --git a/pkg/issuer/vault/setup.go b/pkg/issuer/vault/setup.go index 9a3c450a4..886c9ed79 100644 --- a/pkg/issuer/vault/setup.go +++ b/pkg/issuer/vault/setup.go @@ -19,6 +19,7 @@ package vault import ( "context" "fmt" + "strings" vaultinternal "github.com/cert-manager/cert-manager/internal/vault" apiutil "github.com/cert-manager/cert-manager/pkg/api/util" @@ -135,9 +136,14 @@ func (v *Vault) Setup(ctx context.Context) error { } if err := client.IsVaultInitializedAndUnsealed(); err != nil { - logf.V(logf.WarnLevel).Infof("%s: %s: error: %s", v.issuer.GetObjectMeta().Name, messageVaultStatusVerificationFailed, err.Error()) - apiutil.SetIssuerCondition(v.issuer, v.issuer.GetGeneration(), v1.IssuerConditionReady, cmmeta.ConditionFalse, errorVault, messageVaultStatusVerificationFailed) - return fmt.Errorf(messageVaultStatusVerificationFailed) + if strings.Contains(err.Error(), "error calling Vault") { + logf.V(logf.WarnLevel).Infof("%s: %s: error: %s", v.issuer.GetObjectMeta().Name, messageVaultStatusVerificationFailed, err.Error()) + apiutil.SetIssuerCondition(v.issuer, v.issuer.GetGeneration(), v1.IssuerConditionReady, cmmeta.ConditionFalse, errorVault, messageVaultStatusVerificationFailed) + return fmt.Errorf(messageVaultStatusVerificationFailed) + } + logf.V(logf.WarnLevel).Infof("%s: %s", v.issuer.GetObjectMeta().Name, err.Error) + apiutil.SetIssuerCondition(v.issuer, v.issuer.GetGeneration(), v1.IssuerConditionReady, cmmeta.ConditionFalse, errorVault, err.Error()) + return err } logf.Log.V(logf.DebugLevel).Info(messageVaultVerified) diff --git a/pkg/issuer/vault/setup_test.go b/pkg/issuer/vault/setup_test.go index 9bcb99d1d..6fa891a17 100644 --- a/pkg/issuer/vault/setup_test.go +++ b/pkg/issuer/vault/setup_test.go @@ -368,6 +368,42 @@ func TestVault_Setup(t *testing.T) { }, expectCond: "Ready True: VaultVerified: Vault verified", }, + { + name: "server with invalid url should fail to setup", + givenIssuer: v1.IssuerConfig{ + Vault: &v1.VaultIssuer{ + Path: "pki_int", + Server: "https:/vault.example.com", + Auth: v1.VaultAuth{ + TokenSecretRef: &cmmeta.SecretKeySelector{ + LocalObjectReference: cmmeta.LocalObjectReference{ + Name: "cert-manager", + }, + Key: "", + }, + }, + }, + }, + expectErr: "host not found in vault server url: https:/vault.example.com", + }, + { + name: "server with leading whitespace should fail to parse", + givenIssuer: v1.IssuerConfig{ + Vault: &v1.VaultIssuer{ + Path: "pki_int", + Server: " https:///vault.example.com", + Auth: v1.VaultAuth{ + TokenSecretRef: &cmmeta.SecretKeySelector{ + LocalObjectReference: cmmeta.LocalObjectReference{ + Name: "cert-manager", + }, + Key: "", + }, + }, + }, + }, + expectErr: "error parsing vault url: parse \" https:///vault.example.com\": first path segment in URL cannot contain colon", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {