From fc3c0302ad2ec86628b296ef80aafb099324b3d5 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 4 Oct 2023 15:43:51 +0200 Subject: [PATCH 1/4] ssa: improve and extend Unstructured normalization This allows for a single Unstructured object to be normalized, as was already possible for a slice of objects using `SetNativeKindsDefaults`. In addition, the further logic around normalization has be rewritten to improve maintainability and address certain edge-case issues. More specifically: - Any default Kubernetes resource is normalized by converting it to a typed resource, and back to an Unstructured. - Protocol default fixes have been added for Job and CronJob resources, which can hypothetically include port definitions. - Instead of removing the `creationTimestamp` field, it is written to `nil`, to equal the default set for Unstructured objects returned by the converter. - Test coverage has been added. Signed-off-by: Hidde Beydals --- ssa/go.mod | 2 +- ssa/normalize.go | 161 +++++++++ ssa/normalize_test.go | 767 ++++++++++++++++++++++++++++++++++++++++++ ssa/utils.go | 222 ++---------- 4 files changed, 951 insertions(+), 201 deletions(-) create mode 100644 ssa/normalize.go create mode 100644 ssa/normalize_test.go diff --git a/ssa/go.mod b/ssa/go.mod index 3c721528..32bfe7c1 100644 --- a/ssa/go.mod +++ b/ssa/go.mod @@ -9,6 +9,7 @@ require ( k8s.io/api v0.27.4 k8s.io/apimachinery v0.27.4 k8s.io/client-go v0.27.4 + k8s.io/utils v0.0.0-20230209194617-a36077c30491 sigs.k8s.io/cli-utils v0.35.0 sigs.k8s.io/controller-runtime v0.15.1 sigs.k8s.io/structured-merge-diff/v4 v4.3.0 @@ -78,7 +79,6 @@ require ( k8s.io/klog/v2 v2.90.1 // indirect k8s.io/kube-openapi v0.0.0-20230501164219-8b0f38b5fd1f // indirect k8s.io/kubectl v0.26.0 // indirect - k8s.io/utils v0.0.0-20230209194617-a36077c30491 // indirect sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect sigs.k8s.io/kustomize/api v0.12.1 // indirect sigs.k8s.io/kustomize/kyaml v0.13.9 // indirect diff --git a/ssa/normalize.go b/ssa/normalize.go new file mode 100644 index 00000000..ef07d876 --- /dev/null +++ b/ssa/normalize.go @@ -0,0 +1,161 @@ +/* +Copyright 2023 The Flux 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 + + http://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 ssa + +import ( + appsv1 "k8s.io/api/apps/v1" + batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/scheme" +) + +var defaultScheme = scheme.Scheme + +// FromUnstructured converts an Unstructured object into a typed Kubernetes +// resource. It only works for API types registered with the default client-go +// scheme. +func FromUnstructured(object *unstructured.Unstructured) (metav1.Object, error) { + return FromUnstructuredWithScheme(object, defaultScheme) +} + +// FromUnstructuredWithScheme converts an Unstructured object into a typed +// Kubernetes resource. It only works for API types registered with the given +// scheme. +func FromUnstructuredWithScheme(object *unstructured.Unstructured, scheme *runtime.Scheme) (metav1.Object, error) { + typedObject, err := scheme.New(object.GroupVersionKind()) + if err != nil { + return nil, err + } + + metaObject, ok := typedObject.(metav1.Object) + if !ok { + return nil, err + } + + if err = runtime.DefaultUnstructuredConverter.FromUnstructured(object.Object, metaObject); err != nil { + return nil, err + } + return metaObject, nil +} + +// ToUnstructured converts a typed Kubernetes resource into the Unstructured +// equivalent. +func ToUnstructured(object metav1.Object) (*unstructured.Unstructured, error) { + u, err := runtime.DefaultUnstructuredConverter.ToUnstructured(object) + if err != nil { + return nil, err + } + return &unstructured.Unstructured{Object: u}, nil +} + +// NormalizeUnstructured normalizes an Unstructured object by converting it to +// a typed Kubernetes resource, normalizing it, and then converting it back to +// an Unstructured object. It only works for API types registered with the +// default client-go scheme. If the conversion fails, only certain standard +// fields are removed. +func NormalizeUnstructured(object *unstructured.Unstructured) error { + return NormalizeUnstructuredWithScheme(object, defaultScheme) +} + +// NormalizeUnstructuredWithScheme normalizes an Unstructured object by +// converting it to a typed Kubernetes resource, normalizing it, and then +// converting it back to an Unstructured object. It only works for API types +// registered with the given scheme. If the conversion fails, only certain +// standard fields are removed. +func NormalizeUnstructuredWithScheme(object *unstructured.Unstructured, scheme *runtime.Scheme) error { + if typedObject, err := FromUnstructuredWithScheme(object, scheme); err == nil { + switch o := typedObject.(type) { + case *corev1.Pod: + normalizePodProtoDefault(&o.Spec) + case *appsv1.Deployment: + normalizePodProtoDefault(&o.Spec.Template.Spec) + case *appsv1.StatefulSet: + normalizePodProtoDefault(&o.Spec.Template.Spec) + case *appsv1.DaemonSet: + normalizePodProtoDefault(&o.Spec.Template.Spec) + case *appsv1.ReplicaSet: + normalizePodProtoDefault(&o.Spec.Template.Spec) + case *batchv1.Job: + normalizePodProtoDefault(&o.Spec.Template.Spec) + case *batchv1.CronJob: + normalizePodProtoDefault(&o.Spec.JobTemplate.Spec.Template.Spec) + case *corev1.Service: + normalizeServiceProtoDefault(&o.Spec) + case *corev1.Secret: + normalizeSecret(o) + } + + normalizedObject, err := ToUnstructured(typedObject) + if err != nil { + return err + } + object.Object = normalizedObject.Object + } + + // Ensure the object has an empty creation timestamp, to avoid + // issues with the Kubernetes API server rejecting the object + // or causing any spurious diffs. + unstructured.SetNestedField(object.Object, nil, "metadata", "creationTimestamp") + + // To ensure kstatus continues to work with CRDs, we need to keep the + // status field for CRDs. + if !IsCRD(object) { + unstructured.RemoveNestedField(object.Object, "status") + } + + return nil +} + +// normalizeServiceProtoDefault sets the default protocol for ports in a +// ServiceSpec. +// xref: https://github.com/kubernetes/kubernetes/pull/98576 +func normalizeServiceProtoDefault(spec *corev1.ServiceSpec) { + for i, port := range spec.Ports { + if len(port.Protocol) == 0 { + spec.Ports[i].Protocol = corev1.ProtocolTCP + } + } +} + +// normalizePodProtoDefault sets the default protocol for ports in a PodSpec. +// xref: https://github.com/kubernetes-sigs/structured-merge-diff/issues/130 +func normalizePodProtoDefault(spec *corev1.PodSpec) { + for _, c := range spec.Containers { + for i, port := range c.Ports { + if len(port.Protocol) == 0 { + c.Ports[i].Protocol = corev1.ProtocolTCP + } + } + } +} + +// normalizeSecret converts a Secret's StringData field to Data. +// xref: https://github.com/kubernetes/kubernetes/issues/108008 +func normalizeSecret(object *corev1.Secret) { + if len(object.StringData) > 0 { + if object.Data == nil { + object.Data = make(map[string][]byte, len(object.StringData)) + } + for k, v := range object.StringData { + object.Data[k] = []byte(v) + } + object.StringData = nil + } +} diff --git a/ssa/normalize_test.go b/ssa/normalize_test.go new file mode 100644 index 00000000..a2ddd890 --- /dev/null +++ b/ssa/normalize_test.go @@ -0,0 +1,767 @@ +/* +Copyright 2023 The Flux 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 + + http://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 ssa + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/pointer" +) + +func TestFromUnstructured(t *testing.T) { + tests := []struct { + name string + object *unstructured.Unstructured + want metav1.Object + wantErr bool + }{ + { + name: "valid Pod", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": map[string]interface{}{ + "name": "test-pod", + "namespace": "test-namespace", + }, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "name": "test-container", + "image": "test-image", + }, + }, + }, + }, + }, + want: &corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Pod", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "test-namespace", + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "test-container", + Image: "test-image", + }, + }, + }, + }, + }, + { + name: "valid Deployment", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]interface{}{ + "name": "test-deployment", + "namespace": "test-namespace", + }, + "spec": map[string]interface{}{ + "replicas": int64(1), + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "name": "test-container", + "image": "test-image", + }, + }, + }, + }, + }, + }, + }, + want: &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apps/v1", + Kind: "Deployment", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deployment", + Namespace: "test-namespace", + }, + Spec: appsv1.DeploymentSpec{ + Replicas: pointer.Int32(1), + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "test-container", + Image: "test-image", + }, + }, + }, + }, + }, + }, + }, + { + name: "unrecognized GroupVersionKind", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "test/v1", + "kind": "Test", + "metadata": map[string]interface{}{ + "name": "test-name", + "namespace": "test-namespace", + }, + }, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := FromUnstructured(tt.object) + if (err != nil) != tt.wantErr { + t.Errorf("FromUnstructured() error = %v, wantErr %v", err, tt.wantErr) + return + } + if diff := cmp.Diff(got, tt.want); diff != "" { + t.Errorf("FromUnstructured() mismatch (-want +got):\n%s", diff) + } + }) + } +} + +func TestFromUnstructuredWithScheme(t *testing.T) { + tests := []struct { + name string + object *unstructured.Unstructured + scheme *runtime.Scheme + want metav1.Object + wantErr bool + }{ + { + name: "valid Pod", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": map[string]interface{}{ + "name": "test-pod", + "namespace": "test-namespace", + }, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "name": "test-container", + "image": "test-image", + }, + }, + }, + }, + }, + scheme: defaultScheme, + want: &corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Pod", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "test-namespace", + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "test-container", + Image: "test-image", + }, + }, + }, + }, + }, + { + name: "unrecognized Deployment", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]interface{}{ + "name": "test-deployment", + "namespace": "test-namespace", + }, + "spec": map[string]interface{}{ + "replicas": int64(1), + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "name": "test-container", + "image": "test-image", + }, + }, + }, + }, + }, + }, + }, + scheme: runtime.NewScheme(), + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := FromUnstructuredWithScheme(tt.object, tt.scheme) + if (err != nil) != tt.wantErr { + t.Errorf("FromUnstructuredWithScheme() error = %v, wantErr %v", err, tt.wantErr) + return + } + if diff := cmp.Diff(got, tt.want); diff != "" { + t.Errorf("FromUnstructuredWithScheme() mismatch (-want +got):\n%s", diff) + } + }) + } +} + +func TestNormalizeUnstructured(t *testing.T) { + tests := []struct { + name string + scheme *runtime.Scheme + object *unstructured.Unstructured + want *unstructured.Unstructured + wantErr bool + }{ + { + name: "adds default port protocol to Pod", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Pod", + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "ports": []interface{}{ + map[string]interface{}{ + "containerPort": 80, + }, + map[string]interface{}{ + "containerPort": 8080, + }, + }, + }, + }, + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Pod", + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "name": "", + "ports": []interface{}{ + map[string]interface{}{ + "containerPort": int64(80), + "protocol": "TCP", + }, + map[string]interface{}{ + "containerPort": int64(8080), + "protocol": "TCP", + }, + }, + "resources": map[string]interface{}{}, + }, + }, + }, + }, + }, + }, + { + name: "adds default port protocol to Deployment", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "ports": []interface{}{ + map[string]interface{}{ + "containerPort": 80, + }, + }, + }, + }, + }, + }, + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "selector": nil, + "strategy": map[string]interface{}{}, + "template": map[string]interface{}{ + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "name": "", + "ports": []interface{}{ + map[string]interface{}{ + "containerPort": int64(80), + "protocol": "TCP", + }, + }, + "resources": map[string]interface{}{}, + }, + }, + }, + }, + }, + }, + }, + }, + { + name: "adds default port protocol to StatefulSet", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "StatefulSet", + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "ports": []interface{}{ + map[string]interface{}{ + "containerPort": 80, + }, + }, + }, + }, + }, + }, + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "StatefulSet", + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "selector": nil, + "serviceName": "", + "template": map[string]interface{}{ + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "name": "", + "ports": []interface{}{ + map[string]interface{}{ + "containerPort": int64(80), + "protocol": "TCP", + }, + }, + "resources": map[string]interface{}{}, + }, + }, + }, + }, + "updateStrategy": map[string]interface{}{}, + }, + }, + }, + }, + { + name: "adds default port protocol to DaemonSet", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "DaemonSet", + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "ports": []interface{}{ + map[string]interface{}{ + "containerPort": 80, + }, + }, + }, + }, + }, + }, + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "DaemonSet", + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "selector": nil, + "template": map[string]interface{}{ + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "name": "", + "ports": []interface{}{ + map[string]interface{}{ + "containerPort": int64(80), + "protocol": "TCP", + }, + }, + "resources": map[string]interface{}{}, + }, + }, + }, + }, + "updateStrategy": map[string]interface{}{}, + }, + }, + }, + }, + { + name: "adds default port protocol to ReplicaSet", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "ReplicaSet", + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "ports": []interface{}{ + map[string]interface{}{ + "containerPort": 80, + }, + }, + }, + }, + }, + }, + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "ReplicaSet", + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "selector": nil, + "template": map[string]interface{}{ + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "name": "", + "ports": []interface{}{ + map[string]interface{}{ + "containerPort": int64(80), + "protocol": "TCP", + }, + }, + "resources": map[string]interface{}{}, + }, + }, + }, + }, + }, + }, + }, + }, + { + name: "adds default port protocol to Job", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "batch/v1", + "kind": "Job", + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "ports": []interface{}{ + map[string]interface{}{ + "containerPort": 80, + }, + }, + }, + }, + }, + }, + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "batch/v1", + "kind": "Job", + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "name": "", + "ports": []interface{}{ + map[string]interface{}{ + "containerPort": int64(80), + "protocol": "TCP", + }, + }, + "resources": map[string]interface{}{}, + }, + }, + }, + }, + }, + }, + }, + }, + { + name: "adds default port protocol to CronJob", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "batch/v1", + "kind": "CronJob", + "spec": map[string]interface{}{ + "jobTemplate": map[string]interface{}{ + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "ports": []interface{}{ + map[string]interface{}{ + "containerPort": 80, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "batch/v1", + "kind": "CronJob", + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "jobTemplate": map[string]interface{}{ + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{ + "name": "", + "ports": []interface{}{ + map[string]interface{}{ + "containerPort": int64(80), + "protocol": "TCP", + }, + }, + "resources": map[string]interface{}{}, + }, + }, + }, + }, + }, + }, + "schedule": "", + }, + }, + }, + }, + { + name: "adds default port protocol to Service", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Service", + "spec": map[string]interface{}{ + "ports": []interface{}{ + map[string]interface{}{ + "port": 80, + }, + map[string]interface{}{ + "port": 443, + }, + }, + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Service", + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "ports": []interface{}{ + map[string]interface{}{ + "port": int64(80), + "protocol": "TCP", + "targetPort": int64(0), + }, + map[string]interface{}{ + "port": int64(443), + "protocol": "TCP", + "targetPort": int64(0), + }, + }, + }, + }, + }, + }, + { + name: "moves stringData to data in Secret", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "stringData": map[string]interface{}{ + "foo": "bar", + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "data": map[string]interface{}{ + "foo": "YmFy", + }, + }, + }, + }, + { + name: "removes status from any object", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "test/v1", + "kind": "Test", + "status": map[string]interface{}{ + "foo": "bar", + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "test/v1", + "kind": "Test", + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + }, + }, + }, + { + name: "nil writes creationTimestamp on any object", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "test/v1", + "kind": "Test", + "metadata": map[string]interface{}{ + "creationTimestamp": "2020-01-01T00:00:00Z", + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "test/v1", + "kind": "Test", + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := NormalizeUnstructured(tt.object); (err != nil) != tt.wantErr { + t.Errorf("NormalizeUnstructured() error = %v, wantErr %v", err, tt.wantErr) + } + if diff := cmp.Diff(tt.want, tt.object); diff != "" { + t.Errorf("NormalizeUnstructured() mismatch (-want +got):\n%s", diff) + } + }) + } +} diff --git a/ssa/utils.go b/ssa/utils.go index 24524a8c..2293de2c 100644 --- a/ssa/utils.go +++ b/ssa/utils.go @@ -24,11 +24,8 @@ import ( "regexp" "strings" - appsv1 "k8s.io/api/apps/v1" hpav2 "k8s.io/api/autoscaling/v2" - hpav2beta1 "k8s.io/api/autoscaling/v2beta1" hpav2beta2 "k8s.io/api/autoscaling/v2beta2" - corev1 "k8s.io/api/core/v1" apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -195,14 +192,25 @@ func ObjectsToJSON(objects []*unstructured.Unstructured) (string, error) { // IsClusterDefinition checks if the given object is a Kubernetes namespace or a custom resource definition. func IsClusterDefinition(object *unstructured.Unstructured) bool { - kind := object.GetKind() - switch strings.ToLower(kind) { - case "customresourcedefinition": + switch { + case IsCRD(object): return true - case "namespace": + case IsNamespace(object): return true + default: + return false } - return false +} + +// IsCRD returns true if the given object is a CustomResourceDefinition. +func IsCRD(object *unstructured.Unstructured) bool { + return strings.ToLower(object.GetKind()) == "customresourcedefinition" && + strings.HasPrefix(object.GetAPIVersion(), "apiextensions.k8s.io/") +} + +// IsNamespace returns true if the given object is a Namespace. +func IsNamespace(object *unstructured.Unstructured) bool { + return strings.ToLower(object.GetKind()) == "namespace" && object.GetAPIVersion() == "v1" } // IsKubernetesObject checks if the given object has the minimum required fields to be a Kubernetes object. @@ -215,10 +223,8 @@ func IsKubernetesObject(object *unstructured.Unstructured) bool { // IsKustomization checks if the given object is a Kustomize config. func IsKustomization(object *unstructured.Unstructured) bool { - if object.GetKind() == "Kustomization" && object.GroupVersionKind().GroupKind().Group == "kustomize.config.k8s.io" { - return true - } - return false + return strings.ToLower(object.GetKind()) == "kustomization" && + strings.HasPrefix(object.GetAPIVersion(), "kustomize.config.k8s.io/") } // IsSecret returns true if the given object is a Kubernetes Secret. @@ -265,183 +271,13 @@ func AnyInMetadata(object *unstructured.Unstructured, metadata map[string]string return false } -// SetNativeKindsDefaults implements workarounds for server-side apply upstream bugs affecting Kubernetes < 1.22 -// ContainerPort missing default TCP proto: https://github.com/kubernetes-sigs/structured-merge-diff/issues/130 -// ServicePort missing default TCP proto: https://github.com/kubernetes/kubernetes/pull/98576 -// PodSpec resources missing int to string conversion for e.g. 'cpu: 2' -// secret.stringData key replacement add an extra key in the resulting data map: https://github.com/kubernetes/kubernetes/issues/108008 +// SetNativeKindsDefaults sets default values for native Kubernetes objects, +// working around various upstream Kubernetes API bugs. func SetNativeKindsDefaults(objects []*unstructured.Unstructured) error { - - var setProtoDefault = func(spec *corev1.PodSpec) { - for _, c := range spec.Containers { - for i, port := range c.Ports { - if port.Protocol == "" { - c.Ports[i].Protocol = "TCP" - } - } - } - } - for _, u := range objects { - switch u.GetAPIVersion() { - case "v1": - switch u.GetKind() { - case "Service": - var d corev1.Service - err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - - // set port protocol default - // workaround for: https://github.com/kubernetes-sigs/structured-merge-diff/issues/130 - for i, port := range d.Spec.Ports { - if port.Protocol == "" { - d.Spec.Ports[i].Protocol = "TCP" - } - } - - out, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - u.Object = out - case "Pod": - var d corev1.Pod - err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - - setProtoDefault(&d.Spec) - - out, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - u.Object = out - case "Secret": - var s corev1.Secret - err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &s) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - convertStringDataToData(&s) - out, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&s) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - u.Object = out - } - - case "apps/v1": - switch u.GetKind() { - case "Deployment": - var d appsv1.Deployment - err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - - setProtoDefault(&d.Spec.Template.Spec) - - out, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - u.Object = out - case "StatefulSet": - var d appsv1.StatefulSet - err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - - setProtoDefault(&d.Spec.Template.Spec) - - out, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - u.Object = out - case "DaemonSet": - var d appsv1.DaemonSet - err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - - setProtoDefault(&d.Spec.Template.Spec) - - out, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - u.Object = out - case "ReplicaSet": - var d appsv1.ReplicaSet - err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - - setProtoDefault(&d.Spec.Template.Spec) - - out, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - u.Object = out - } - } - - switch u.GetKind() { - case "HorizontalPodAutoscaler": - switch u.GetAPIVersion() { - case "autoscaling/v2beta1": - var d hpav2beta1.HorizontalPodAutoscaler - err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - out, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - u.Object = out - case "autoscaling/v2beta2": - var d hpav2beta2.HorizontalPodAutoscaler - err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - out, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - u.Object = out - case "autoscaling/v2": - var d hpav2.HorizontalPodAutoscaler - err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - out, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - u.Object = out - } - } - - // remove fields that are not supposed to be present in manifests - unstructured.RemoveNestedField(u.Object, "metadata", "creationTimestamp") - - // remove status but for CRDs (kstatus wait doesn't work with empty status fields) - if u.GetKind() != "CustomResourceDefinition" { - unstructured.RemoveNestedField(u.Object, "status") + if err := NormalizeUnstructured(u); err != nil { + return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) } - } return nil } @@ -520,17 +356,3 @@ func containsItemString(s []string, e string) bool { } return false } - -func convertStringDataToData(secret *corev1.Secret) { - // StringData overwrites Data - if len(secret.StringData) > 0 { - if secret.Data == nil { - secret.Data = map[string][]byte{} - } - for k, v := range secret.StringData { - secret.Data[k] = []byte(v) - } - - secret.StringData = nil - } -} From 3773fe181897e9a7717fd265338bde7d566ddda6 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 4 Oct 2023 16:25:27 +0200 Subject: [PATCH 2/4] ssa: add post-dry-run normalize function To facilitate fixing issues to Kubernetes resources as returned by a server-side dry-run apply. The behavior of the function itself equals to the previous behavior of `fixHorizontalPodAutoscaler`. However, this is now exposed to allow other consumers (e.g. the to be introduced `jsondiff` sub-library) to make use of the same patches without further code duplication. Signed-off-by: Hidde Beydals --- ssa/manager_diff.go | 2 +- ssa/normalize.go | 58 +++++++++++++ ssa/normalize_test.go | 198 ++++++++++++++++++++++++++++++++++++++++++ ssa/utils.go | 69 --------------- 4 files changed, 257 insertions(+), 70 deletions(-) diff --git a/ssa/manager_diff.go b/ssa/manager_diff.go index a9ea1768..adaf71a5 100644 --- a/ssa/manager_diff.go +++ b/ssa/manager_diff.go @@ -116,7 +116,7 @@ func prepareObjectForDiff(object *unstructured.Unstructured) *unstructured.Unstr deepCopy := object.DeepCopy() unstructured.RemoveNestedField(deepCopy.Object, "metadata") unstructured.RemoveNestedField(deepCopy.Object, "status") - if err := fixHorizontalPodAutoscaler(deepCopy); err != nil { + if err := NormalizeDryRunUnstructured(deepCopy); err != nil { return object } return deepCopy diff --git a/ssa/normalize.go b/ssa/normalize.go index ef07d876..18678ac4 100644 --- a/ssa/normalize.go +++ b/ssa/normalize.go @@ -18,8 +18,11 @@ package ssa import ( appsv1 "k8s.io/api/apps/v1" + hpav2 "k8s.io/api/autoscaling/v2" + hpav2beta2 "k8s.io/api/autoscaling/v2beta2" batchv1 "k8s.io/api/batch/v1" corev1 "k8s.io/api/core/v1" + apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -123,6 +126,61 @@ func NormalizeUnstructuredWithScheme(object *unstructured.Unstructured, scheme * return nil } +// NormalizeDryRunUnstructured normalizes an Unstructured object retrieved from +// a dry-run by performing fixes for known upstream issues. +func NormalizeDryRunUnstructured(object *unstructured.Unstructured) error { + // Address an issue with dry-run returning a HorizontalPodAutoscaler + // with the first metric duplicated and an empty metric added at the + // end of the list. Which happens on Kubernetes < 1.27.x. + // xref: https://github.com/kubernetes/kubernetes/issues/118293 + if object.GetKind() == "HorizontalPodAutoscaler" { + typedObject, err := FromUnstructured(object) + if err != nil { + return err + } + + switch o := typedObject.(type) { + case *hpav2beta2.HorizontalPodAutoscaler: + var metrics []hpav2beta2.MetricSpec + for _, metric := range o.Spec.Metrics { + found := false + for _, existing := range metrics { + if apiequality.Semantic.DeepEqual(metric, existing) { + found = true + break + } + } + if !found && metric.Type != "" { + metrics = append(metrics, metric) + } + } + o.Spec.Metrics = metrics + case *hpav2.HorizontalPodAutoscaler: + var metrics []hpav2.MetricSpec + for _, metric := range o.Spec.Metrics { + found := false + for _, existing := range metrics { + if apiequality.Semantic.DeepEqual(metric, existing) { + found = true + break + } + } + if !found && metric.Type != "" { + metrics = append(metrics, metric) + } + } + o.Spec.Metrics = metrics + } + + normalizedObject, err := ToUnstructured(typedObject) + if err != nil { + return err + } + object.Object = normalizedObject.Object + } + return nil +} + // normalizeServiceProtoDefault sets the default protocol for ports in a // ServiceSpec. // xref: https://github.com/kubernetes/kubernetes/pull/98576 diff --git a/ssa/normalize_test.go b/ssa/normalize_test.go index a2ddd890..e0b3084d 100644 --- a/ssa/normalize_test.go +++ b/ssa/normalize_test.go @@ -765,3 +765,201 @@ func TestNormalizeUnstructured(t *testing.T) { }) } } + +func TestNormalizeDryRunUnstructured(t *testing.T) { + tests := []struct { + name string + object *unstructured.Unstructured + want *unstructured.Unstructured + wantErr bool + }{ + { + name: "removes duplicated metrics from v2beta2 HorizontalPodAutoscaler", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "autoscaling/v2beta2", + "kind": "HorizontalPodAutoscaler", + "spec": map[string]interface{}{ + "metrics": []interface{}{ + map[string]interface{}{ + "type": "Resource", + "resource": map[string]interface{}{ + "name": "cpu", + "target": map[string]interface{}{ + "type": "Utilization", + "averageUtilization": int64(60), + }, + }, + }, + map[string]interface{}{ + "type": "ContainerResource", + "containerResource": map[string]interface{}{ + "name": "cpu", + "container": "application", + "target": map[string]interface{}{ + "type": "Utilization", + "averageUtilization": int64(60), + }, + }, + }, + map[string]interface{}{ + "type": "Resource", + "resource": map[string]interface{}{ + "name": "cpu", + "target": map[string]interface{}{ + "type": "Utilization", + "averageUtilization": int64(60), + }, + }, + }, + map[string]interface{}{}, + }, + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "autoscaling/v2beta2", + "kind": "HorizontalPodAutoscaler", + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "maxReplicas": int64(0), + "metrics": []interface{}{ + map[string]interface{}{ + "type": "Resource", + "resource": map[string]interface{}{ + "name": "cpu", + "target": map[string]interface{}{ + "type": "Utilization", + "averageUtilization": int64(60), + }, + }, + }, + map[string]interface{}{ + "type": "ContainerResource", + "containerResource": map[string]interface{}{ + "name": "cpu", + "container": "application", + "target": map[string]interface{}{ + "type": "Utilization", + "averageUtilization": int64(60), + }, + }, + }, + }, + "scaleTargetRef": map[string]interface{}{ + "kind": "", + "name": "", + }, + }, + "status": map[string]interface{}{ + "conditions": nil, + "currentMetrics": nil, + "currentReplicas": int64(0), + "desiredReplicas": int64(0), + }, + }, + }, + }, + { + name: "removes duplicated metrics from v2 HorizontalPodAutoscaler", + object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "autoscaling/v2", + "kind": "HorizontalPodAutoscaler", + "spec": map[string]interface{}{ + "metrics": []interface{}{ + map[string]interface{}{ + "type": "Resource", + "resource": map[string]interface{}{ + "name": "cpu", + "target": map[string]interface{}{ + "type": "Utilization", + "averageUtilization": int64(60), + }, + }, + }, + map[string]interface{}{ + "type": "ContainerResource", + "containerResource": map[string]interface{}{ + "name": "cpu", + "container": "application", + "target": map[string]interface{}{ + "type": "Utilization", + "averageUtilization": int64(60), + }, + }, + }, + map[string]interface{}{ + "type": "Resource", + "resource": map[string]interface{}{ + "name": "cpu", + "target": map[string]interface{}{ + "type": "Utilization", + "averageUtilization": int64(60), + }, + }, + }, + map[string]interface{}{}, + }, + }, + }, + }, + want: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "autoscaling/v2", + "kind": "HorizontalPodAutoscaler", + "metadata": map[string]interface{}{ + "creationTimestamp": nil, + }, + "spec": map[string]interface{}{ + "maxReplicas": int64(0), + "metrics": []interface{}{ + map[string]interface{}{ + "type": "Resource", + "resource": map[string]interface{}{ + "name": "cpu", + "target": map[string]interface{}{ + "type": "Utilization", + "averageUtilization": int64(60), + }, + }, + }, + map[string]interface{}{ + "type": "ContainerResource", + "containerResource": map[string]interface{}{ + "name": "cpu", + "container": "application", + "target": map[string]interface{}{ + "type": "Utilization", + "averageUtilization": int64(60), + }, + }, + }, + }, + "scaleTargetRef": map[string]interface{}{ + "kind": "", + "name": "", + }, + }, + "status": map[string]interface{}{ + "currentMetrics": nil, + "desiredReplicas": int64(0), + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := NormalizeDryRunUnstructured(tt.object); (err != nil) != tt.wantErr { + t.Errorf("NormalizeDryRunUnstructured() error = %v, wantErr %v", err, tt.wantErr) + } + if diff := cmp.Diff(tt.want, tt.object); diff != "" { + t.Errorf("NormalizeDryRunUnstructured() mismatch (-want +got):\n%s", diff) + } + }) + } +} diff --git a/ssa/utils.go b/ssa/utils.go index 2293de2c..f523610a 100644 --- a/ssa/utils.go +++ b/ssa/utils.go @@ -24,9 +24,6 @@ import ( "regexp" "strings" - hpav2 "k8s.io/api/autoscaling/v2" - hpav2beta2 "k8s.io/api/autoscaling/v2beta2" - apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -282,72 +279,6 @@ func SetNativeKindsDefaults(objects []*unstructured.Unstructured) error { return nil } -// Fix bug in server-side dry-run apply that duplicates the first item in the metrics array -// and inserts an empty metric as the last item in the array. -func fixHorizontalPodAutoscaler(object *unstructured.Unstructured) error { - if object.GetKind() == "HorizontalPodAutoscaler" { - switch object.GetAPIVersion() { - case "autoscaling/v2beta2": - var d hpav2beta2.HorizontalPodAutoscaler - err := runtime.DefaultUnstructuredConverter.FromUnstructured(object.Object, &d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(object), err) - } - - var metrics []hpav2beta2.MetricSpec - for _, metric := range d.Spec.Metrics { - found := false - for _, existing := range metrics { - if apiequality.Semantic.DeepEqual(metric, existing) { - found = true - break - } - } - if !found && metric.Type != "" { - metrics = append(metrics, metric) - } - } - - d.Spec.Metrics = metrics - - out, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(object), err) - } - object.Object = out - case "autoscaling/v2": - var d hpav2.HorizontalPodAutoscaler - err := runtime.DefaultUnstructuredConverter.FromUnstructured(object.Object, &d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(object), err) - } - - var metrics []hpav2.MetricSpec - for _, metric := range d.Spec.Metrics { - found := false - for _, existing := range metrics { - if apiequality.Semantic.DeepEqual(metric, existing) { - found = true - break - } - } - if !found && metric.Type != "" { - metrics = append(metrics, metric) - } - } - - d.Spec.Metrics = metrics - - out, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&d) - if err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(object), err) - } - object.Object = out - } - } - return nil -} - func containsItemString(s []string, e string) bool { for _, a := range s { if a == e { From e1cb374995a9a390dc59ef0cc57ff7422dd6c06f Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Fri, 6 Oct 2023 15:23:53 +0200 Subject: [PATCH 3/4] ssa: deprecate `SetNativeKindsDefaults` In favor of the newly added `NormalizeUnstructuredList`. Signed-off-by: Hidde Beydals --- ssa/manager_apply_test.go | 8 ++++---- ssa/manager_diff_test.go | 18 ++++++++++++------ ssa/normalize.go | 25 +++++++++++++++++++++++++ ssa/utils.go | 10 +++------- 4 files changed, 44 insertions(+), 17 deletions(-) diff --git a/ssa/manager_apply_test.go b/ssa/manager_apply_test.go index f3004824..61f12811 100644 --- a/ssa/manager_apply_test.go +++ b/ssa/manager_apply_test.go @@ -377,7 +377,7 @@ func TestApply_SetNativeKindsDefaults(t *testing.T) { manager.SetOwnerLabels(objects, "app1", "default") - if err := SetNativeKindsDefaults(objects); err != nil { + if err := NormalizeUnstructuredList(objects); err != nil { t.Fatal(err) } @@ -417,7 +417,7 @@ func TestApply_NoOp(t *testing.T) { manager.SetOwnerLabels(objects, "app1", "default") - if err := SetNativeKindsDefaults(objects); err != nil { + if err := NormalizeUnstructuredList(objects); err != nil { t.Fatal(err) } @@ -639,7 +639,7 @@ func TestApply_Cleanup(t *testing.T) { _, deployObject := getFirstObject(objects, "Deployment", id) - if err := SetNativeKindsDefaults(objects); err != nil { + if err = NormalizeUnstructuredList(objects); err != nil { t.Fatal(err) } @@ -897,7 +897,7 @@ func TestApply_Cleanup_Exclusions(t *testing.T) { _, deployObject := getFirstObject(objects, "Deployment", id) - if err := SetNativeKindsDefaults(objects); err != nil { + if err = NormalizeUnstructuredList(objects); err != nil { t.Fatal(err) } diff --git a/ssa/manager_diff_test.go b/ssa/manager_diff_test.go index 8aa72dd0..5f30ef3d 100644 --- a/ssa/manager_diff_test.go +++ b/ssa/manager_diff_test.go @@ -90,7 +90,7 @@ func TestDiff(t *testing.T) { t.Run("generates diff for replaced key in stringData secret", func(t *testing.T) { // create a new stringData secret sec := secret.DeepCopy() - if err := unstructured.SetNestedField(sec.Object, generateName("diff"), "metadata", "name"); err != nil { + if err = unstructured.SetNestedField(sec.Object, generateName("diff"), "metadata", "name"); err != nil { t.Fatal(err) } @@ -98,7 +98,9 @@ func TestDiff(t *testing.T) { diffSecret := sec.DeepCopy() // apply stringData conversion - SetNativeKindsDefaults([]*unstructured.Unstructured{sec}) + if err = NormalizeUnstructured(sec); err != nil { + t.Fatal(err) + } if _, err = manager.Apply(ctx, sec, DefaultApplyOptions()); err != nil { t.Fatal(err) @@ -108,13 +110,14 @@ func TestDiff(t *testing.T) { unstructured.RemoveNestedField(diffSecret.Object, "stringData", "key") newKey := "key.new" - err = unstructured.SetNestedField(diffSecret.Object, newVal, "stringData", newKey) - if err != nil { + if err = unstructured.SetNestedField(diffSecret.Object, newVal, "stringData", newKey); err != nil { t.Fatal(err) } // apply stringData conversion - SetNativeKindsDefaults([]*unstructured.Unstructured{diffSecret}) + if err = NormalizeUnstructured(diffSecret); err != nil { + t.Fatal(err) + } _, liveObj, mergedObj, err := manager.Diff(ctx, diffSecret, DefaultDiffOptions()) if err != nil { @@ -234,7 +237,10 @@ func TestDiff_Removals(t *testing.T) { if err != nil { t.Fatal(err) } - SetNativeKindsDefaults(objects) + + if err = NormalizeUnstructuredList(objects); err != nil { + t.Fatal(err) + } configMapName, configMap := getFirstObject(objects, "ConfigMap", id) diff --git a/ssa/normalize.go b/ssa/normalize.go index 18678ac4..f8ffe6c6 100644 --- a/ssa/normalize.go +++ b/ssa/normalize.go @@ -17,6 +17,8 @@ limitations under the License. package ssa import ( + "fmt" + appsv1 "k8s.io/api/apps/v1" hpav2 "k8s.io/api/autoscaling/v2" hpav2beta2 "k8s.io/api/autoscaling/v2beta2" @@ -68,6 +70,29 @@ func ToUnstructured(object metav1.Object) (*unstructured.Unstructured, error) { return &unstructured.Unstructured{Object: u}, nil } +// NormalizeUnstructuredList normalizes a list of Unstructured objects by +// converting them to typed Kubernetes resources, normalizing them, and then +// converting them back to Unstructured objects. It only works for API types +// registered with the default client-go scheme. If the conversion fails, only +// certain standard fields are removed. +func NormalizeUnstructuredList(objects []*unstructured.Unstructured) error { + return NormalizeUnstructuredListWithScheme(objects, defaultScheme) +} + +// NormalizeUnstructuredListWithScheme normalizes a list of Unstructured +// objects by converting them to typed Kubernetes resources, normalizing them, +// and then converting them back to Unstructured objects. It only works for API +// types registered with the given scheme. If the conversion fails, only +// certain standard fields are removed. +func NormalizeUnstructuredListWithScheme(objects []*unstructured.Unstructured, scheme *runtime.Scheme) error { + for _, o := range objects { + if err := NormalizeUnstructuredWithScheme(o, scheme); err != nil { + return fmt.Errorf("%s normalization error: %w", FmtUnstructured(o), err) + } + } + return nil +} + // NormalizeUnstructured normalizes an Unstructured object by converting it to // a typed Kubernetes resource, normalizing it, and then converting it back to // an Unstructured object. It only works for API types registered with the diff --git a/ssa/utils.go b/ssa/utils.go index f523610a..23d55858 100644 --- a/ssa/utils.go +++ b/ssa/utils.go @@ -19,7 +19,6 @@ package ssa import ( "encoding/json" - "fmt" "io" "regexp" "strings" @@ -270,13 +269,10 @@ func AnyInMetadata(object *unstructured.Unstructured, metadata map[string]string // SetNativeKindsDefaults sets default values for native Kubernetes objects, // working around various upstream Kubernetes API bugs. +// +// Deprecated: use NormalizeUnstructuredList or NormalizeUnstructured instead. func SetNativeKindsDefaults(objects []*unstructured.Unstructured) error { - for _, u := range objects { - if err := NormalizeUnstructured(u); err != nil { - return fmt.Errorf("%s validation error: %w", FmtUnstructured(u), err) - } - } - return nil + return NormalizeUnstructuredList(objects) } func containsItemString(s []string, e string) bool { From dd98209a95d96771af1c39af90f4d4436f512195 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Fri, 6 Oct 2023 23:33:59 +0200 Subject: [PATCH 4/4] ssa: make dry-run error typed This enriches the options a consumer has to debug the error, as it now provides access to the Unstructured which produced the error. While also allowing the upcoming `jsondiff` sub-package to reuse and emit the error. Signed-off-by: Hidde Beydals --- ssa/errors.go | 76 +++++++++++++++++++++++++++++++++++++++ ssa/manager_apply.go | 4 +-- ssa/manager_apply_test.go | 2 +- ssa/manager_diff.go | 29 +-------------- 4 files changed, 80 insertions(+), 31 deletions(-) create mode 100644 ssa/errors.go diff --git a/ssa/errors.go b/ssa/errors.go new file mode 100644 index 00000000..dc300dd2 --- /dev/null +++ b/ssa/errors.go @@ -0,0 +1,76 @@ +/* +Copyright 2023 The Flux 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 + + http://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 ssa + +import ( + "fmt" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +// DryRunErr is an error that occurs during a server-side dry-run apply. +type DryRunErr struct { + underlyingErr error + involvedObject *unstructured.Unstructured +} + +// NewDryRunErr returns a new DryRunErr. +func NewDryRunErr(err error, involvedObject *unstructured.Unstructured) *DryRunErr { + return &DryRunErr{ + underlyingErr: err, + involvedObject: involvedObject, + } +} + +// InvolvedObject returns the involved object. +func (e *DryRunErr) InvolvedObject() *unstructured.Unstructured { + return e.involvedObject +} + +// Error returns the error message. +func (e *DryRunErr) Error() string { + if e.involvedObject == nil { + return e.underlyingErr.Error() + } + + if apierrors.IsNotFound(e.Unwrap()) { + if e.involvedObject.GetNamespace() != "" { + return fmt.Sprintf("%s namespace not specified: %s", FmtUnstructured(e.involvedObject), e.Unwrap().Error()) + } + return fmt.Sprintf("%s not found: %s", FmtUnstructured(e.involvedObject), e.Unwrap().Error()) + } + + reason := fmt.Sprintf("%s", apierrors.ReasonForError(e.Unwrap())) + + // Detect managed field conflict. + if status, ok := apierrors.StatusCause(e.Unwrap(), metav1.CauseTypeFieldManagerConflict); ok { + reason = fmt.Sprintf("%s", status.Type) + } + + if reason != "" { + reason = fmt.Sprintf(" (%s)", reason) + } + + return fmt.Sprintf("%s dry-run failed%s: %s", FmtUnstructured(e.involvedObject), reason, e.underlyingErr.Error()) +} + +// Unwrap returns the underlying error. +func (e *DryRunErr) Unwrap() error { + return e.underlyingErr +} diff --git a/ssa/manager_apply.go b/ssa/manager_apply.go index 1863ba6e..f76413c9 100644 --- a/ssa/manager_apply.go +++ b/ssa/manager_apply.go @@ -108,7 +108,7 @@ func (m *ResourceManager) Apply(ctx context.Context, object *unstructured.Unstru return m.Apply(ctx, object, opts) } - return nil, m.validationError(dryRunObject, err) + return nil, NewDryRunErr(err, dryRunObject) } patched, err := m.cleanupMetadata(ctx, object, existingObject, opts.Cleanup) @@ -191,7 +191,7 @@ func (m *ResourceManager) ApplyAll(ctx context.Context, objects []*unstructured. } if err != nil { - return m.validationError(dryRunObject, err) + return NewDryRunErr(err, dryRunObject) } } diff --git a/ssa/manager_apply_test.go b/ssa/manager_apply_test.go index 61f12811..34bc9cb4 100644 --- a/ssa/manager_apply_test.go +++ b/ssa/manager_apply_test.go @@ -176,7 +176,7 @@ func TestApply_Force(t *testing.T) { // verify that the error message does not contain sensitive information expectedErr := fmt.Sprintf( - "%s dry-run failed, reason: Invalid: Secret \"%s\" is invalid: data: Forbidden: field is immutable when `immutable` is set", + "%s dry-run failed (Invalid): Secret \"%s\" is invalid: data: Forbidden: field is immutable when `immutable` is set", FmtUnstructured(secret), secret.GetName()) if diff := cmp.Diff(expectedErr, err.Error()); diff != "" { t.Errorf("Mismatch from expected value (-want +got):\n%s", diff) diff --git a/ssa/manager_diff.go b/ssa/manager_diff.go index adaf71a5..2a3d7d72 100644 --- a/ssa/manager_diff.go +++ b/ssa/manager_diff.go @@ -19,11 +19,7 @@ package ssa import ( "context" - "fmt" - apiequality "k8s.io/apimachinery/pkg/api/equality" - apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -61,7 +57,7 @@ func (m *ResourceManager) Diff(ctx context.Context, object *unstructured.Unstruc dryRunObject := object.DeepCopy() if err := m.dryRunApply(ctx, dryRunObject); err != nil { - return nil, nil, nil, m.validationError(dryRunObject, err) + return nil, nil, nil, NewDryRunErr(err, dryRunObject) } if dryRunObject.GetResourceVersion() == "" { @@ -121,26 +117,3 @@ func prepareObjectForDiff(object *unstructured.Unstructured) *unstructured.Unstr } return deepCopy } - -// validationError formats the given error and hides sensitive data -// if the error was caused by an invalid Kubernetes secrets. -func (m *ResourceManager) validationError(object *unstructured.Unstructured, err error) error { - if apierrors.IsNotFound(err) { - return fmt.Errorf("%s namespace not specified: %w", FmtUnstructured(object), err) - } - - reason := fmt.Sprintf("%v", apierrors.ReasonForError(err)) - - // detect managed field conflict - if status, ok := apierrors.StatusCause(err, metav1.CauseTypeFieldManagerConflict); ok { - reason = fmt.Sprintf("%v", status.Type) - } - - if reason != "" { - reason = fmt.Sprintf(", reason: %s", reason) - } - - return fmt.Errorf("%s dry-run failed%s: %w", - FmtUnstructured(object), reason, err) - -}