diff --git a/e2e/testcases/declared_fields_test.go b/e2e/testcases/declared_fields_test.go index 69c9c33883..993ce17592 100644 --- a/e2e/testcases/declared_fields_test.go +++ b/e2e/testcases/declared_fields_test.go @@ -27,7 +27,7 @@ import ( "kpt.dev/configsync/pkg/testing/fake" ) -func TestDeclaredFieldsPod(t *testing.T) { +func TestDeclaredFields(t *testing.T) { nt := nomostest.New(t, nomostesting.Reconciliation1, ntopts.Unstructured) namespace := fake.NamespaceObject("bookstore") @@ -48,7 +48,18 @@ spec: ports: - containerPort: 80 `)) - nt.RootRepos[configsync.RootSyncName].CommitAndPush("add pod missing protocol from port") + nt.RootRepos[configsync.RootSyncName].AddFile("acme/service.yaml", []byte(` +apiVersion: v1 +kind: Service +metadata: + name: nginx + namespace: bookstore +spec: + type: ExternalName + selector: + app: nginx +`)) + nt.RootRepos[configsync.RootSyncName].CommitAndPush("add a pod missing protocol from port and a ExternalName-type Service") nt.WaitForRepoSyncs() // Parse the pod yaml into an object @@ -59,12 +70,26 @@ spec: nt.T.Fatal(err) } + // Parse the service yaml into an object + svc := nt.RootRepos[configsync.RootSyncName].Get("acme/service.yaml") + + err = nt.Validate(svc.GetName(), svc.GetNamespace(), &corev1.Service{}) + if err != nil { + nt.T.Fatal(err) + } + nt.RootRepos[configsync.RootSyncName].Remove("acme/pod.yaml") - nt.RootRepos[configsync.RootSyncName].CommitAndPush("Remove the pod") + nt.RootRepos[configsync.RootSyncName].Remove("acme/service.yaml") + nt.RootRepos[configsync.RootSyncName].CommitAndPush("Remove the pod and the service") nt.WaitForRepoSyncs() err = nomostest.WatchForNotFound(nt, kinds.Pod(), pod.GetName(), pod.GetNamespace()) if err != nil { nt.T.Fatal(err) } + + err = nomostest.WatchForNotFound(nt, kinds.Service(), svc.GetName(), svc.GetNamespace()) + if err != nil { + nt.T.Fatal(err) + } } diff --git a/pkg/validate/raw/hydrate/declared_field_hydrator.go b/pkg/validate/raw/hydrate/declared_field_hydrator.go index 674ccb24ea..b5d73af618 100644 --- a/pkg/validate/raw/hydrate/declared_field_hydrator.go +++ b/pkg/validate/raw/hydrate/declared_field_hydrator.go @@ -15,17 +15,10 @@ package hydrate import ( - "errors" - "fmt" - "strings" - - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/klog/v2" "kpt.dev/configsync/pkg/core" "kpt.dev/configsync/pkg/declared" - "kpt.dev/configsync/pkg/kinds" "kpt.dev/configsync/pkg/metadata" "kpt.dev/configsync/pkg/status" "kpt.dev/configsync/pkg/validate/objects" @@ -94,14 +87,6 @@ var identityFields = fieldpath.NewSet( // is compatible with server-side apply. func encodeDeclaredFields(converter *declared.ValueConverter, obj runtime.Object) ([]byte, error) { var err error - u, isUnstructured := obj.(*unstructured.Unstructured) - if isUnstructured { - err = setDefaultProtocol(u) - if err != nil { - return nil, err - } - } - val, err := converter.TypedValue(obj) if err != nil { return nil, err @@ -115,178 +100,3 @@ func encodeDeclaredFields(converter *declared.ValueConverter, obj runtime.Object set = set.Difference(identityFields) return set.ToJSON() } - -// setDefaultProtocol sets the nested protocol field in anything containing -// an array of Ports. -// TODO: This should be deleted once we've upgraded to k8s 1.21 libraries. -func setDefaultProtocol(u *unstructured.Unstructured) status.MultiError { - var errs []error - switch u.GroupVersionKind().GroupKind() { - case kinds.Pod().GroupKind(): - errs = setDefaultProtocolInNestedPodSpec(u.Object, "spec") - case kinds.DaemonSet().GroupKind(), - kinds.Deployment().GroupKind(), - kinds.ReplicaSet().GroupKind(), - kinds.StatefulSet().GroupKind(), - kinds.Job().GroupKind(), - kinds.ReplicationController().GroupKind(): - errs = setDefaultProtocolInNestedPodSpec(u.Object, "spec", "template", "spec") - case kinds.CronJob().GroupKind(): - errs = setDefaultProtocolInNestedPodSpec(u.Object, "spec", "jobTemplate", "spec", "template", "spec") - case kinds.Service().GroupKind(): - errs = setDefaultProtocolInNestedPorts(u.Object, true, "spec", "ports") - } - - if len(errs) > 0 { - // These errors represent malformed objects. The user needs to correct their - // YAML/JSON as it is invalid. In almost all cases these errors are caught - // before here, but we still need to handle the errors rather than ignoring - // them. So this is _necessary_, but it doesn't need to be perfect. If in - // practice these errors come up more frequently we'll need to revisit. - message := "" - for _, err := range errs { - message += err.Error() + "\n" - } - return status.ObjectParseError(u, errors.New(message)) - } - - return nil -} - -func setDefaultProtocolInNestedPodSpec(obj map[string]interface{}, fields ...string) []error { - // We have to use the generic NestedFieldNoCopy and manually cast to a map as unstructured.NestedMap - // returns a deepcopy of the object, which does not allow us to modify the object in place. - podSpec, found, err := unstructured.NestedFieldNoCopy(obj, fields...) - if err != nil { - return []error{fmt.Errorf("unable to get pod spec: %w", err)} - } - if !found || podSpec == nil { - return []error{fmt.Errorf(".%s is required", strings.Join(fields, "."))} - } - - mPodSpec, ok := podSpec.(map[string]interface{}) - if !ok { - return []error{fmt.Errorf(".%s accessor error: %v is of the type %T, expected map[string]interface{}", strings.Join(fields, "."), podSpec, podSpec)} - } - - return setDefaultProtocolInPodSpec(mPodSpec, fields) -} - -func setDefaultProtocolInPodSpec(podSpec map[string]interface{}, fields []string) []error { - var errs []error - - // Use the more generic NestedField instead of NestedSlice. We can have occurences where - // the nested slice is empty/nill/null in the resource, causing unstructured.NestedSlice to - // error when it tries to assert nil to be []interface{}. We need to be able to ignore empty - // initContainers by handling nil values. - initContainers, found, err := unstructured.NestedFieldNoCopy(podSpec, "initContainers") - if err != nil { - errs = append(errs, err) - } else if found && initContainers != nil { - initContainersSlice, ok := initContainers.([]interface{}) - if !ok { - errs = append(errs, fmt.Errorf(".%s.initContainers accessor error: %v is of the type %T, expected []interface{}", strings.Join(fields, "."), initContainers, initContainers)) - } else { - errs = updateDefaultProtocolInContainers(podSpec, initContainersSlice, "initContainers", errs) - } - } - - // We don't need to use the generic NestedField function since we want it to error - // if the containers field is empty. A pod spec with no containers field is invalid. - containers, found, err := unstructured.NestedSlice(podSpec, "containers") - if err != nil { - errs = append(errs, err) - } else if found { - errs = updateDefaultProtocolInContainers(podSpec, containers, "containers", errs) - } - - return errs -} - -func updateDefaultProtocolInContainers(podSpec map[string]interface{}, containers []interface{}, field string, errs []error) []error { - setErrs := setDefaultProtocolInContainers(containers) - if len(setErrs) != 0 { - return append(errs, setErrs...) - } - - err := unstructured.SetNestedSlice(podSpec, containers, field) - if err != nil { - return append(errs, err) - } - - return errs -} - -func setDefaultProtocolInContainers(containers []interface{}) []error { - var errs []error - for _, c := range containers { - setErrs := setDefaultProtocolInContainer(c) - if len(setErrs) > 0 { - errs = append(errs, setErrs...) - } - } - return errs -} - -func setDefaultProtocolInContainer(container interface{}) []error { - mContainer, ok := container.(map[string]interface{}) - if !ok { - return []error{errors.New("container must be a map")} - } - - return setDefaultProtocolInNestedPorts(mContainer, false, "ports") -} - -func setDefaultProtocolInNestedPorts(obj map[string]interface{}, mustExist bool, fields ...string) []error { - ports, found, err := unstructured.NestedFieldNoCopy(obj, fields...) - if err != nil { - return []error{err} - } - if !found || ports == nil { - // Service resource requires the port field to be specified, or it is not a valid resource. - if mustExist { - return []error{fmt.Errorf(".%s is required", strings.Join(fields, "."))} - } - // Other resources can have empty ports field, and we can gracefully return early. - return nil - } - - sPorts, ok := ports.([]interface{}) - if !ok { - return []error{fmt.Errorf(".%s accessor error: %v is of the type %T, expected []interface{}", strings.Join(fields, "."), ports, ports)} - } - - setErrs := setDefaultProtocolInPorts(sPorts) - if len(setErrs) != 0 { - return setErrs - } - - err = unstructured.SetNestedSlice(obj, sPorts, fields...) - if err != nil { - return []error{err} - } - return nil -} - -func setDefaultProtocolInPorts(ports []interface{}) []error { - var errs []error - for _, p := range ports { - err := setDefaultProtocolInPort(p) - if err != nil { - errs = append(errs, err) - } - } - return errs -} - -func setDefaultProtocolInPort(port interface{}) error { - mPort, ok := port.(map[string]interface{}) - if !ok { - return errors.New("port must be a map") - } - - if _, found := mPort["protocol"]; !found { - mPort["protocol"] = string(corev1.ProtocolTCP) - } - return nil -} diff --git a/pkg/validate/raw/hydrate/declared_field_hydrator_raw_test.go b/pkg/validate/raw/hydrate/declared_field_hydrator_raw_test.go deleted file mode 100644 index a0f043b68a..0000000000 --- a/pkg/validate/raw/hydrate/declared_field_hydrator_raw_test.go +++ /dev/null @@ -1,385 +0,0 @@ -// Copyright 2022 Google LLC -// -// 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 hydrate - -import ( - "testing" - - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "kpt.dev/configsync/clientgen/apis/scheme" - "kpt.dev/configsync/pkg/importer/analyzer/ast" - "kpt.dev/configsync/pkg/testing/openapitest" - "kpt.dev/configsync/pkg/validate/objects" -) - -func TestRawYAML(t *testing.T) { - // We use literal YAML here instead of objects as: - // 1) If we used literal structs the protocol field would implicitly be added. - // 2) It's really annoying to specify these as Unstructureds. - testCases := []struct { - name string - expErr bool - yaml string - }{ - { - name: "Pod", - yaml: ` -apiVersion: v1 -kind: Pod -metadata: - name: nginx - namespace: bookstore -spec: - containers: - - image: nginx:1.7.9 - name: nginx - ports: - - containerPort: 80 -`, - }, - { - name: "Pod with protocol", - yaml: ` -apiVersion: v1 -kind: Pod -metadata: - name: nginx - namespace: bookstore -spec: - containers: - - image: nginx:1.7.9 - name: nginx - ports: - - containerPort: 80 - protocol: TCP -`, - }, - { - name: "Pod with empty ports section should not error", - yaml: ` -apiVersion: v1 -kind: Pod -metadata: - name: nginx - namespace: bookstore -spec: - containers: - - image: nginx:1.7.9 - name: nginx - ports: -`, - }, - { - name: "Pod initContainers", - yaml: ` -apiVersion: v1 -kind: Pod -metadata: - name: nginx - namespace: bookstore -spec: - containers: - - image: nginx:1.7.9 - name: nginx - initContainers: - - image: nginx:1.7.9 - name: nginx - ports: - - containerPort: 80 -`, - }, - { - name: "Pod initContainers empty - implicit", - yaml: ` -apiVersion: v1 -kind: Pod -metadata: - name: nginx - namespace: bookstore -spec: - containers: - - image: nginx:1.7.9 - name: nginx - initContainers: -`, - }, - { - name: "Pod initContainers empty - explicit", - yaml: ` -apiVersion: v1 -kind: Pod -metadata: - name: nginx - namespace: bookstore -spec: - containers: - - image: nginx:1.7.9 - name: nginx - initContainers: null -`, - }, - { - name: "ReplicationController", - yaml: ` -apiVersion: v1 -kind: ReplicationController -metadata: - name: nginx - namespace: bookstore -spec: - template: - metadata: - labels: - app: nginx - spec: - containers: - - image: nginx:1.7.9 - name: nginx - ports: - - containerPort: 80 -`, - }, - { - name: "DaemonSet", - yaml: ` -apiVersion: apps/v1 -kind: DaemonSet -metadata: - name: nginx - namespace: bookstore -spec: - selector: - matchLabels: - app: nginx - template: - metadata: - labels: - app: nginx - spec: - containers: - - image: nginx:1.7.9 - name: nginx - ports: - - containerPort: 80 -`, - }, - { - name: "Deployment", - yaml: ` -apiVersion: apps/v1 -kind: Deployment -metadata: - name: nginx - namespace: bookstore -spec: - replicas: 3 - selector: - matchLabels: - app: nginx - template: - metadata: - labels: - app: nginx - spec: - containers: - - image: nginx:1.7.9 - name: nginx - ports: - - containerPort: 80 -`, - }, - { - name: "ReplicaSet", - yaml: ` -apiVersion: apps/v1 -kind: ReplicaSet -metadata: - name: nginx - namespace: bookstore -spec: - replicas: 3 - selector: - matchLabels: - app: nginx - template: - metadata: - labels: - app: nginx - spec: - containers: - - image: nginx:1.7.9 - name: nginx - ports: - - containerPort: 80 -`, - }, - { - name: "StatefulSet", - yaml: ` -apiVersion: apps/v1 -kind: StatefulSet -metadata: - name: nginx - namespace: bookstore -spec: - replicas: 3 - selector: - matchLabels: - app: nginx - template: - metadata: - labels: - app: nginx - spec: - containers: - - image: nginx:1.7.9 - name: nginx - ports: - - containerPort: 80 -`, - }, - { - name: "Job", - yaml: ` -apiVersion: batch/v1 -kind: Job -metadata: - name: nginx - namespace: bookstore -spec: - selector: - matchLabels: - app: nginx - template: - metadata: - labels: - app: nginx - spec: - containers: - - image: nginx:1.7.9 - name: nginx - ports: - - containerPort: 80 -`, - }, - { - name: "CronJob", - yaml: ` -apiVersion: batch/v1beta1 -kind: CronJob -metadata: - name: nginx - namespace: bookstore -spec: - jobTemplate: - spec: - template: - spec: - containers: - - image: nginx:1.7.9 - name: nginx - ports: - - containerPort: 80 -`, - }, - { - name: "Service", - yaml: ` -apiVersion: v1 -kind: Service -metadata: - name: nginx - namespace: bookstore -spec: - selector: - app: nginx - ports: - - port: 80 -`, - }, - { - name: "Pod with empty containers field should error", - expErr: true, - yaml: ` -apiVersion: v1 -kind: Pod -metadata: - name: nginx - namespace: bookstore -spec: - containers: -`, - }, - { - name: "Pod with initContainers but not containers should error", - expErr: true, - yaml: ` -apiVersion: v1 -kind: Pod -metadata: - name: nginx - namespace: bookstore -spec: - initContainers: - - image: nginx:1.7.9 - name: nginx - ports: - - containerPort: 80 -`, - }, - { - name: "Service with no ports should error", - expErr: true, - yaml: ` -apiVersion: v1 -kind: Service -metadata: - name: nginx - namespace: bookstore -spec: - selector: - app: nginx -`, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - - u := &unstructured.Unstructured{} - _, _, err := scheme.Codecs.UniversalDeserializer().Decode([]byte(tc.yaml), nil, u) - if err != nil { - t.Fatal(err) - } - - converter, err := openapitest.ValueConverterForTest() - if err != nil { - t.Fatal(err) - } - - objs := &objects.Raw{ - Converter: converter, - Objects: []ast.FileObject{{ - Unstructured: u, - }}, - } - - err = DeclaredFields(objs) - if err != nil && !tc.expErr { - t.Fatal(err) - } - }) - } -}