From 1140fe0db41b74e106a62f3d8ee35df3066ca8b2 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Tue, 18 Feb 2025 11:35:51 -0600 Subject: [PATCH] Document a Kubernetes bug with the duration format Fractional numbers are valid but not parsed correctly. See: https://www.github.com/kubernetes/kube-openapi/issues/523 --- .../postgres-operator.crunchydata.com_pgadmins.yaml | 2 +- ...es-operator.crunchydata.com_postgresclusters.yaml | 2 +- internal/testing/cmp/cmp.go | 10 ++++++++-- internal/testing/validation/pgadmin_test.go | 3 ++- .../v1beta1/instrumentation_types.go | 3 ++- .../v1beta1/shared_types.go | 3 +++ .../v1beta1/shared_types_test.go | 12 ++++++++++++ 7 files changed, 29 insertions(+), 6 deletions(-) diff --git a/config/crd/bases/postgres-operator.crunchydata.com_pgadmins.yaml b/config/crd/bases/postgres-operator.crunchydata.com_pgadmins.yaml index 3a6f881721..90890e2371 100644 --- a/config/crd/bases/postgres-operator.crunchydata.com_pgadmins.yaml +++ b/config/crd/bases/postgres-operator.crunchydata.com_pgadmins.yaml @@ -1952,7 +1952,7 @@ spec: retentionPeriod: description: |- How long to retain log files locally. An RFC 3339 duration or a number - and unit: `3d`, `4 weeks`, `12 hr`, etc. + and unit: `12 hr`, `3d`, `4 weeks`, etc. format: duration maxLength: 20 minLength: 1 diff --git a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml index e7dac855bb..b2af2deb47 100644 --- a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml +++ b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml @@ -11485,7 +11485,7 @@ spec: retentionPeriod: description: |- How long to retain log files locally. An RFC 3339 duration or a number - and unit: `3d`, `4 weeks`, `12 hr`, etc. + and unit: `12 hr`, `3d`, `4 weeks`, etc. format: duration maxLength: 20 minLength: 1 diff --git a/internal/testing/cmp/cmp.go b/internal/testing/cmp/cmp.go index 6da0edecf4..df138dbf4b 100644 --- a/internal/testing/cmp/cmp.go +++ b/internal/testing/cmp/cmp.go @@ -5,6 +5,7 @@ package cmp import ( + "regexp" "strings" gocmp "github.com/google/go-cmp/cmp" @@ -46,10 +47,15 @@ func Contains(collection, item any) Comparison { // succeeds if the values are equal. The comparison can be customized using // comparison Options. See [github.com/google/go-cmp/cmp.Option] constructors // and [github.com/google/go-cmp/cmp/cmpopts]. -func DeepEqual(x, y any, opts ...gocmp.Option) Comparison { +func DeepEqual[T any](x, y T, opts ...gocmp.Option) Comparison { return gotest.DeepEqual(x, y, opts...) } +// Len succeeds if actual has the expected length. +func Len[Slice ~[]E, E any](actual Slice, expected int) Comparison { + return gotest.Len(actual, expected) +} + // MarshalContains converts actual to YAML and succeeds if expected is in the result. func MarshalContains(actual any, expected string) Comparison { b, err := yaml.Marshal(actual) @@ -71,6 +77,6 @@ func MarshalMatches(actual any, expected string) Comparison { // Regexp succeeds if value contains any match of the regular expression re. // The regular expression may be a *regexp.Regexp or a string that is a valid // regexp pattern. -func Regexp(re any, value string) Comparison { +func Regexp[RE *regexp.Regexp | ~string](re RE, value string) Comparison { return gotest.Regexp(re, value) } diff --git a/internal/testing/validation/pgadmin_test.go b/internal/testing/validation/pgadmin_test.go index 082c877370..d2ba6e095f 100644 --- a/internal/testing/validation/pgadmin_test.go +++ b/internal/testing/validation/pgadmin_test.go @@ -15,6 +15,7 @@ import ( "sigs.k8s.io/yaml" "github.com/crunchydata/postgres-operator/internal/controller/runtime" + "github.com/crunchydata/postgres-operator/internal/testing/cmp" "github.com/crunchydata/postgres-operator/internal/testing/require" "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) @@ -49,7 +50,7 @@ func TestPGAdminInstrumentation(t *testing.T) { //nolint:errorlint // This is a test, and a panic is unlikely. status := err.(apierrors.APIStatus).Status() assert.Assert(t, status.Details != nil) - assert.Equal(t, len(status.Details.Causes), 2) + assert.Assert(t, cmp.Len(status.Details.Causes, 2)) for _, cause := range status.Details.Causes { assert.Equal(t, cause.Field, "spec.instrumentation.logs.retentionPeriod") diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/instrumentation_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/instrumentation_types.go index f99a54fafa..93613bd1fc 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/instrumentation_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/instrumentation_types.go @@ -54,10 +54,11 @@ type InstrumentationLogsSpec struct { Exporters []string `json:"exporters,omitempty"` // How long to retain log files locally. An RFC 3339 duration or a number - // and unit: `3d`, `4 weeks`, `12 hr`, etc. + // and unit: `12 hr`, `3d`, `4 weeks`, etc. // --- // Kubernetes ensures the value is in the "duration" format, but go ahead // and loosely validate the format to show some acceptable units. + // NOTE: This rejects fractional numbers: https://github.com/kubernetes/kube-openapi/issues/523 // +kubebuilder:validation:Pattern=`^(PT)?( *[0-9]+ *(?i:(h|hr|d|w|wk)|(hour|day|week)s?))+$` // // `controller-gen` needs to know "Type=string" to allow a "Pattern". diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types.go index 72a7042d48..4b999597bf 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types.go @@ -40,6 +40,9 @@ import ( // - https://docs.k8s.io/reference/using-api/cel/#type-system-integration // - https://github.com/google/cel-spec/blob/-/doc/langdef.md#types-and-conversions // +// NOTE: When using this type, reject fractional numbers using a Pattern to +// avoid an upstream bug: https://github.com/kubernetes/kube-openapi/issues/523 +// // [defined by OpenAPI]: https://spec.openapis.org/registry/format/duration.html // [format]: https://spec.openapis.org/oas/latest.html#data-type-format type Duration struct { diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types_test.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types_test.go index 45c1556cd8..1dde5359a0 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types_test.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/shared_types_test.go @@ -139,6 +139,18 @@ func TestDurationYAML(t *testing.T) { assert.Assert(t, !strfmt.IsDuration(tt)) } }) + + t.Run("DoNotUsePartialAmounts", func(t *testing.T) { + var parsed Duration + assert.NilError(t, yaml.Unmarshal([]byte(`1.5 hours`), &parsed)) + + expected, err := time.ParseDuration(`1.5h`) + assert.NilError(t, err) + + // The parsed value is *not* the expected amount. + assert.Assert(t, parsed.AsDuration().Duration != expected, + "expected https://github.com/kubernetes/kube-openapi/issues/523") + }) } func TestSchemalessObjectDeepCopy(t *testing.T) {