feat(core): accept duration string for SecretStore refreshInterval (#6594)

Co-authored-by: Gergely Bräutigam <gergely.brautigam@sap.com>
Signed-off-by: Alexander Chernov <alexander@chernov.it>

apis/externalsecrets/v1/secretstore_types.go

@@ -17,8 +17,12 @@ limitations under the License.
 package v1
 
 import (
+	"fmt"
+	"time"
+
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
 )
 
 // SecretStoreSpec defines the desired state of SecretStore.
@@ -35,15 +39,53 @@ type SecretStoreSpec struct {
 	// +optional
 	RetrySettings *SecretStoreRetrySettings `json:"retrySettings,omitempty"`
 
-	// Used to configure store refresh interval in seconds. Empty or 0 will default to the controller config.
+	// Used to configure store refresh interval. Accepts either an integer number
+	// of seconds (legacy) or a Go duration string such as "1h" or "5m". Empty or
+	// 0 will default to the controller config.
 	// +optional
-	RefreshInterval int `json:"refreshInterval,omitempty"`
+	// +kubebuilder:validation:XIntOrString
+	RefreshInterval *intstr.IntOrString `json:"refreshInterval,omitempty"`
 
 	// Used to constrain a ClusterSecretStore to specific namespaces. Relevant only to ClusterSecretStore.
 	// +optional
 	Conditions []ClusterSecretStoreCondition `json:"conditions,omitempty"`
 }
 
+// GetRefreshInterval resolves the refresh interval to a time.Duration. The field
+// accepts either an integer number of seconds (legacy) or a Go duration string
+// such as "1h" or "5m". A nil value (unset) returns 0, meaning the controller
+// default should be used. A negative value, or a string that is not a valid
+// duration, returns an error.
+//
+// TODO(v2): the int-or-string form is a non-breaking shim to accept duration
+// strings without changing the v1 field type. In the next major API version,
+// change RefreshInterval to a first-class *metav1.Duration and drop this
+// accessor. See issue #2977.
+func (s *SecretStoreSpec) GetRefreshInterval() (time.Duration, error) {
+	if s.RefreshInterval == nil {
+		return 0, nil
+	}
+	switch s.RefreshInterval.Type {
+	case intstr.Int:
+		secs := s.RefreshInterval.IntValue()
+		if secs < 0 {
+			return 0, fmt.Errorf("invalid refreshInterval %d: must not be negative", secs)
+		}
+		return time.Duration(secs) * time.Second, nil
+	case intstr.String:
+		d, err := time.ParseDuration(s.RefreshInterval.StrVal)
+		if err != nil {
+			return 0, fmt.Errorf("invalid refreshInterval %q: %w", s.RefreshInterval.StrVal, err)
+		}
+		if d < 0 {
+			return 0, fmt.Errorf("invalid refreshInterval %q: must not be negative", s.RefreshInterval.StrVal)
+		}
+		return d, nil
+	default:
+		return 0, fmt.Errorf("unsupported refreshInterval type %d", s.RefreshInterval.Type)
+	}
+}
+
 // ClusterSecretStoreCondition describes a condition by which to choose namespaces to process ExternalSecrets in
 // for a ClusterSecretStore instance.
 type ClusterSecretStoreCondition struct {

apis/externalsecrets/v1/secretstore_types_test.go

@@ -0,0 +1,68 @@
+/*
+Copyright © The ESO Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    https://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/util/intstr"
+)
+
+func TestSecretStoreSpecGetRefreshInterval(t *testing.T) {
+	fromInt := intstr.FromInt32(300)
+	fromZero := intstr.FromInt32(0)
+	fromDuration := intstr.FromString("1h")
+	fromSecondsString := intstr.FromString("90s")
+	fromBadString := intstr.FromString("nonsense")
+	fromNegInt := intstr.FromInt32(-5)
+	fromNegString := intstr.FromString("-5m")
+	// A quoted, unitless number ("10") is a string, not the legacy integer 10.
+	// It has no duration unit, so it is rejected (whereas the bare int 10 is 10s).
+	fromNumericString := intstr.FromString("10")
+
+	tests := []struct {
+		name    string
+		in      *intstr.IntOrString
+		want    time.Duration
+		wantErr bool
+	}{
+		{name: "nil defaults to zero", in: nil, want: 0},
+		{name: "zero is unset (uses default)", in: &fromZero, want: 0},
+		{name: "legacy integer is seconds", in: &fromInt, want: 300 * time.Second},
+		{name: "duration string", in: &fromDuration, want: time.Hour},
+		{name: "seconds as duration string", in: &fromSecondsString, want: 90 * time.Second},
+		{name: "invalid duration string errors", in: &fromBadString, wantErr: true},
+		{name: "negative integer errors", in: &fromNegInt, wantErr: true},
+		{name: "negative duration string errors", in: &fromNegString, wantErr: true},
+		{name: "unitless numeric string is rejected", in: &fromNumericString, wantErr: true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			spec := SecretStoreSpec{RefreshInterval: tt.in}
+			got, err := spec.GetRefreshInterval()
+			if tt.wantErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}

apis/externalsecrets/v1/secretstore_validator.go

@@ -75,6 +75,13 @@ func validateStore(store GenericStore) (admission.Warnings, error) {
 		return nil, err
 	}
 
+	// Reject an unparseable refreshInterval duration string at admission. The
+	// x-kubernetes-int-or-string schema accepts any string, so the duration
+	// format can only be validated here (or at reconcile).
+	if _, err := store.GetSpec().GetRefreshInterval(); err != nil {
+		return nil, err
+	}
+
 	provider, err := GetProvider(store)
 	if err != nil {
 		return nil, err

apis/externalsecrets/v1/secretstore_validator_test.go

@@ -22,6 +22,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/util/intstr"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
@@ -68,6 +69,84 @@ func TestValidateSecretStore(t *testing.T) {
 				require.Equal(t, 0, len(warns))
 			},
 		},
+		{
+			name: "valid duration string refreshInterval",
+			obj: &SecretStore{
+				Spec: SecretStoreSpec{
+					RefreshInterval: new(intstr.FromString("1h")),
+					Provider: &SecretStoreProvider{
+						AWS: &AWSProvider{},
+					},
+				},
+			},
+			mock: func() {
+				ForceRegister(&ValidationProvider{}, &SecretStoreProvider{
+					AWS: &AWSProvider{},
+				}, MaintenanceStatusMaintained)
+			},
+			assertErr: func(t *testing.T, err error) {
+				require.NoError(t, err)
+			},
+			assertWarns: func(t *testing.T, warns admission.Warnings) {
+				require.Equal(t, 0, len(warns))
+			},
+		},
+		{
+			name: "legacy integer refreshInterval",
+			obj: &SecretStore{
+				Spec: SecretStoreSpec{
+					RefreshInterval: new(intstr.FromInt32(300)),
+					Provider: &SecretStoreProvider{
+						AWS: &AWSProvider{},
+					},
+				},
+			},
+			mock: func() {
+				ForceRegister(&ValidationProvider{}, &SecretStoreProvider{
+					AWS: &AWSProvider{},
+				}, MaintenanceStatusMaintained)
+			},
+			assertErr: func(t *testing.T, err error) {
+				require.NoError(t, err)
+			},
+			assertWarns: func(t *testing.T, warns admission.Warnings) {
+				require.Equal(t, 0, len(warns))
+			},
+		},
+		{
+			name: "invalid duration string refreshInterval is rejected",
+			obj: &SecretStore{
+				Spec: SecretStoreSpec{
+					RefreshInterval: new(intstr.FromString("nonsense")),
+					Provider: &SecretStoreProvider{
+						AWS: &AWSProvider{},
+					},
+				},
+			},
+			assertErr: func(t *testing.T, err error) {
+				assert.ErrorContains(t, err, "invalid refreshInterval")
+			},
+			assertWarns: func(t *testing.T, warns admission.Warnings) {
+				require.Equal(t, 0, len(warns))
+			},
+		},
+		{
+			name: "negative refreshInterval is rejected",
+			obj: &SecretStore{
+				Spec: SecretStoreSpec{
+					RefreshInterval: new(intstr.FromInt32(-5)),
+					Provider: &SecretStoreProvider{
+						AWS: &AWSProvider{},
+					},
+				},
+			},
+			assertErr: func(t *testing.T, err error) {
+				assert.ErrorContains(t, err, "must not be negative")
+			},
+			assertWarns: func(t *testing.T, warns admission.Warnings) {
+				require.Equal(t, 0, len(warns))
+			},
+		},
 		{
 			name: "invalid regex",
 			obj: &SecretStore{

apis/externalsecrets/v1/zz_generated.deepcopy.go

@@ -24,6 +24,7 @@ import (
 	apismetav1 "github.com/external-secrets/external-secrets/apis/meta/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/intstr"
 )
 
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
@@ -4168,6 +4169,11 @@ func (in *SecretStoreSpec) DeepCopyInto(out *SecretStoreSpec) {
 		*out = new(SecretStoreRetrySettings)
 		(*in).DeepCopyInto(*out)
 	}
+	if in.RefreshInterval != nil {
+		in, out := &in.RefreshInterval, &out.RefreshInterval
+		*out = new(intstr.IntOrString)
+		**out = **in
+	}
 	if in.Conditions != nil {
 		in, out := &in.Conditions, &out.Conditions
 		*out = make([]ClusterSecretStoreCondition, len(*in))

config/crds/bases/external-secrets.io_clustersecretstores.yaml

@@ -7385,9 +7385,14 @@ spec:
                     type: object
                 type: object
               refreshInterval:
-                description: Used to configure store refresh interval in seconds.
-                  Empty or 0 will default to the controller config.
-                type: integer
+                anyOf:
+                - type: integer
+                - type: string
+                description: |-
+                  Used to configure store refresh interval. Accepts either an integer number
+                  of seconds (legacy) or a Go duration string such as "1h" or "5m". Empty or
+                  0 will default to the controller config.
+                x-kubernetes-int-or-string: true
               retrySettings:
                 description: Used to configure HTTP retries on failures.
                 properties:

config/crds/bases/external-secrets.io_secretstores.yaml

@@ -7385,9 +7385,14 @@ spec:
                     type: object
                 type: object
               refreshInterval:
-                description: Used to configure store refresh interval in seconds.
-                  Empty or 0 will default to the controller config.
-                type: integer
+                anyOf:
+                - type: integer
+                - type: string
+                description: |-
+                  Used to configure store refresh interval. Accepts either an integer number
+                  of seconds (legacy) or a Go duration string such as "1h" or "5m". Empty or
+                  0 will default to the controller config.
+                x-kubernetes-int-or-string: true
               retrySettings:
                 description: Used to configure HTTP retries on failures.
                 properties:

deploy/crds/bundle.yaml

@@ -9216,8 +9216,14 @@ spec:
                       type: object
                   type: object
                 refreshInterval:
-                  description: Used to configure store refresh interval in seconds. Empty or 0 will default to the controller config.
-                  type: integer
+                  anyOf:
+                    - type: integer
+                    - type: string
+                  description: |-
+                    Used to configure store refresh interval. Accepts either an integer number
+                    of seconds (legacy) or a Go duration string such as "1h" or "5m". Empty or
+                    0 will default to the controller config.
+                  x-kubernetes-int-or-string: true
                 retrySettings:
                   description: Used to configure HTTP retries on failures.
                   properties:
@@ -22410,8 +22416,14 @@ spec:
                       type: object
                   type: object
                 refreshInterval:
-                  description: Used to configure store refresh interval in seconds. Empty or 0 will default to the controller config.
-                  type: integer
+                  anyOf:
+                    - type: integer
+                    - type: string
+                  description: |-
+                    Used to configure store refresh interval. Accepts either an integer number
+                    of seconds (legacy) or a Go duration string such as "1h" or "5m". Empty or
+                    0 will default to the controller config.
+                  x-kubernetes-int-or-string: true
                 retrySettings:
                   description: Used to configure HTTP retries on failures.
                   properties:

docs/api/spec.md

@@ -3278,12 +3278,14 @@ SecretStoreRetrySettings
 <td>
 <code>refreshInterval</code></br>
 <em>
-int
+k8s.io/apimachinery/pkg/util/intstr.IntOrString
 </em>
 </td>
 <td>
 <em>(Optional)</em>
-<p>Used to configure store refresh interval in seconds. Empty or 0 will default to the controller config.</p>
+<p>Used to configure store refresh interval. Accepts either an integer number
+of seconds (legacy) or a Go duration string such as &ldquo;1h&rdquo; or &ldquo;5m&rdquo;. Empty or
+0 will default to the controller config.</p>
 </td>
 </tr>
 <tr>
@@ -10692,12 +10694,14 @@ SecretStoreRetrySettings
 <td>
 <code>refreshInterval</code></br>
 <em>
-int
+k8s.io/apimachinery/pkg/util/intstr.IntOrString
 </em>
 </td>
 <td>
 <em>(Optional)</em>
-<p>Used to configure store refresh interval in seconds. Empty or 0 will default to the controller config.</p>
+<p>Used to configure store refresh interval. Accepts either an integer number
+of seconds (legacy) or a Go duration string such as &ldquo;1h&rdquo; or &ldquo;5m&rdquo;. Empty or
+0 will default to the controller config.</p>
 </td>
 </tr>
 <tr>
@@ -11553,12 +11557,14 @@ SecretStoreRetrySettings
 <td>
 <code>refreshInterval</code></br>
 <em>
-int
+k8s.io/apimachinery/pkg/util/intstr.IntOrString
 </em>
 </td>
 <td>
 <em>(Optional)</em>
-<p>Used to configure store refresh interval in seconds. Empty or 0 will default to the controller config.</p>
+<p>Used to configure store refresh interval. Accepts either an integer number
+of seconds (legacy) or a Go duration string such as &ldquo;1h&rdquo; or &ldquo;5m&rdquo;. Empty or
+0 will default to the controller config.</p>
 </td>
 </tr>
 <tr>

pkg/controllers/secretstore/common.go

@@ -90,8 +90,12 @@ func reconcile(ctx context.Context, req ctrl.Request, ss esapi.GenericStore, cl
 
 	requeueInterval := opts.RequeueInterval
 
-	if ss.GetSpec().RefreshInterval != 0 {
-		requeueInterval = time.Second * time.Duration(ss.GetSpec().RefreshInterval)
+	refreshInterval, refreshErr := ss.GetSpec().GetRefreshInterval()
+	if refreshErr != nil {
+		return ctrl.Result{}, fmt.Errorf("invalid refreshInterval: %w", refreshErr)
+	}
+	if refreshInterval > 0 {
+		requeueInterval = refreshInterval
 	}
 
 	// patch status when done processing

tests/__snapshot__/clustersecretstore-v1.yaml

@@ -1086,7 +1086,7 @@ spec:
         byID: {}
         byName:
           folderID: string
-  refreshInterval: 1
+  refreshInterval: 
   retrySettings:
     maxRetries: 1
     retryInterval: string

tests/__snapshot__/secretstore-v1.yaml

@@ -1086,7 +1086,7 @@ spec:
         byID: {}
         byName:
           folderID: string
-  refreshInterval: 1
+  refreshInterval: 
   retrySettings:
     maxRetries: 1
     retryInterval: string