Skip to content

Commit

Permalink
Implement setter by reflection
Browse files Browse the repository at this point in the history 
We add tests to ensure we have parity with our existing (hard-coded)
setter logic.
  • Loading branch information
@justinsb
justinsb committed Aug 16, 2020
1 parent fa87875 commit ffd3037
Show file tree
Hide file tree
Showing 14 changed files with 817 additions and 31 deletions.
2 changes: 1 addition & 1 deletion pkg/commands/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ go_library(
"//pkg/client/simple:go_default_library",
"//pkg/featureflag:go_default_library",
"//pkg/resources/digitalocean:go_default_library",
"//upup/pkg/fi:go_default_library",
"//upup/pkg/fi/cloudup:go_default_library",
"//upup/pkg/fi/cloudup/aliup:go_default_library",
"//upup/pkg/fi/cloudup/awstasks:go_default_library",
"//upup/pkg/fi/cloudup/awsup:go_default_library",
"//upup/pkg/fi/cloudup/gce:go_default_library",
"//upup/pkg/fi/cloudup/openstack:go_default_library",
"//util/pkg/reflectutils:go_default_library",
"//vendor/github.com/aws/aws-sdk-go/aws:go_default_library",
"//vendor/github.com/spf13/cobra:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
Expand Down
14 changes: 12 additions & 2 deletions pkg/commands/set_cluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"context"
"fmt"
"io"
"strconv"
"strings"

"github.com/spf13/cobra"
Expand All @@ -29,7 +28,7 @@ import (
"k8s.io/kops/cmd/kops/util"
api "k8s.io/kops/pkg/apis/kops"
"k8s.io/kops/pkg/featureflag"
"k8s.io/kops/upup/pkg/fi"
"k8s.io/kops/util/pkg/reflectutils"
)

type SetClusterOptions struct {
Expand Down Expand Up @@ -81,6 +80,14 @@ func SetClusterFields(fields []string, cluster *api.Cluster, instanceGroups []*a
return fmt.Errorf("unhandled field: %q", field)
}

key := kv[0]
key = strings.TrimPrefix(key, "cluster.")

if err := reflectutils.SetString(cluster, key, kv[1]); err != nil {
return fmt.Errorf("failed to set %s=%s: %v", kv[0], kv[1], err)
}

/*
// For now we have hard-code the values we want to support; we'll get test coverage and then do this properly...
switch kv[0] {
case "spec.kubelet.authorizationMode":
Expand Down Expand Up @@ -175,10 +182,12 @@ func SetClusterFields(fields []string, cluster *api.Cluster, instanceGroups []*a
default:
return fmt.Errorf("unhandled field: %q", field)
}
*/
}
return nil
}

/*
func createCiliumNetworking(cluster *api.Cluster) {
if cluster.Spec.Networking == nil {
cluster.Spec.Networking = &api.NetworkingSpec{}
Expand All @@ -187,6 +196,7 @@ func createCiliumNetworking(cluster *api.Cluster) {
cluster.Spec.Networking.Cilium = &api.CiliumNetworkingSpec{}
}
}
*/

func toEtcdProviderType(in string) (api.EtcdProviderType, error) {
s := strings.ToLower(in)
Expand Down
246 changes: 246 additions & 0 deletions pkg/commands/set_cluster_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,252 @@ func TestSetClusterFields(t *testing.T) {
},
},
},
{
Fields: []string{"spec.kubelet.authorizationMode=Webhook"},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
Kubelet: &kops.KubeletConfigSpec{
AuthorizationMode: "Webhook",
},
},
},
},
{
Fields: []string{"spec.kubelet.authenticationTokenWebhook=false"},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
Kubelet: &kops.KubeletConfigSpec{
AuthenticationTokenWebhook: fi.Bool(false),
},
},
},
},
{
Fields: []string{"spec.docker.selinuxEnabled=true"},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
Docker: &kops.DockerConfig{
SelinuxEnabled: fi.Bool(true),
},
},
},
},
{
Fields: []string{"spec.kubernetesVersion=v1.2.3"},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
KubernetesVersion: "v1.2.3",
},
},
},
{
Fields: []string{"spec.masterPublicName=api.example.com"},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
MasterPublicName: "api.example.com",
},
},
},
{
Fields: []string{"spec.kubeDNS.provider=CoreDNS"},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
KubeDNS: &kops.KubeDNSConfig{
Provider: "CoreDNS",
},
},
},
},
{
Fields: []string{
"cluster.spec.nodePortAccess=10.0.0.0/8",
"cluster.spec.nodePortAccess=192.168.0.0/16",
},
Input: kops.Cluster{},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
NodePortAccess: []string{"10.0.0.0/8", "192.168.0.0/16"},
},
},
},
{
Fields: []string{
"cluster.spec.etcdClusters[*].enableEtcdTLS=true",
},
Input: kops.Cluster{
Spec: kops.ClusterSpec{
EtcdClusters: []*kops.EtcdClusterSpec{
{Name: "one", EnableEtcdTLS: true},
{Name: "two", EnableEtcdTLS: false},
},
},
},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
EtcdClusters: []*kops.EtcdClusterSpec{
{Name: "one", EnableEtcdTLS: true},
{Name: "two", EnableEtcdTLS: true},
},
},
},
},
{
Fields: []string{
"cluster.spec.etcdClusters[*].enableTLSAuth=true",
},
Input: kops.Cluster{
Spec: kops.ClusterSpec{
EtcdClusters: []*kops.EtcdClusterSpec{
{Name: "one", EnableTLSAuth: true},
{Name: "two", EnableTLSAuth: false},
},
},
},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
EtcdClusters: []*kops.EtcdClusterSpec{
{Name: "one", EnableTLSAuth: true},
{Name: "two", EnableTLSAuth: true},
},
},
},
},
{
Fields: []string{
"cluster.spec.etcdClusters[*].version=v3.2.1",
},
Input: kops.Cluster{
Spec: kops.ClusterSpec{
EtcdClusters: []*kops.EtcdClusterSpec{
{Name: "one", Version: "v2.0.0"},
{Name: "two"},
},
},
},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
EtcdClusters: []*kops.EtcdClusterSpec{
{Name: "one", Version: "v3.2.1"},
{Name: "two", Version: "v3.2.1"},
},
},
},
},
{
Fields: []string{
"cluster.spec.etcdClusters[*].provider=Manager",
},
Input: kops.Cluster{
Spec: kops.ClusterSpec{
EtcdClusters: []*kops.EtcdClusterSpec{
{Name: "one", Provider: kops.EtcdProviderTypeLegacy},
{Name: "two"},
},
},
},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
EtcdClusters: []*kops.EtcdClusterSpec{
{Name: "one", Provider: kops.EtcdProviderTypeManager},
{Name: "two", Provider: kops.EtcdProviderTypeManager},
},
},
},
},
{
Fields: []string{
"cluster.spec.etcdClusters[*].image=etcd-manager:v1.2.3",
},
Input: kops.Cluster{
Spec: kops.ClusterSpec{
EtcdClusters: []*kops.EtcdClusterSpec{
{Name: "one", Image: "foo"},
{Name: "two"},
},
},
},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
EtcdClusters: []*kops.EtcdClusterSpec{
{Name: "one", Image: "etcd-manager:v1.2.3"},
{Name: "two", Image: "etcd-manager:v1.2.3"},
},
},
},
},
{
Fields: []string{
"cluster.spec.networking.cilium.ipam=on",
},
Input: kops.Cluster{},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
Networking: &kops.NetworkingSpec{
Cilium: &kops.CiliumNetworkingSpec{
Ipam: "on",
},
},
},
},
},
{
Fields: []string{
"cluster.spec.networking.cilium.enableNodePort=true",
},
Input: kops.Cluster{},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
Networking: &kops.NetworkingSpec{
Cilium: &kops.CiliumNetworkingSpec{
EnableNodePort: true,
},
},
},
},
},
{
Fields: []string{
"cluster.spec.networking.cilium.disableMasquerade=true",
},
Input: kops.Cluster{},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
Networking: &kops.NetworkingSpec{
Cilium: &kops.CiliumNetworkingSpec{
DisableMasquerade: true,
},
},
},
},
},
{
Fields: []string{
"cluster.spec.kubeProxy.enabled=true",
},
Input: kops.Cluster{},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
KubeProxy: &kops.KubeProxyConfig{
Enabled: fi.Bool(true),
},
},
},
},
{
Fields: []string{
"cluster.spec.networking.cilium.agentPrometheusPort=1234",
},
Input: kops.Cluster{},
Output: kops.Cluster{
Spec: kops.ClusterSpec{
Networking: &kops.NetworkingSpec{
Cilium: &kops.CiliumNetworkingSpec{
AgentPrometheusPort: 1234,
},
},
},
},
},
}

for _, g := range grid {
Expand Down
4 changes: 2 additions & 2 deletions pkg/configbuilder/buildconfigfile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import (

// BuildConfigYaml reflects the options interface and extracts the parameters for the config file
func BuildConfigYaml(options *kops.KubeSchedulerConfig, target interface{}) ([]byte, error) {
walker := func(path string, field *reflect.StructField, val reflect.Value) error {
walker := func(path *reflectutils.FieldPath, field *reflect.StructField, val reflect.Value) error {
if field == nil {
klog.V(8).Infof("ignoring non-field: %s", path)
return nil
Expand Down Expand Up @@ -78,7 +78,7 @@ func BuildConfigYaml(options *kops.KubeSchedulerConfig, target interface{}) ([]b

}

err := reflectutils.ReflectRecursive(reflect.ValueOf(options), walker)
err := reflectutils.ReflectRecursive(reflect.ValueOf(options), walker, &reflectutils.ReflectOptions{DeprecatedDoubleVisit: true})
if err != nil {
return nil, fmt.Errorf("BuildFlagsList to reflect value: %s", err)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/flagbuilder/build_flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func BuildFlags(options interface{}) (string, error) {
func BuildFlagsList(options interface{}) ([]string, error) {
var flags []string

walker := func(path string, field *reflect.StructField, val reflect.Value) error {
walker := func(path *reflectutils.FieldPath, field *reflect.StructField, val reflect.Value) error {
if field == nil {
klog.V(8).Infof("ignoring non-field: %s", path)
return nil
Expand Down Expand Up @@ -205,7 +205,7 @@ func BuildFlagsList(options interface{}) ([]string, error) {

return reflectutils.SkipReflection
}
err := reflectutils.ReflectRecursive(reflect.ValueOf(options), walker)
err := reflectutils.ReflectRecursive(reflect.ValueOf(options), walker,&reflectutils.ReflectOptions{DeprecatedDoubleVisit: true})
if err != nil {
return nil, fmt.Errorf("BuildFlagsList to reflect value: %s", err)
}
Expand Down
7 changes: 4 additions & 3 deletions upup/pkg/fi/cloudup/loader.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,12 +220,12 @@ func (l *Loader) processDeferrals() error {
for taskKey, task := range l.tasks {
taskValue := reflect.ValueOf(task)

err := reflectutils.ReflectRecursive(taskValue, func(path string, f *reflect.StructField, v reflect.Value) error {
visitor := func(path *reflectutils.FieldPath, f *reflect.StructField, v reflect.Value) error {
if reflectutils.IsPrimitiveValue(v) {
return nil
}

if path == "" {
if path.IsEmpty() {
// Don't process top-level value
return nil
}
Expand Down Expand Up @@ -292,8 +292,9 @@ func (l *Loader) processDeferrals() error {
}

return nil
})
}

err := reflectutils.ReflectRecursive(taskValue, visitor, &reflectutils.ReflectOptions{DeprecatedDoubleVisit: true})
if err != nil {
return fmt.Errorf("unexpected error resolving task %q: %v", taskKey, err)
}
Expand Down
Loading

0 comments on commit ffd3037

Please sign in to comment.