Skip to content

Commit

Permalink
Refactor validation logic around checking for multiple options
Browse files Browse the repository at this point in the history
We also have to move some of the EnvVar handling around to avoid a circular reference.
  • Loading branch information
justinsb committed Jan 8, 2025
1 parent 2db9dbc commit 33a1c37
Show file tree
Hide file tree
Showing 6 changed files with 129 additions and 71 deletions.
17 changes: 16 additions & 1 deletion pkg/apis/kops/networking.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
17 changes: 14 additions & 3 deletions pkg/apis/kops/v1alpha2/networking.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
66 changes: 11 additions & 55 deletions pkg/apis/kops/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
if v.CNI != nil {
// Nothing to validate
}

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 {
Expand All @@ -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 {
Expand All @@ -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"))
Expand All @@ -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
}

Expand Down
23 changes: 16 additions & 7 deletions util/pkg/reflectutils/access.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down
66 changes: 66 additions & 0 deletions util/pkg/reflectutils/set_fields.go
Original file line number Diff line number Diff line change
@@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}
Expand Down

0 comments on commit 33a1c37

Please sign in to comment.