Skip to content

Commit

Permalink
Remove the logic for setting the nested protocol field in k8s objects
Browse files Browse the repository at this point in the history 
containing an array of `ports`.

The logic was added in March 2021 as a workaround for a known k8s issue,
which causes getting the declared fields for k8s objects containing an
array of `ports` to fail. The fixes to the k8s issue have been merged
into k8s 1.20 and 1.21:
1) kubernetes/kubernetes#96317 (merged in 1.20)
2) kubernetes/kubernetes#98576 (merged in 1.21).

k8s 1.21 is no longer supported on GKE and Anthos:
1) GKE release schedule: https://cloud.google.com/kubernetes-engine/docs/release-schedule
2) Anthos version and upgrade support: https://cloud.google.com/anthos/docs/version-and-upgrade-support

Therefore, we can remove the logic and related unit tests. We can keep
the e2e test to make sure this change does not break things.
  • Loading branch information
@haiyanmeng
haiyanmeng committed Feb 24, 2023
1 parent 2a55baa commit 550e87a
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 578 deletions.
31 changes: 28 additions & 3 deletions e2e/testcases/declared_fields_test.go
View file
Expand Up @@ -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")
Expand All @@ -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
Expand All @@ -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)
}
}
190 changes: 0 additions & 190 deletions pkg/validate/raw/hydrate/declared_field_hydrator.go
View file
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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
}

0 comments on commit 550e87a

Please sign in to comment.