VC: update spec.timeout to be a string

Previously, spec.timeout was a *metav1.Duration. This caused some
challenges when creating/updating VaultConnection instances via VSO's
API, when the value was 60s or greater the marshalled value would no
longer be in valid duration notation and the API request would fail
during field validation.

Author: benashz

api/v1beta1/vaultconnection_types.go

@@ -24,7 +24,7 @@ type VaultConnectionSpec struct {
 	// default timeout from the Vault API client config is used.
 	// +kubebuilder:validation:Type=string
 	// +kubebuilder:validation:Pattern="^([0-9]+(\\.[0-9]+)?(s|m|h))$"
-	Timeout *metav1.Duration `json:"timeout,omitempty"`
+	Timeout string `json:"timeout,omitempty"`
 }
 
 // VaultConnectionStatus defines the observed state of VaultConnection

api/v1beta1/zz_generated.deepcopy.go

@@ -883,7 +883,7 @@ _Appears in:_
 | `tlsServerName` _string_ | TLSServerName to use as the SNI host for TLS connections. |  |  |
 | `caCertSecretRef` _string_ | CACertSecretRef is the name of a Kubernetes secret containing the trusted PEM encoded CA certificate chain as `ca.crt`. |  |  |
 | `skipTLSVerify` _boolean_ | SkipTLSVerify for TLS connections. | false |  |
-| `timeout` _[Duration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.24/#duration-v1-meta)_ | Timeout applied to all Vault requests for this connection. If not set, the<br />default timeout from the Vault API client config is used. |  | Pattern: `^([0-9]+(\.[0-9]+)?(s|m|h))$` <br />Type: string <br /> |
+| `timeout` _string_ | Timeout applied to all Vault requests for this connection. If not set, the<br />default timeout from the Vault API client config is used. |  | Pattern: `^([0-9]+(\.[0-9]+)?(s|m|h))$` <br />Type: string <br /> |
 
 
 

docs/api/api-reference.md

@@ -906,8 +906,12 @@ func NewClientConfigFromConnObj(connObj *secretsv1beta1.VaultConnection, vaultNS
 		VaultNamespace:  vaultNS,
 	}
 
-	if connObj.Spec.Timeout != nil {
-		cfg.Timeout = connObj.Spec.Timeout
+	if connObj.Spec.Timeout != "" {
+		d, err := time.ParseDuration(connObj.Spec.Timeout)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse timeout: %w", err)
+		}
+		cfg.Timeout = &d
 	}
 	return cfg, nil
 }

internal/vault/client.go

@@ -346,12 +346,12 @@ func Test_cachingClientFactory_storageEncryptionClient(t *testing.T) {
 			UID: connUID,
 		},
 		Spec: secretsv1beta1.VaultConnectionSpec{
-			Timeout: &metav1.Duration{Duration: 10 * time.Second},
+			Timeout: "10s",
 		},
 	}
 
 	vcObjTimeout5s := vcObj.DeepCopy()
-	vcObjTimeout5s.Spec.Timeout = &metav1.Duration{Duration: 5 * time.Second}
+	vcObjTimeout5s.Spec.Timeout = "5s"
 
 	saObj := &corev1.ServiceAccount{
 		ObjectMeta: metav1.ObjectMeta{

internal/vault/client_factory_test.go

@@ -18,6 +18,7 @@ import (
 	"golang.org/x/crypto/blake2b"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/utils/ptr"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
@@ -1168,12 +1169,12 @@ func TestNewClientConfigFromConnObj(t *testing.T) {
 			TLSServerName:   "baz.biff",
 			CACertSecretRef: "ca.crt",
 			SkipTLSVerify:   true,
-			Timeout:         &metav1.Duration{Duration: 10 * time.Second},
+			Timeout:         "10s",
 		},
 	}
 
-	connObjNilTimeout := connObjBase.DeepCopy()
-	connObjNilTimeout.Spec.Timeout = nil
+	connObjEmptyTimeout := connObjBase.DeepCopy()
+	connObjEmptyTimeout.Spec.Timeout = ""
 
 	tests := []struct {
 		name    string
@@ -1191,13 +1192,13 @@ func TestNewClientConfigFromConnObj(t *testing.T) {
 				TLSServerName:   "baz.biff",
 				CACertSecretRef: "ca.crt",
 				SkipTLSVerify:   true,
-				Timeout:         &metav1.Duration{Duration: 10 * time.Second},
+				Timeout:         ptr.To[time.Duration](10 * time.Second),
 			},
 			wantErr: assert.NoError,
 		},
 		{
-			name:    "nil-timeout",
-			connObj: connObjNilTimeout,
+			name:    "empty-timeout",
+			connObj: connObjEmptyTimeout,
 			want: &ClientConfig{
 				Address:         "https://vault.example.com",
 				Headers:         map[string]string{"foo": "bar"},

internal/vault/client_test.go

@@ -7,11 +7,11 @@ import (
 	"context"
 	"crypto/x509"
 	"fmt"
+	"time"
 
 	"github.com/hashicorp/vault/api"
 	vconsts "github.com/hashicorp/vault/sdk/helper/consts"
 	v1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
@@ -41,7 +41,7 @@ type ClientConfig struct {
 	Headers map[string]string
 	// Timeout applied to all Vault requests. If not set, the default timeout from
 	// the Vault API client config is used.
-	Timeout *metav1.Duration `json:"timeout,omitempty"`
+	Timeout *time.Duration
 }
 
 // MakeVaultClient creates a Vault api.Client from a ClientConfig.
@@ -93,7 +93,7 @@ func MakeVaultClient(ctx context.Context, cfg *ClientConfig, client ctrlclient.C
 	}
 
 	if cfg.Timeout != nil {
-		config.Timeout = cfg.Timeout.Duration
+		config.Timeout = *cfg.Timeout
 	}
 
 	config.CloneToken = true

internal/vault/config.go

@@ -17,6 +17,7 @@ import (
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	"github.com/hashicorp/vault-secrets-operator/internal/consts"
@@ -81,9 +82,7 @@ func TestMakeVaultClient(t *testing.T) {
 		"vault timeout not nil": {
 			vaultConfig: &ClientConfig{
 				VaultNamespace: "vault-test-namespace",
-				Timeout: &v1.Duration{
-					Duration: 10 * time.Second,
-				},
+				Timeout:        ptr.To[time.Duration](10 * time.Second),
 			},
 			CACert:        nil,
 			expectedError: nil,
@@ -165,7 +164,7 @@ func TestMakeVaultClient(t *testing.T) {
 
 				var expectedTimeout time.Duration
 				if tc.vaultConfig.Timeout != nil {
-					expectedTimeout = tc.vaultConfig.Timeout.Duration
+					expectedTimeout = *tc.vaultConfig.Timeout
 				} else {
 					expectedTimeout = api.DefaultConfig().Timeout
 				}