Merge pull request #4296 from deads2k/deads-allow-dots

pkg/kubectl/cmd/config/navigation_step_parser.go

+/*
+Copyright 2014 Google Inc. All rights reserved.
+
+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 config
+
+import (
+	"fmt"
+	"reflect"
+	"strings"
+
+	clientcmdapi "github.com/GoogleCloudPlatform/kubernetes/pkg/client/clientcmd/api"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
+)
+
+type navigationSteps struct {
+	steps            []navigationStep
+	currentStepIndex int
+}
+
+type navigationStep struct {
+	stepValue string
+	stepType  reflect.Type
+}
+
+func newNavigationSteps(path string) (*navigationSteps, error) {
+	steps := []navigationStep{}
+	individualParts := strings.Split(path, ".")
+
+	currType := reflect.TypeOf(clientcmdapi.Config{})
+	currPartIndex := 0
+	for currPartIndex < len(individualParts) {
+		switch currType.Kind() {
+		case reflect.Map:
+			// if we're in a map, we need to locate a name.  That name may contain dots, so we need to know what tokens are legal for the map's value type
+			// for example, we could have a set request like: `set clusters.10.10.12.56.insecure-skip-tls-verify true`.  We enter this case with
+			// steps representing 10, 10, 12, 56, insecure-skip-tls-verify.  The name is "10.10.12.56", so we want to collect all those parts together and
+			// store them as a single step.  In order to do that, we need to determine what set of tokens is a legal step AFTER the name of the map key
+			// This set of reflective code pulls the type of the map values, uses that type to look up the set of legal tags.  Those legal tags are used to
+			// walk the list of remaining parts until we find a match to a legal tag or the end of the string.  That name is used to burn all the used parts.
+			mapValueType := currType.Elem()
+			mapValueOptions, err := getPotentialTypeValues(mapValueType)
+			if err != nil {
+				return nil, err
+			}
+			nextPart := findNameStep(individualParts[currPartIndex:], util.KeySet(reflect.ValueOf(mapValueOptions)))
+
+			steps = append(steps, navigationStep{nextPart, mapValueType})
+			currPartIndex += len(strings.Split(nextPart, "."))
+			currType = mapValueType
+
+		case reflect.Struct:
+			nextPart := individualParts[currPartIndex]
+
+			options, err := getPotentialTypeValues(currType)
+			if err != nil {
+				return nil, err
+			}
+			fieldType, exists := options[nextPart]
+			if !exists {
+				return nil, fmt.Errorf("unable to parse %v after %v at %v", path, steps, currType)
+			}
+
+			steps = append(steps, navigationStep{nextPart, fieldType})
+			currPartIndex += len(strings.Split(nextPart, "."))
+			currType = fieldType
+		}
+	}
+
+	return &navigationSteps{steps, 0}, nil
+}
+
+func (s *navigationSteps) pop() navigationStep {
+	if s.moreStepsRemaining() {
+		s.currentStepIndex++
+		return s.steps[s.currentStepIndex-1]
+	}
+	return navigationStep{}
+}
+
+func (s *navigationSteps) peek() navigationStep {
+	if s.moreStepsRemaining() {
+		return s.steps[s.currentStepIndex]
+	}
+	return navigationStep{}
+}
+
+func (s *navigationSteps) moreStepsRemaining() bool {
+	return len(s.steps) > s.currentStepIndex
+}
+
+// findNameStep takes the list of parts and a set of valid tags that can be used after the name.  It then walks the list of parts
+// until it find a valid "next" tag or until it reaches the end of the parts and then builds the name back up out of the individual parts
+func findNameStep(parts []string, typeOptions util.StringSet) string {
+	if len(parts) == 0 {
+		return ""
+	}
+
+	numberOfPartsInStep := findKnownValue(parts[1:], typeOptions) + 1
+	// if we didn't find a known value, then the entire thing must be a name
+	if numberOfPartsInStep == 0 {
+		numberOfPartsInStep = len(parts)
+	}
+	nextParts := parts[0:numberOfPartsInStep]
+
+	return strings.Join(nextParts, ".")
+}
+
+// getPotentialTypeValues takes a type and looks up the tags used to represent its fields when serialized.
+func getPotentialTypeValues(typeValue reflect.Type) (map[string]reflect.Type, error) {
+	if typeValue.Kind() != reflect.Struct {
+		return nil, fmt.Errorf("%v is not of type struct", typeValue)
+	}
+
+	ret := make(map[string]reflect.Type)
+
+	for fieldIndex := 0; fieldIndex < typeValue.NumField(); fieldIndex++ {
+		fieldType := typeValue.Field(fieldIndex)
+		yamlTag := fieldType.Tag.Get("json")
+		yamlTagName := strings.Split(yamlTag, ",")[0]
+
+		ret[yamlTagName] = fieldType.Type
+	}
+
+	return ret, nil
+}
+
+func findKnownValue(parts []string, valueOptions util.StringSet) int {
+	for i := range parts {
+		if valueOptions.Has(parts[i]) {
+			return i
+		}
+	}
+
+	return -1
+}

pkg/kubectl/cmd/config/navigation_step_parser_test.go

+/*
+Copyright 2014 Google Inc. All rights reserved.
+
+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 config
+
+import (
+	"reflect"
+	"strings"
+	"testing"
+
+	clientcmdapi "github.com/GoogleCloudPlatform/kubernetes/pkg/client/clientcmd/api"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
+)
+
+type stepParserTest struct {
+	path                    string
+	expectedNavigationSteps navigationSteps
+	expectedError           string
+}
+
+func TestParseWithDots(t *testing.T) {
+	test := stepParserTest{
+		path: "clusters.my.dot.delimited.name.server",
+		expectedNavigationSteps: navigationSteps{
+			steps: []navigationStep{
+				{"clusters", reflect.TypeOf(make(map[string]clientcmdapi.Cluster))},
+				{"my.dot.delimited.name", reflect.TypeOf(clientcmdapi.Cluster{})},
+				{"server", reflect.TypeOf("")},
+			},
+		},
+	}
+
+	test.run(t)
+}
+
+func TestParseWithDotsEndingWithName(t *testing.T) {
+	test := stepParserTest{
+		path: "contexts.10.12.12.12",
+		expectedNavigationSteps: navigationSteps{
+			steps: []navigationStep{
+				{"contexts", reflect.TypeOf(make(map[string]clientcmdapi.Context))},
+				{"10.12.12.12", reflect.TypeOf(clientcmdapi.Context{})},
+			},
+		},
+	}
+
+	test.run(t)
+}
+
+func TestParseWithBadValue(t *testing.T) {
+	test := stepParserTest{
+		path: "user.bad",
+		expectedNavigationSteps: navigationSteps{
+			steps: []navigationStep{},
+		},
+		expectedError: "unable to parse user.bad after [] at api.Config",
+	}
+
+	test.run(t)
+}
+
+func (test stepParserTest) run(t *testing.T) {
+	actualSteps, err := newNavigationSteps(test.path)
+	if len(test.expectedError) != 0 {
+		if err == nil {
+			t.Errorf("Did not get %v", test.expectedError)
+		} else {
+			if !strings.Contains(err.Error(), test.expectedError) {
+				t.Errorf("Expected %v, but got %v", test.expectedError, err)
+			}
+		}
+		return
+	}
+
+	if err != nil {
+		t.Errorf("Unexpected error: %v", err)
+	}
+
+	if !reflect.DeepEqual(test.expectedNavigationSteps, *actualSteps) {
+		t.Errorf("diff: %v", util.ObjectDiff(test.expectedNavigationSteps, *actualSteps))
+	}
+}

pkg/kubectl/cmd/config/set.go

@@ -33,8 +33,6 @@ const (
 	additionStepRequiredUnlessUnsettingError = "Must have additional steps after %v unless you are unsetting it"
 )
 
-type navigationSteps []string
-
 type setOptions struct {
 	pathOptions   *pathOptions
 	propertyName  string
@@ -83,8 +81,11 @@ func (o setOptions) run() error {
 		return errors.New("cannot set property without using a specific file")
 	}
 
-	parts := strings.Split(o.propertyName, ".")
-	err = modifyConfig(reflect.ValueOf(config), parts, o.propertyValue, false)
+	steps, err := newNavigationSteps(o.propertyName)
+	if err != nil {
+		return err
+	}
+	err = modifyConfig(reflect.ValueOf(config), steps, o.propertyValue, false)
 	if err != nil {
 		return err
 	}
@@ -121,28 +122,8 @@ func (o setOptions) validate() error {
 	return nil
 }
 
-// moreStepsRemaining just makes code read cleaner
-func moreStepsRemaining(remainder []string) bool {
-	return len(remainder) != 0
-}
-
-func (s navigationSteps) nextSteps() navigationSteps {
-	if len(s) < 2 {
-		return make([]string, 0, 0)
-	} else {
-		return s[1:]
-	}
-}
-func (s navigationSteps) moreStepsRemaining() bool {
-	return len(s) != 0
-}
-func (s navigationSteps) nextStep() string {
-	return s[0]
-}
-
-func modifyConfig(curr reflect.Value, steps navigationSteps, propertyValue string, unset bool) error {
-	shouldUnsetNextField := !steps.nextSteps().moreStepsRemaining() && unset
-	shouldSetThisField := !steps.moreStepsRemaining() && !unset
+func modifyConfig(curr reflect.Value, steps *navigationSteps, propertyValue string, unset bool) error {
+	currStep := steps.pop()
 
 	actualCurrValue := curr
 	if curr.Kind() == reflect.Ptr {
@@ -151,14 +132,14 @@ func modifyConfig(curr reflect.Value, steps navigationSteps, propertyValue strin
 
 	switch actualCurrValue.Kind() {
 	case reflect.Map:
-		if shouldSetThisField {
+		if !steps.moreStepsRemaining() && !unset {
 			return fmt.Errorf("Can't set a map to a value: %v", actualCurrValue)
 		}
 
-		mapKey := reflect.ValueOf(steps.nextStep())
+		mapKey := reflect.ValueOf(currStep.stepValue)
 		mapValueType := curr.Type().Elem().Elem()
 
-		if shouldUnsetNextField {
+		if !steps.moreStepsRemaining() && unset {
 			actualCurrValue.SetMapIndex(mapKey, reflect.Value{})
 			return nil
 		}
@@ -181,7 +162,7 @@ func modifyConfig(curr reflect.Value, steps navigationSteps, propertyValue strin
 		if modifiableMapValue.Kind() == reflect.Struct {
 			modifiableMapValue = modifiableMapValue.Addr()
 		}
-		err := modifyConfig(modifiableMapValue, steps.nextSteps(), propertyValue, unset)
+		err := modifyConfig(modifiableMapValue, steps, propertyValue, unset)
 		if err != nil {
 			return err
 		}
@@ -208,40 +189,36 @@ func modifyConfig(curr reflect.Value, steps navigationSteps, propertyValue strin
 		return nil
 
 	case reflect.Struct:
-		if !steps.moreStepsRemaining() {
-			return fmt.Errorf("Can't set a struct to a value: %v", actualCurrValue)
-		}
-
 		for fieldIndex := 0; fieldIndex < actualCurrValue.NumField(); fieldIndex++ {
 			currFieldValue := actualCurrValue.Field(fieldIndex)
 			currFieldType := actualCurrValue.Type().Field(fieldIndex)
 			currYamlTag := currFieldType.Tag.Get("json")
 			currFieldTypeYamlName := strings.Split(currYamlTag, ",")[0]
 
-			if currFieldTypeYamlName == steps.nextStep() {
+			if currFieldTypeYamlName == currStep.stepValue {
 				thisMapHasNoValue := (currFieldValue.Kind() == reflect.Map && currFieldValue.IsNil())
 
 				if thisMapHasNoValue {
 					newValue := reflect.MakeMap(currFieldValue.Type())
 					currFieldValue.Set(newValue)
 
-					if shouldUnsetNextField {
+					if !steps.moreStepsRemaining() && unset {
 						return nil
 					}
 				}
 
-				if shouldUnsetNextField {
+				if !steps.moreStepsRemaining() && unset {
 					// if we're supposed to unset the value or if the value is a map that doesn't exist, create a new value and overwrite
 					newValue := reflect.New(currFieldValue.Type()).Elem()
 					currFieldValue.Set(newValue)
 					return nil
 				}
 
-				return modifyConfig(currFieldValue.Addr(), steps.nextSteps(), propertyValue, unset)
+				return modifyConfig(currFieldValue.Addr(), steps, propertyValue, unset)
 			}
 		}
 
-		return fmt.Errorf("Unable to locate path %v under %v", steps, actualCurrValue)
+		return fmt.Errorf("Unable to locate path %#v under %v", currStep, actualCurrValue)
 
 	}
 

pkg/kubectl/cmd/config/unset.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"io"
 	"reflect"
-	"strings"
 
 	"github.com/spf13/cobra"
 
@@ -73,8 +72,11 @@ func (o unsetOptions) run() error {
 		return errors.New("cannot set property without using a specific file")
 	}
 
-	parts := strings.Split(o.propertyName, ".")
-	err = modifyConfig(reflect.ValueOf(config), parts, "", true)
+	steps, err := newNavigationSteps(o.propertyName)
+	if err != nil {
+		return err
+	}
+	err = modifyConfig(reflect.ValueOf(config), steps, "", true)
 	if err != nil {
 		return err
 	}

pkg/util/set.go

@@ -17,6 +17,7 @@ limitations under the License.
 package util
 
 import (
+	"reflect"
 	"sort"
 )
 
@@ -32,6 +33,18 @@ func NewStringSet(items ...string) StringSet {
 	return ss
 }
 
+// KeySet creates a StringSet from a keys of a map[string](? extends interface{}).  Since you can't describe that map type in the Go type system
+// the reflected value is required.
+func KeySet(theMap reflect.Value) StringSet {
+	ret := StringSet{}
+
+	for _, keyValue := range theMap.MapKeys() {
+		ret.Insert(keyValue.String())
+	}
+
+	return ret
+}
+
 // Insert adds items to the set.
 func (s StringSet) Insert(items ...string) {
 	for _, item := range items {