Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the logic for setting the nested protocol field in k8s objects containing an array of ports. #416

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove the logic for setting the nested protocol field in k8s objects containing an array of ports. #416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions e2e/testcases/declared_fields_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
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)
}
}
205 changes: 4 additions & 201 deletions pkg/validate/raw/hydrate/declared_field_hydrator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
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 All @@ -47,17 +40,10 @@ func DeclaredFields(objs *objects.Raw) status.MultiError {
for _, obj := range objs.Objects {
fields, err := encodeDeclaredFields(objs.Converter, obj.Unstructured)
if err != nil {
switch err.(type) {
case status.MultiError:
// This error is from the function setDefaultProtocol.
// No schema checking involved.
errs = status.Append(errs, err)
default:
errs = status.Append(errs, status.EncodeDeclaredFieldError(obj.Unstructured, err))
// This error could be due to an out of date schema.
// So the converter needs to be refreshed.
needRefresh = true
}
errs = status.Append(errs, status.EncodeDeclaredFieldError(obj.Unstructured, err))
// This error could be due to an out of date schema.
// So the converter needs to be refreshed.
needRefresh = true
}
core.SetAnnotation(obj, metadata.DeclaredFieldsKey, string(fields))
}
Expand Down Expand Up @@ -94,14 +80,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 +93,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
}
Loading