Merge pull request #17187 from justinsb/wip_cleanup_options_checking

Refactor validation logic around checking for multiple options

Author: k8s-ci-robot

pkg/apis/kops/networking.go

@@ -16,7 +16,12 @@ limitations under the License.
 
 package kops
 
-import "k8s.io/apimachinery/pkg/api/resource"
+import (
+	"k8s.io/apimachinery/pkg/api/resource"
+	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/klog/v2"
+	"k8s.io/kops/util/pkg/reflectutils"
+)
 
 // NetworkingSpec configures networking.
 type NetworkingSpec struct {
@@ -81,6 +86,16 @@ type NetworkingSpec struct {
 	Kindnet    *KindnetNetworkingSpec    `json:"kindnet,omitempty"`
 }
 
+// ConfiguredOptions returns the set of networking options that are configured (non-nil)
+// in the struct.  We only expect a single option to be configured.
+func (n *NetworkingSpec) ConfiguredOptions() sets.Set[string] {
+	options, err := reflectutils.FindSetFields(n, "classic", "kubenet", "external", "cni", "kopeio", "weave", "flannel", "calico", "canal", "kubeRouter", "romana", "amazonVPC", "cilium", "lyftvpc", "gcp", "kindnet")
+	if err != nil {
+		klog.Fatalf("error getting set fields: %v", err)
+	}
+	return options
+}
+
 // UsesKubenet returns true if our networking is derived from kubenet
 func (n *NetworkingSpec) UsesKubenet() bool {
 	if n == nil {

pkg/apis/kops/v1alpha2/networking.go

@@ -18,6 +18,9 @@ package v1alpha2
 
 import (
 	"k8s.io/apimachinery/pkg/api/resource"
+	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/klog/v2"
+	"k8s.io/kops/util/pkg/reflectutils"
 )
 
 // NetworkingSpec allows selection and configuration of a networking plugin
@@ -53,9 +56,17 @@ type NetworkingSpec struct {
 }
 
 func (s *NetworkingSpec) IsEmpty() bool {
-	return s.Classic == nil && s.Kubenet == nil && s.External == nil && s.CNI == nil && s.Kopeio == nil &&
-		s.Weave == nil && s.Flannel == nil && s.Calico == nil && s.Canal == nil && s.KubeRouter == nil &&
-		s.Romana == nil && s.AmazonVPC == nil && s.Cilium == nil && s.LyftVPC == nil && s.GCP == nil && s.Kindnet == nil
+	return s.ConfiguredOptions().Len() == 0
+}
+
+// ConfiguredOptions returns the set of networking options that are configured (non-nil)
+// in the struct.  We only expect a single option to be configured.
+func (s *NetworkingSpec) ConfiguredOptions() sets.Set[string] {
+	options, err := reflectutils.FindSetFields(s, "classic", "kubenet", "external", "cni", "kopeio", "weave", "flannel", "calico", "canal", "kuberouter", "romana", "amazonvpc", "cilium", "lyftvpc", "gce", "kindnet")
+	if err != nil {
+		klog.Fatalf("error getting configured options: %v", err)
+	}
+	return options
 }
 
 // ClassicNetworkingSpec is the specification of classic networking mode, integrated into kubernetes.

pkg/apis/kops/validation/validation.go

@@ -1089,54 +1089,35 @@ func validateNetworking(cluster *kops.Cluster, v *kops.NetworkingSpec, fldPath *
 		allErrs = append(allErrs, validateTopology(cluster, v.Topology, fldPath.Child("topology"))...)
 	}
 
-	optionTaken := false
-
 	if v.Classic != nil {
 		allErrs = append(allErrs, field.Invalid(fldPath, "classic", "classic networking is not supported"))
 	}
 
 	if v.Kubenet != nil {
-		optionTaken = true
-
 		if cluster.Spec.IsIPv6Only() {
 			allErrs = append(allErrs, field.Forbidden(fldPath.Child("kubenet"), "Kubenet does not support IPv6"))
 		}
 	}
 
 	if v.External != nil {
-		if optionTaken {
-			allErrs = append(allErrs, field.Forbidden(fldPath.Child("external"), "only one networking option permitted"))
-		}
-
 		allErrs = append(allErrs, field.Forbidden(fldPath.Child("external"), "external is not supported for Kubernetes >= 1.26"))
-		optionTaken = true
 	}
 
 	if v.Kopeio != nil {
-		if optionTaken {
-			allErrs = append(allErrs, field.Forbidden(fldPath.Child("kopeio"), "only one networking option permitted"))
-		}
-		optionTaken = true
-
 		if cluster.Spec.IsIPv6Only() {
 			allErrs = append(allErrs, field.Forbidden(fldPath.Child("kopeio"), "Kopeio does not support IPv6"))
 		}
 	}
 
-	if v.CNI != nil && optionTaken {
-		allErrs = append(allErrs, field.Forbidden(fldPath.Child("cni"), "only one networking option permitted"))
-	}
+	// Nothing to validate for CNI
+	// if v.CNI != nil {
+	// }
 
 	if v.Weave != nil {
 		allErrs = append(allErrs, field.Forbidden(fldPath.Child("weave"), "Weave is no longer supported"))
 	}
 
 	if v.Flannel != nil {
-		if optionTaken {
-			allErrs = append(allErrs, field.Forbidden(fldPath.Child("flannel"), "only one networking option permitted"))
-		}
-		optionTaken = true
-
 		if cluster.IsKubernetesGTE("1.28") {
 			allErrs = append(allErrs, field.Forbidden(fldPath.Child("flannel"), "Flannel is not supported for Kubernetes >= 1.28"))
 		} else {
@@ -1145,20 +1126,10 @@ func validateNetworking(cluster *kops.Cluster, v *kops.NetworkingSpec, fldPath *
 	}
 
 	if v.Calico != nil {
-		if optionTaken {
-			allErrs = append(allErrs, field.Forbidden(fldPath.Child("calico"), "only one networking option permitted"))
-		}
-		optionTaken = true
-
 		allErrs = append(allErrs, validateNetworkingCalico(&cluster.Spec, v.Calico, fldPath.Child("calico"))...)
 	}
 
 	if v.Canal != nil {
-		if optionTaken {
-			allErrs = append(allErrs, field.Forbidden(fldPath.Child("canal"), "only one networking option permitted"))
-		}
-		optionTaken = true
-
 		if cluster.IsKubernetesGTE("1.28") {
 			allErrs = append(allErrs, field.Forbidden(fldPath.Child("canal"), "Canal is not supported for Kubernetes >= 1.28"))
 		} else {
@@ -1167,13 +1138,9 @@ func validateNetworking(cluster *kops.Cluster, v *kops.NetworkingSpec, fldPath *
 	}
 
 	if v.KubeRouter != nil {
-		if optionTaken {
-			allErrs = append(allErrs, field.Forbidden(fldPath.Child("kubeRouter"), "only one networking option permitted"))
-		}
 		if c.KubeProxy != nil && (c.KubeProxy.Enabled == nil || *c.KubeProxy.Enabled) {
 			allErrs = append(allErrs, field.Forbidden(fldPath.Root().Child("spec", "kubeProxy", "enabled"), "kube-router requires kubeProxy to be disabled"))
 		}
-		optionTaken = true
 
 		if cluster.Spec.IsIPv6Only() {
 			allErrs = append(allErrs, field.Forbidden(fldPath.Child("kubeRouter"), "kube-router does not support IPv6"))
@@ -1185,50 +1152,39 @@ func validateNetworking(cluster *kops.Cluster, v *kops.NetworkingSpec, fldPath *
 	}
 
 	if v.AmazonVPC != nil {
-		if optionTaken {
-			allErrs = append(allErrs, field.Forbidden(fldPath.Child("amazonVPC"), "only one networking option permitted"))
-		}
-		optionTaken = true
-
 		if cluster.GetCloudProvider() != kops.CloudProviderAWS {
 			allErrs = append(allErrs, field.Forbidden(fldPath.Child("amazonVPC"), "amazon-vpc-routed-eni networking is supported only in AWS"))
 		}
 
 		if cluster.Spec.IsIPv6Only() {
 			allErrs = append(allErrs, field.Forbidden(fldPath.Child("amazonVPC"), "amazon-vpc-routed-eni networking does not support IPv6"))
 		}
-
 	}
 
 	if v.Cilium != nil {
-		if optionTaken {
-			allErrs = append(allErrs, field.Forbidden(fldPath.Child("cilium"), "only one networking option permitted"))
-		}
-		optionTaken = true
-
 		allErrs = append(allErrs, validateNetworkingCilium(cluster, v.Cilium, fldPath.Child("cilium"))...)
 	}
 
 	if v.LyftVPC != nil {
-		allErrs = append(allErrs, field.Forbidden(fldPath.Child("lyftvp"), "support for LyftVPC has been removed"))
+		allErrs = append(allErrs, field.Forbidden(fldPath.Child("lyftvpc"), "support for LyftVPC has been removed"))
 	}
 
 	if v.GCP != nil {
-		if optionTaken {
-			allErrs = append(allErrs, field.Forbidden(fldPath.Child("gcp"), "only one networking option permitted"))
-		}
-
 		allErrs = append(allErrs, validateNetworkingGCP(cluster, v.GCP, fldPath.Child("gcp"))...)
 	}
 
 	if v.Kindnet != nil {
-		if optionTaken {
-			allErrs = append(allErrs, field.Forbidden(fldPath.Child("kindnet"), "only one networking option permitted"))
-		}
-
 		allErrs = append(allErrs, validateNetworkingKindnet(cluster, v.Kindnet, fldPath.Child("kindnet"))...)
 	}
 
+	options := v.ConfiguredOptions()
+	if options.Len() > 1 {
+		optionsList := sets.List(options)
+		for _, option := range optionsList {
+			allErrs = append(allErrs, field.Forbidden(fldPath.Child(option), fmt.Sprintf("only one networking option permitted, found %s", strings.Join(optionsList, ", "))))
+		}
+	}
+
 	return allErrs
 }
 

util/pkg/reflectutils/access.go

@@ -26,7 +26,6 @@ import (
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
-	"k8s.io/kops/pkg/apis/kops"
 )
 
 func SetString(target interface{}, targetPath string, newValue string) error {
@@ -216,14 +215,24 @@ func setType(v reflect.Value, newValue string) error {
 		newV = reflect.ValueOf(intstr.Parse(newValue))
 
 	case "kops.EnvVar":
-		name, value, found := strings.Cut(newValue, "=")
-		envVar := kops.EnvVar{
-			Name: name,
+		newV = reflect.New(v.Type()).Elem()
+
+		envVarType := newV.Type()
+
+		fdName, found := envVarType.FieldByName("Name")
+		if !found {
+			return fmt.Errorf("field Name not found in %T", newV.Interface())
 		}
-		if found {
-			envVar.Value = value
+		fdValue, found := envVarType.FieldByName("Value")
+		if !found {
+			return fmt.Errorf("field Value not found in %T", newV.Interface())
+		}
+
+		name, value, hasValue := strings.Cut(newValue, "=")
+		newV.FieldByIndex(fdName.Index).SetString(name)
+		if hasValue {
+			newV.FieldByIndex(fdValue.Index).SetString(value)
 		}
-		newV = reflect.ValueOf(envVar)
 
 	case "v1.Duration":
 		duration, err := time.ParseDuration(newValue)

util/pkg/reflectutils/set_fields.go

@@ -0,0 +1,66 @@
+/*
+Copyright 2025 The Kubernetes 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 reflectutils
+
+import (
+	"fmt"
+	"reflect"
+	"strings"
+
+	"k8s.io/apimachinery/pkg/util/sets"
+)
+
+// FindSetFields returns the set of fields that are set in the struct,
+// using the json tags as the field names.
+// It only considers the fields that are listed in the fields argument.
+func FindSetFields[T any](v *T, fields ...string) (sets.Set[string], error) {
+	val := reflect.ValueOf(v).Elem()
+	valType := val.Type()
+
+	fieldsByJsonName := make(map[string]reflect.StructField)
+
+	for i := 0; i < val.NumField(); i++ {
+		fd := valType.Field(i)
+		jsonName := fd.Tag.Get("json")
+		if jsonName == "" || jsonName == "-" {
+			continue
+		}
+		jsonName = strings.TrimSuffix(jsonName, ",omitempty")
+		fieldsByJsonName[jsonName] = fd
+	}
+
+	setFields := sets.New[string]()
+	for _, field := range fields {
+		fd, ok := fieldsByJsonName[field]
+		if !ok {
+			return nil, fmt.Errorf("field %s is not known", field)
+		}
+
+		fieldVal := val.FieldByIndex(fd.Index)
+		switch fieldVal.Kind() {
+		case reflect.Ptr:
+			if !fieldVal.IsNil() {
+				setFields.Insert(field)
+			}
+		default:
+			return nil, fmt.Errorf("field %s is not a pointer", fd.Name)
+		}
+
+	}
+
+	return setFields, nil
+}

util/pkg/reflectutils/access_test.go renamed to util/pkg/reflectutils/tests/access_test.go

@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package reflectutils
+package tests
 
 import (
 	"encoding/json"
@@ -25,6 +25,7 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/kops/pkg/apis/kops"
+	"k8s.io/kops/util/pkg/reflectutils"
 )
 
 type fakeEnum string
@@ -239,7 +240,7 @@ func TestSet(t *testing.T) {
 				t.Fatalf("failed to unmarshal input: %v", err)
 			}
 
-			if err := SetString(c, g.Path, g.Value); err != nil {
+			if err := reflectutils.SetString(c, g.Path, g.Value); err != nil {
 				t.Fatalf("error from SetString: %v", err)
 			}
 
@@ -312,7 +313,7 @@ func TestSetInvalidPath(t *testing.T) {
 				t.Fatalf("failed to unmarshal input: %v", err)
 			}
 
-			err := SetString(c, g.Path, g.Value)
+			err := reflectutils.SetString(c, g.Path, g.Value)
 			if err == nil {
 				t.Fatalf("Expected error for invalid path %s", g.Path)
 			}
@@ -402,7 +403,7 @@ func TestUnset(t *testing.T) {
 				t.Fatalf("failed to unmarshal input: %v", err)
 			}
 
-			if err := Unset(c, g.Path); err != nil {
+			if err := reflectutils.Unset(c, g.Path); err != nil {
 				t.Fatalf("error from Unset: %v", err)
 			}
 
@@ -452,7 +453,7 @@ func TestUnsetInvalidPath(t *testing.T) {
 				t.Fatalf("failed to unmarshal input: %v", err)
 			}
 
-			err := Unset(c, g.Path)
+			err := reflectutils.Unset(c, g.Path)
 			if err == nil {
 				t.Fatalf("Expected error for invalid path %s", g.Path)
 			}