From f3b83c0b055679a6925404d8ed3565b70dd1c0f6 Mon Sep 17 00:00:00 2001 From: Jakob Gillich Date: Fri, 18 Mar 2022 16:16:32 +0100 Subject: [PATCH] Fix empty resources spec field failing schema validation (#1589) In Go, when a struct field is not set, it becomes a struct with default values for all fields. These default values are included during serialization. This causes issues with schema validation where optional fields cannot be omitted because default values are considered invalid. This patch addresses this issue for `Resources` fields on several types by using a pointer value. --- pkg/apis/acid.zalan.do/v1/postgresql_type.go | 6 ++-- pkg/apis/acid.zalan.do/v1/util_test.go | 12 ++++---- pkg/cluster/cluster_test.go | 2 +- pkg/cluster/k8sres.go | 31 ++++++++++++-------- pkg/cluster/k8sres_test.go | 14 ++++----- 5 files changed, 35 insertions(+), 30 deletions(-) diff --git a/pkg/apis/acid.zalan.do/v1/postgresql_type.go b/pkg/apis/acid.zalan.do/v1/postgresql_type.go index 5bb24ed10..52aa66726 100644 --- a/pkg/apis/acid.zalan.do/v1/postgresql_type.go +++ b/pkg/apis/acid.zalan.do/v1/postgresql_type.go @@ -27,7 +27,7 @@ type PostgresSpec struct { PostgresqlParam `json:"postgresql"` Volume `json:"volume,omitempty"` Patroni `json:"patroni,omitempty"` - Resources `json:"resources,omitempty"` + *Resources `json:"resources,omitempty"` EnableConnectionPooler *bool `json:"enableConnectionPooler,omitempty"` EnableReplicaConnectionPooler *bool `json:"enableReplicaConnectionPooler,omitempty"` @@ -199,7 +199,7 @@ type CloneDescription struct { // Sidecar defines a container to be run in the same pod as the Postgres container. type Sidecar struct { - Resources `json:"resources,omitempty"` + *Resources `json:"resources,omitempty"` Name string `json:"name,omitempty"` DockerImage string `json:"image,omitempty"` Ports []v1.ContainerPort `json:"ports,omitempty"` @@ -232,7 +232,7 @@ type ConnectionPooler struct { DockerImage string `json:"dockerImage,omitempty"` MaxDBConnections *int32 `json:"maxDBConnections,omitempty"` - Resources `json:"resources,omitempty"` + *Resources `json:"resources,omitempty"` } type Stream struct { diff --git a/pkg/apis/acid.zalan.do/v1/util_test.go b/pkg/apis/acid.zalan.do/v1/util_test.go index bf6875a82..2ff40d347 100644 --- a/pkg/apis/acid.zalan.do/v1/util_test.go +++ b/pkg/apis/acid.zalan.do/v1/util_test.go @@ -163,7 +163,7 @@ var unmarshalCluster = []struct { "kind": "Postgresql","apiVersion": "acid.zalan.do/v1", "metadata": {"name": "acid-testcluster1"}, "spec": {"teamId": 100}}`), &tmp).Error(), }, - marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"acid-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null},"resources":{"requests":{"cpu":"","memory":""},"limits":{"cpu":"","memory":""}},"teamId":"","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":null},"status":"Invalid"}`), + marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"acid-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null},"teamId":"","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":null},"status":"Invalid"}`), err: nil}, { about: "example with /status subresource", @@ -184,7 +184,7 @@ var unmarshalCluster = []struct { "kind": "Postgresql","apiVersion": "acid.zalan.do/v1", "metadata": {"name": "acid-testcluster1"}, "spec": {"teamId": 100}}`), &tmp).Error(), }, - marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"acid-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null},"resources":{"requests":{"cpu":"","memory":""},"limits":{"cpu":"","memory":""}},"teamId":"","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":null},"status":{"PostgresClusterStatus":"Invalid"}}`), + marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"acid-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null},"teamId":"","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":null},"status":{"PostgresClusterStatus":"Invalid"}}`), err: nil}, { about: "example with detailed input manifest and deprecated pod_priority_class_name -> podPriorityClassName", @@ -300,7 +300,7 @@ var unmarshalCluster = []struct { MaximumLagOnFailover: 33554432, Slots: map[string]map[string]string{"permanent_logical_1": {"type": "logical", "database": "foo", "plugin": "pgoutput"}}, }, - Resources: Resources{ + Resources: &Resources{ ResourceRequests: ResourceDescription{CPU: "10m", Memory: "50Mi"}, ResourceLimits: ResourceDescription{CPU: "300m", Memory: "3000Mi"}, }, @@ -351,7 +351,7 @@ var unmarshalCluster = []struct { Status: PostgresStatus{PostgresClusterStatus: ClusterStatusInvalid}, Error: errors.New("name must match {TEAM}-{NAME} format").Error(), }, - marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"teapot-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null} ,"resources":{"requests":{"cpu":"","memory":""},"limits":{"cpu":"","memory":""}},"teamId":"acid","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":null},"status":{"PostgresClusterStatus":"Invalid"}}`), + marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"teapot-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null},"teamId":"acid","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":null},"status":{"PostgresClusterStatus":"Invalid"}}`), err: nil}, { about: "example with clone", @@ -373,7 +373,7 @@ var unmarshalCluster = []struct { }, Error: "", }, - marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"acid-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null},"resources":{"requests":{"cpu":"","memory":""},"limits":{"cpu":"","memory":""}},"teamId":"acid","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":{"cluster":"team-batman"}},"status":{"PostgresClusterStatus":""}}`), + marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"acid-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null},"teamId":"acid","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":{"cluster":"team-batman"}},"status":{"PostgresClusterStatus":""}}`), err: nil}, { about: "standby example", @@ -395,7 +395,7 @@ var unmarshalCluster = []struct { }, Error: "", }, - marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"acid-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null},"resources":{"requests":{"cpu":"","memory":""},"limits":{"cpu":"","memory":""}},"teamId":"acid","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"standby":{"s3_wal_path":"s3://custom/path/to/bucket/"}},"status":{"PostgresClusterStatus":""}}`), + marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"acid-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null},"teamId":"acid","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"standby":{"s3_wal_path":"s3://custom/path/to/bucket/"}},"status":{"PostgresClusterStatus":""}}`), err: nil}, { about: "expect error on malformatted JSON", diff --git a/pkg/cluster/cluster_test.go b/pkg/cluster/cluster_test.go index 98cc40f05..20c1c5c93 100644 --- a/pkg/cluster/cluster_test.go +++ b/pkg/cluster/cluster_test.go @@ -60,7 +60,7 @@ func TestStatefulSetAnnotations(t *testing.T) { testName := "CheckStatefulsetAnnotations" spec := acidv1.PostgresSpec{ TeamID: "myapp", NumberOfInstances: 1, - Resources: acidv1.Resources{ + Resources: &acidv1.Resources{ ResourceRequests: acidv1.ResourceDescription{CPU: "1", Memory: "10"}, ResourceLimits: acidv1.ResourceDescription{CPU: "1", Memory: "10"}, }, diff --git a/pkg/cluster/k8sres.go b/pkg/cluster/k8sres.go index 9dcca842e..7e96adacc 100644 --- a/pkg/cluster/k8sres.go +++ b/pkg/cluster/k8sres.go @@ -136,11 +136,18 @@ func (c *Cluster) makeDefaultResources() acidv1.Resources { } } -func generateResourceRequirements(resources acidv1.Resources, defaultResources acidv1.Resources) (*v1.ResourceRequirements, error) { +func generateResourceRequirements(resources *acidv1.Resources, defaultResources acidv1.Resources) (*v1.ResourceRequirements, error) { var err error - specRequests := resources.ResourceRequests - specLimits := resources.ResourceLimits + var specRequests acidv1.ResourceDescription + var specLimits acidv1.ResourceDescription + if resources == nil { + specRequests = acidv1.ResourceDescription{} + specLimits = acidv1.ResourceDescription{} + } else { + specRequests = resources.ResourceRequests + specLimits = resources.ResourceLimits + } result := v1.ResourceRequirements{} @@ -513,16 +520,14 @@ func generateSidecarContainers(sidecars []acidv1.Sidecar, if len(sidecars) > 0 { result := make([]v1.Container, 0) for index, sidecar := range sidecars { + var resourcesSpec acidv1.Resources + if sidecar.Resources == nil { + resourcesSpec = acidv1.Resources{} + } else { + sidecar.Resources.DeepCopyInto(&resourcesSpec) + } - resources, err := generateResourceRequirements( - makeResources( - sidecar.Resources.ResourceRequests.CPU, - sidecar.Resources.ResourceRequests.Memory, - sidecar.Resources.ResourceLimits.CPU, - sidecar.Resources.ResourceLimits.Memory, - ), - defaultResources, - ) + resources, err := generateResourceRequirements(&resourcesSpec, defaultResources) if err != nil { return nil, err } @@ -1387,7 +1392,7 @@ func generateScalyrSidecarSpec(clusterName, APIKey, serverURL, dockerImage strin scalyrCPULimit, scalyrMemoryLimit, ) - resourceRequirementsScalyrSidecar, err := generateResourceRequirements(resourcesScalyrSidecar, defaultResources) + resourceRequirementsScalyrSidecar, err := generateResourceRequirements(&resourcesScalyrSidecar, defaultResources) if err != nil { return nil, fmt.Errorf("invalid resources for Scalyr sidecar: %v", err) } diff --git a/pkg/cluster/k8sres_test.go b/pkg/cluster/k8sres_test.go index cb2fd83c0..daa8aa4e4 100644 --- a/pkg/cluster/k8sres_test.go +++ b/pkg/cluster/k8sres_test.go @@ -915,7 +915,7 @@ func TestNodeAffinity(t *testing.T) { makeSpec := func(nodeAffinity *v1.NodeAffinity) acidv1.PostgresSpec { return acidv1.PostgresSpec{ TeamID: "myapp", NumberOfInstances: 1, - Resources: acidv1.Resources{ + Resources: &acidv1.Resources{ ResourceRequests: acidv1.ResourceDescription{CPU: "1", Memory: "10"}, ResourceLimits: acidv1.ResourceDescription{CPU: "1", Memory: "10"}, }, @@ -1011,7 +1011,7 @@ func TestTLS(t *testing.T) { }, Spec: acidv1.PostgresSpec{ TeamID: "myapp", NumberOfInstances: 1, - Resources: acidv1.Resources{ + Resources: &acidv1.Resources{ ResourceRequests: acidv1.ResourceDescription{CPU: "1", Memory: "10"}, ResourceLimits: acidv1.ResourceDescription{CPU: "1", Memory: "10"}, }, @@ -1130,7 +1130,7 @@ func TestAdditionalVolume(t *testing.T) { }, Spec: acidv1.PostgresSpec{ TeamID: "myapp", NumberOfInstances: 1, - Resources: acidv1.Resources{ + Resources: &acidv1.Resources{ ResourceRequests: acidv1.ResourceDescription{CPU: "1", Memory: "10"}, ResourceLimits: acidv1.ResourceDescription{CPU: "1", Memory: "10"}, }, @@ -1236,7 +1236,7 @@ func TestSidecars(t *testing.T) { }, }, TeamID: "myapp", NumberOfInstances: 1, - Resources: acidv1.Resources{ + Resources: &acidv1.Resources{ ResourceRequests: acidv1.ResourceDescription{CPU: "1", Memory: "10"}, ResourceLimits: acidv1.ResourceDescription{CPU: "1", Memory: "10"}, }, @@ -1249,7 +1249,7 @@ func TestSidecars(t *testing.T) { }, acidv1.Sidecar{ Name: "cluster-specific-sidecar-with-resources", - Resources: acidv1.Resources{ + Resources: &acidv1.Resources{ ResourceRequests: acidv1.ResourceDescription{CPU: "210m", Memory: "0.8Gi"}, ResourceLimits: acidv1.ResourceDescription{CPU: "510m", Memory: "1.4Gi"}, }, @@ -1411,7 +1411,7 @@ func TestGenerateService(t *testing.T) { var enableLB bool = true spec = acidv1.PostgresSpec{ TeamID: "myapp", NumberOfInstances: 1, - Resources: acidv1.Resources{ + Resources: &acidv1.Resources{ ResourceRequests: acidv1.ResourceDescription{CPU: "1", Memory: "10"}, ResourceLimits: acidv1.ResourceDescription{CPU: "1", Memory: "10"}, }, @@ -1424,7 +1424,7 @@ func TestGenerateService(t *testing.T) { }, acidv1.Sidecar{ Name: "cluster-specific-sidecar-with-resources", - Resources: acidv1.Resources{ + Resources: &acidv1.Resources{ ResourceRequests: acidv1.ResourceDescription{CPU: "210m", Memory: "0.8Gi"}, ResourceLimits: acidv1.ResourceDescription{CPU: "510m", Memory: "1.4Gi"}, },