diff --git a/kyaml/setters2/settersutil/substitutioncreator.go b/kyaml/setters2/settersutil/substitutioncreator.go index 4acef990f..fc187c63e 100644 --- a/kyaml/setters2/settersutil/substitutioncreator.go +++ b/kyaml/setters2/settersutil/substitutioncreator.go @@ -4,7 +4,6 @@ package settersutil import ( - "math" "strings" "sigs.k8s.io/kustomize/kyaml/errors" @@ -138,17 +137,23 @@ func (c SubstitutionCreator) GetValuesForMarkers() (map[string]string, error) { if marker, ok := indices[patternInd]; ok { // increment the patternInd to end of marker. This helps us to extract the substring before next marker. patternInd += len(marker) - value := c.extractValueForMarker(&fvInd, fv, patternInd, indices) + var value string + if value, fvInd, err = c.extractValueForMarker(fvInd, fv, patternInd, indices); err != nil { + return nil, err + } // if marker is repeated in the pattern, make sure that the corresponding values // are same and throw error if not. if prevValue, ok := m[marker]; ok && prevValue != value { - return nil, errors.Errorf("same marker is found to have different values in field value") + return nil, errors.Errorf( + "marker %s is found to have different values %s and %s", marker, prevValue, value) } m[marker] = value } else { // Ex: fv = "samething/ubuntu:0.1.0" pattern = "something/IMAGE:VERSION". Error out at 'a' in fv. if fv[fvInd] != pattern[patternInd] { - return nil, errors.Errorf("unable to derive values for markers, create setters for all markers and then try again") + return nil, errors.Errorf( + "unable to derive values for markers, " + + "create setters for all markers and then try again") } fvInd++ patternInd++ @@ -156,7 +161,9 @@ func (c SubstitutionCreator) GetValuesForMarkers() (map[string]string, error) { } // check if both strings are completely visited or throw error if fvInd < len(fv) || patternInd < len(pattern) { - return nil, errors.Errorf("unable to derive values for markers, create setters for all markers and then try again") + return nil, errors.Errorf( + "unable to derive values for markers, " + + "create setters for all markers and then try again") } return m, nil } @@ -164,48 +171,83 @@ func (c SubstitutionCreator) GetValuesForMarkers() (map[string]string, error) { // GetStartIndices returns the start indices of all the markers in the pattern func (c SubstitutionCreator) GetStartIndices() (map[int]string, error) { indices := make(map[int]string) - p := c.Pattern for _, value := range c.Values { - m := value.Marker found := false - for i := range p { - if strings.HasPrefix(p[i:], m) { - indices[i] = m + for i := range c.Pattern { + if strings.HasPrefix(c.Pattern[i:], value.Marker) { + indices[i] = value.Marker found = true } } if !found { - return nil, errors.Errorf("Unable to find marker " + m + " in the pattern") + return nil, errors.Errorf("unable to find marker " + value.Marker + " in the pattern") } } + if err := validateMarkers(indices); err != nil { + return nil, err + } return indices, nil } -func (c SubstitutionCreator) extractValueForMarker(fvInd *int, fv string, patternInd int, indices map[int]string) string { - value := "" - lm := lenTillNextMarker(indices, patternInd) - - // keep appending chars to value till we find the substring between 2 markers in fv - // In example fv = "something/ubuntu:0.1.0", pattern = "something/IMAGE:VERSION", - // proceed till we find substring ":" in fv which makes value = ubuntu for marker IMAGE - for *fvInd < len(fv) && (patternInd == len(c.Pattern) || - fv[*fvInd:min(len(fv), *fvInd+lm)] != c.Pattern[patternInd:min(patternInd+lm, len(c.Pattern))]) { - value += string(fv[*fvInd]) - *fvInd++ - } - return value -} - -// lenToNextMarker takes in the indices map, an index and returns the distance to -// next marker start index in indices map -func lenTillNextMarker(m map[int]string, j int) int { - res := math.MaxInt32 - for k := range m { - if k > j { - res = min(k-j, res) +// validateMarkers takes the indices map, checks if any of 2 markers not have delimiters, +// checks if any marker is substring of other and returns error +func validateMarkers(indices map[int]string) error { + for k1, v1 := range indices { + for k2, v2 := range indices { + if k1 != k2 && k1+len(v1) == k2 { + return errors.Errorf( + "markers %s and %s are found to have no delimiters between them,"+ + " pre-create setters and try again", v1, v2) + } + if v1 != v2 && strings.Contains(v1, v2) { + return errors.Errorf( + "markers %s is substring of %s,"+ + " no marker should be substring of other", v2, v1) + } } } - return res + return nil +} + +// extractValueForMarker returns the value string for a marker and the incremented index +func (c SubstitutionCreator) extractValueForMarker(fvInd int, fv string, patternInd int, indices map[int]string) (string, int, error) { + nonMarkerStr := strTillNextMarker(indices, patternInd, c.Pattern) + + // return the remaining string of fv till end if patternInd is at end of pattern + if patternInd == len(c.Pattern) { + return fv[fvInd:], len(fv), nil + } + + // split remaining fv starting from fvInd with the non marker substring delimiter and get the first value + // In example fv = "something/ubuntu::0.1.0", pattern = "something/IMAGE::VERSION", + // split with "::" delimiter in fv which gives markerValue = ubuntu for marker IMAGE + // increment fvInd by length of extracted marker value and return fvInd + if markerValues := strings.Split(fv[fvInd:], nonMarkerStr); len(markerValues) > 0 { + return markerValues[0], fvInd + len(markerValues[0]), nil + } + + return "", -1, errors.Errorf( + "unable to derive values for markers," + + " create setters for all markers and then try again") +} + +// substrOfLen takes a string, start index and length and returns substring of given length +// or till end of string +func substrOfLen(str string, startInd int, length int) string { + return str[startInd:min(len(str), startInd+length)] +} + +// strTillNextMarker takes in the indices map, a start index and returns the substring till +// start of next marker +func strTillNextMarker(indices map[int]string, startInd int, pattern string) string { + // initialize with max value which is length of pattern + nextMarkerStartInd := len(pattern) + for ind := range indices { + if ind > startInd { + nextMarkerStartInd = min(ind-startInd, nextMarkerStartInd) + } + } + return substrOfLen(pattern, startInd, nextMarkerStartInd) } func min(a int, b int) int { diff --git a/kyaml/setters2/settersutil/substitutioncreator_test.go b/kyaml/setters2/settersutil/substitutioncreator_test.go index c39956424..9c6b2951b 100644 --- a/kyaml/setters2/settersutil/substitutioncreator_test.go +++ b/kyaml/setters2/settersutil/substitutioncreator_test.go @@ -7,110 +7,105 @@ import ( "testing" "github.com/stretchr/testify/assert" + "sigs.k8s.io/kustomize/kyaml/errors" "sigs.k8s.io/kustomize/kyaml/setters2" ) -func TestGetValuesForMarkersPositive(t *testing.T) { - c := SubstitutionCreator{ - Pattern: "something/IMAGE::VERSION/otherthing/IMAGE::VERSION/", - Values: Values(), - FieldValue: "something/nginx::0.1.0/otherthing/nginx::0.1.0/", +func TestGetValuesForMarkers(t *testing.T) { + var tests = []struct { + name string + pattern string + fieldValue string + markers []string + expectedError error + expectedOutput map[string]string + }{ + { + name: "positive example", + markers: []string{"IMAGE", "VERSION"}, + pattern: "something/IMAGE::VERSION/otherthing/IMAGE::VERSION/", + fieldValue: "something/nginx::0.1.0/otherthing/nginx::0.1.0/", + expectedOutput: map[string]string{"IMAGE": "nginx", "VERSION": "0.1.0"}, + }, + { + name: "marker with different values", + markers: []string{"IMAGE", "VERSION"}, + pattern: "something/IMAGE:VERSION/IMAGE", + fieldValue: "something/nginx:0.1.0/ubuntu", + expectedError: errors.Errorf("marker IMAGE is found to have different values nginx and ubuntu"), + }, + { + name: "unmatched pattern", + markers: []string{"IMAGE", "VERSION"}, + pattern: "something/IMAGE:VERSION", + fieldValue: "otherthing/nginx:0.1.0", + expectedError: errors.Errorf("unable to derive values for markers"), + }, + { + name: "unmatched pattern at the end", + markers: []string{"IMAGE", "VERSION"}, + pattern: "something/IMAGE:VERSION/abc", + fieldValue: "something/nginx:0.1.0/abcd", + expectedError: errors.Errorf("unable to derive values for markers"), + }, + { + name: "substring markers", + markers: []string{"IMAGE", "VERSION", "MAGE"}, + pattern: "something/IMAGE:VERSION/abc/MAGE", + fieldValue: "something/nginx:0.1.0/abc/ubuntu", + expectedError: errors.Errorf("no marker should be substring of other"), + }, + { + name: "markers with no delimiters", + markers: []string{"IMAGE", "VERSION"}, + pattern: "something/IMAGEVERSION/", + fieldValue: "something/nginx0.1.0/", + expectedError: errors.Errorf("no delimiters between them"), + }, + { + name: "unmatched delimiter", + markers: []string{"IMAGE", "VERSION"}, + pattern: "something/IMAGE:^VERSION/otherthing/IMAGE::VERSION/", + fieldValue: "something/nginx::0.1.0/otherthing/nginx::0.1.0/", + expectedError: errors.Errorf("unable to derive values for markers"), + }, } + for i := range tests { + test := tests[i] + t.Run(test.name, func(t *testing.T) { + values := []setters2.Value{} + for _, marker := range test.markers { + value := setters2.Value{ + Marker: marker, + } + values = append(values, value) + } - m, err := c.GetValuesForMarkers() + sc := SubstitutionCreator{ + Pattern: test.pattern, + Values: values, + FieldValue: test.fieldValue, + } - if !assert.NoError(t, err) { - t.FailNow() - } + m, err := sc.GetValuesForMarkers() - assert.Equal(t, m["IMAGE"], "nginx") - assert.Equal(t, m["VERSION"], "0.1.0") -} - -func TestGetValuesForMarkersDiffMarkerValues(t *testing.T) { - c := SubstitutionCreator{ - Pattern: "something/IMAGE:VERSION/IMAGE", - Values: Values(), - FieldValue: "something/nginx:0.1.0/ubuntu", - } - - _, err := c.GetValuesForMarkers() - - if !assert.Error(t, err) { - t.FailNow() - } - - if !assert.Equal(t, err.Error(), "same marker is found to have different values in field value") { - t.FailNow() + if test.expectedError == nil { + // fail if expectedError is nil but actual error is not + if !assert.NoError(t, err) { + t.FailNow() + } + // check if all the expected markers and values are present in actual map + for k, v := range test.expectedOutput { + if val, ok := m[k]; ok { + assert.Equal(t, v, val) + } else { + t.FailNow() + } + } + } else { + //if expectedError is not nil, check for correctness of error message + assert.Contains(t, err.Error(), test.expectedError.Error()) + } + }) } } - -func TestGetValuesForMarkersNoMatch(t *testing.T) { - c := SubstitutionCreator{ - Pattern: "something/IMAGE:VERSION", - Values: Values(), - FieldValue: "otherthing/nginx:0.1.0", - } - - _, err := c.GetValuesForMarkers() - - if !assert.Error(t, err) { - t.FailNow() - } - - if !assert.Equal(t, err.Error(), "unable to derive values for markers, create setters for all markers and then try again") { - t.FailNow() - } -} - -func TestGetValuesForMarkersNoMatch2(t *testing.T) { - c := SubstitutionCreator{ - Pattern: "something/IMAGE:VERSION/abc", - Values: Values(), - FieldValue: "something/nginx:0.1.0/abcd", - } - - _, err := c.GetValuesForMarkers() - - if !assert.Error(t, err) { - t.FailNow() - } - - if !assert.Equal(t, err.Error(), "unable to derive values for markers, create setters for all markers and then try again") { - t.FailNow() - } -} - -func TestGetValuesForMarkersSubStngMarkers(t *testing.T) { - value3 := setters2.Value{ - Marker: "MAGE", - } - - c := SubstitutionCreator{ - Pattern: "something/IMAGE:VERSION/abc/MAGE", - Values: append(Values(), value3), - FieldValue: "something/nginx:0.1.0/abc/ubuntu", - } - - m, err := c.GetValuesForMarkers() - - if !assert.NoError(t, err) { - t.FailNow() - } - - assert.Equal(t, m["IMAGE"], "nginx") - assert.Equal(t, m["VERSION"], "0.1.0") - assert.Equal(t, m["MAGE"], "ubuntu") -} - -func Values() []setters2.Value { - value1 := setters2.Value{ - Marker: "IMAGE", - } - - value2 := setters2.Value{ - Marker: "VERSION", - } - - return []setters2.Value{value1, value2} -}