codegen - support multiple annotation keys (#2350)

* allow multiple values for comment tag keys

this will allow us to support multiple annotation keys

* add OR filter chain

* update codegen to support multiple annotation keys

* Preserve previous code & formatting if the number of annotation keys is 1

- preserve the order of the annotations vs. sorting it
- deprecate and don't remove ClassAnnotationKey to allow migration to happen smoothly

* fix default value for krshape

* include clarifying comment

codegen/cmd/injection-gen/generators/comment_parser.go

@@ -19,55 +19,74 @@ import "strings"
 
 // Adapted from the k8s.io comment parser https://github.com/kubernetes/gengo/blob/master/types/comments.go
 
+// CommentsTags maps marker prefixes to a set of tags containing keys and values
+type CommentTags map[string]CommentTag
+
+// CommentTags maps keys to a list of values
+type CommentTag map[string][]string
+
 // ExtractCommentTags parses comments for lines of the form:
 //
-//   'marker' + ':' "key=value,key2=value2".
+//   "marker" + "prefix" + ':' + "key=value,key2=value2".
 //
-// Values are optional; empty map is the default.  A tag can be specified more than
+//  In the following example the marker is '+' and the prefix is 'foo':
+//   +foo:key=value1,key2=value2,key=value3
+//
+// Values are optional; empty map is the default. A tag can be specified more than
 // one time and all values are returned.  If the resulting map has an entry for
 // a key, the value (a slice) is guaranteed to have at least 1 element.
 //
 // Example: if you pass "+" for 'marker', and the following lines are in
 // the comments:
-//   +foo:key=value1,key2=value2
+//   +foo:key=value1,key2=value2,key=value3
 //   +bar
 //
 // Then this function will return:
-//   map[string]map[string]string{"foo":{"key":value1","key2":"value2"}, "bar": nil}
+//   map[string]map[string]string{
+//     "foo":{
+//      "key":  []string{"value1", "value3"},
+//      "key2": []string{"value2"}
+//     },
+//     "bar": {},
+//  }
 //
 // Users are not expected to repeat values.
-func ExtractCommentTags(marker string, lines []string) map[string]map[string]string {
-	out := map[string]map[string]string{}
+func ExtractCommentTags(marker string, lines []string) CommentTags {
+	out := CommentTags{}
+
 	for _, line := range lines {
 		line = strings.TrimSpace(line)
 		if len(line) == 0 || !strings.HasPrefix(line, marker) {
 			continue
 		}
 
 		options := strings.SplitN(line[len(marker):], ":", 2)
+		prefix := options[0]
+
 		if len(options) == 2 {
 			vals := strings.Split(options[1], ",")
 
-			opts := out[options[0]]
+			opts := out[prefix]
 			if opts == nil {
-				opts = make(map[string]string, len(vals))
+				opts = make(CommentTag, len(vals))
+				out[prefix] = opts
 			}
 
 			for _, pair := range vals {
-				if kv := strings.SplitN(pair, "=", 2); len(kv) == 2 {
-					opts[kv[0]] = kv[1]
-				} else if kv[0] != "" {
-					opts[kv[0]] = ""
+				kv := strings.SplitN(pair, "=", 2)
+				if len(kv) == 1 && kv[0] == "" {
+					continue
+				}
+				if _, ok := opts[kv[0]]; !ok {
+					opts[kv[0]] = []string{}
+				}
+				if len(kv) == 2 {
+					opts[kv[0]] = append(opts[kv[0]], kv[1])
 				}
-			}
-			if len(opts) == 0 {
-				out[options[0]] = nil
-			} else {
-				out[options[0]] = opts
 			}
 		} else if len(options) == 1 && options[0] != "" {
-			if _, has := out[options[0]]; !has {
-				out[options[0]] = nil
+			if _, has := out[prefix]; !has {
+				out[prefix] = CommentTag{}
 			}
 		}
 	}

codegen/cmd/injection-gen/generators/comment_parser_test.go

@@ -15,7 +15,11 @@ limitations under the License.
 */
 package generators
 
-import "testing"
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+)
 
 func TestParseComments(t *testing.T) {
 	comment := []string{
@@ -26,28 +30,31 @@ func TestParseComments(t *testing.T) {
 		"+with:option",
 		"+pair:key=value",
 		"+manypairs:key1=value1,key2=value2",
+		"+duplicate:key1=value1,key1=value2",
 	}
 
 	extracted := ExtractCommentTags("+", comment)
 
-	if val, ok := extracted["foo"]; !ok || val != nil {
-		t.Errorf("Failed to extract single key got=%t,%v want=true,nil", ok, val)
-	}
-
-	if val, ok := extracted["bar"]; !ok || val != nil {
-		t.Errorf("Failed to extract single key got=%t,%v want=true,nil", ok, val)
+	expected := CommentTags{
+		"foo": {},
+		"bar": {},
+		"with": {
+			"option": {},
+		},
+		"pair": {
+			"key": []string{"value"},
+		},
+		"manypairs": {
+			"key1": []string{"value1"},
+			"key2": []string{"value2"},
+		},
+		"duplicate": {
+			"key1": []string{"value1", "value2"},
+		},
 	}
 
-	if val, ok := extracted["with"]; !ok || val["option"] != "" {
-		t.Errorf(`Failed to extract single key got=%t,%v want=true,{"option":""}`, ok, val)
-	}
-
-	if val, ok := extracted["pair"]; !ok || val["key"] != "value" {
-		t.Errorf(`Failed to extract single key got=%t,%v want=true,{"key":"value"}`, ok, val)
-	}
-
-	if val, ok := extracted["manypairs"]; !ok || val["key1"] != "value1" || val["key2"] != "value2" {
-		t.Errorf(`Failed to extract single key got=%t,%v want=true,{"key":"value"}`, ok, val)
+	if diff := cmp.Diff(expected, extracted); diff != "" {
+		t.Error("diff (-want, +got): ", diff)
 	}
 }
 
@@ -61,24 +68,25 @@ func TestMergeDuplicates(t *testing.T) {
 		"+bar",
 		"+manypairs:key1=value1",
 		"+manypairs:key2=value2",
+		"+manypairs:key1=value3",
 		"+oops:,,,",
 	}
 
 	extracted := ExtractCommentTags("+", comment)
 
-	if val, ok := extracted["foo"]; !ok || val != nil {
-		t.Errorf("Failed to extract single key got=%t,%v want=true,nil", ok, val)
-	}
-
-	if val, ok := extracted["bar"]; !ok || val["key"] != "value" {
-		t.Errorf(`Failed to extract single key got=%t,%v want=true,{"key":"value"}`, ok, val)
-	}
-
-	if val, ok := extracted["manypairs"]; !ok || val["key1"] != "value1" || val["key2"] != "value2" {
-		t.Errorf(`Failed to extract single key got=%t,%v want=true,{"key":"value"}`, ok, val)
+	expected := CommentTags{
+		"foo": {},
+		"bar": {
+			"key": []string{"value"},
+		},
+		"manypairs": {
+			"key1": []string{"value1", "value3"},
+			"key2": []string{"value2"},
+		},
+		"oops": {},
 	}
 
-	if val, ok := extracted["oops"]; !ok || val != nil {
-		t.Errorf(`Failed to extract single key got=%t,%v want=true,{"oops":nil}`, ok, val)
+	if diff := cmp.Diff(expected, extracted); diff != "" {
+		t.Error("diff (-want, +got): ", diff)
 	}
 }

codegen/cmd/injection-gen/generators/packages.go

@@ -193,29 +193,36 @@ func MustParseClientGenTags(lines []string) Tags {
 	return ret
 }
 
-func extractCommentTags(t *types.Type) map[string]map[string]string {
+func extractCommentTags(t *types.Type) CommentTags {
 	comments := append(append([]string{}, t.SecondClosestCommentLines...), t.CommentLines...)
 	return ExtractCommentTags("+", comments)
 }
 
-func extractReconcilerClassTag(tags map[string]map[string]string) (string, bool) {
+func extractReconcilerClassesTag(tags CommentTags) ([]string, bool) {
 	vals, ok := tags["genreconciler"]
 	if !ok {
-		return "", false
+		return nil, false
 	}
-	classname, has := vals["class"]
-	return classname, has
+	classnames, has := vals["class"]
+	if has && len(classnames) == 0 {
+		return nil, false
+	}
+	return classnames, has
 }
 
-func isKRShaped(tags map[string]map[string]string) bool {
+func isKRShaped(tags CommentTags) bool {
 	vals, has := tags["genreconciler"]
 	if !has {
 		return false
 	}
-	return vals["krshapedlogic"] != "false"
+	stringVals, has := vals["krshapedlogic"]
+	if !has || len(vals) == 0 {
+		return true // Default is true
+	}
+	return stringVals[0] != "false"
 }
 
-func isNonNamespaced(tags map[string]map[string]string) bool {
+func isNonNamespaced(tags CommentTags) bool {
 	vals, has := tags["genclient"]
 	if !has {
 		return false
@@ -224,7 +231,7 @@ func isNonNamespaced(tags map[string]map[string]string) bool {
 	return has
 }
 
-func stubs(tags map[string]map[string]string) bool {
+func stubs(tags CommentTags) bool {
 	vals, has := tags["genreconciler"]
 	if !has {
 		return false
@@ -545,7 +552,7 @@ func reconcilerPackages(basePackage string, groupPkgName string, gv clientgentyp
 		// Fix for golang iterator bug.
 		t := t
 		extracted := extractCommentTags(t)
-		reconcilerClass, hasReconcilerClass := extractReconcilerClassTag(extracted)
+		reconcilerClasses, hasReconcilerClass := extractReconcilerClassesTag(extracted)
 		nonNamespaced := isNonNamespaced(extracted)
 		isKRShaped := isKRShaped(extracted)
 		stubs := stubs(extracted)
@@ -573,7 +580,7 @@ func reconcilerPackages(basePackage string, groupPkgName string, gv clientgentyp
 					clientPkg:           clientPackagePath,
 					informerPackagePath: informerPackagePath,
 					schemePkg:           filepath.Join(customArgs.VersionedClientSetPackage, "scheme"),
-					reconcilerClass:     reconcilerClass,
+					reconcilerClasses:   reconcilerClasses,
 					hasReconcilerClass:  hasReconcilerClass,
 					hasStatus:           hasStatus(t),
 				})
@@ -602,7 +609,7 @@ func reconcilerPackages(basePackage string, groupPkgName string, gv clientgentyp
 						outputPackage:       filepath.Join(packagePath, "stub"),
 						imports:             generator.NewImportTracker(),
 						informerPackagePath: informerPackagePath,
-						reconcilerClass:     reconcilerClass,
+						reconcilerClasses:   reconcilerClasses,
 						hasReconcilerClass:  hasReconcilerClass,
 					})
 					return generators
@@ -633,7 +640,7 @@ func reconcilerPackages(basePackage string, groupPkgName string, gv clientgentyp
 					listerPkg:          listerPackagePath,
 					groupGoName:        groupGoName,
 					groupVersion:       gv,
-					reconcilerClass:    reconcilerClass,
+					reconcilerClasses:  reconcilerClasses,
 					hasReconcilerClass: hasReconcilerClass,
 					nonNamespaced:      nonNamespaced,
 					isKRShaped:         isKRShaped,

codegen/cmd/injection-gen/generators/reconciler_controller.go

@@ -38,7 +38,7 @@ type reconcilerControllerGenerator struct {
 	schemePkg           string
 	informerPackagePath string
 
-	reconcilerClass    string
+	reconcilerClasses  []string
 	hasReconcilerClass bool
 	hasStatus          bool
 }
@@ -69,7 +69,7 @@ func (g *reconcilerControllerGenerator) GenerateType(c *generator.Context, t *ty
 	m := map[string]interface{}{
 		"type":      t,
 		"group":     g.groupName,
-		"class":     g.reconcilerClass,
+		"classes":   g.reconcilerClasses,
 		"hasClass":  g.hasReconcilerClass,
 		"hasStatus": g.hasStatus,
 		"controllerImpl": c.Universe.Type(types.Name{
@@ -195,12 +195,24 @@ var reconcilerControllerNewImpl = `
 const (
 	defaultControllerAgentName = "{{.type|lowercaseSingular}}-controller"
 	defaultFinalizerName       = "{{.type|allLowercasePlural}}.{{.group}}"
-	{{if .hasClass}}
+	{{if .hasClass }}
 	// ClassAnnotationKey points to the annotation for the class of this resource.
-	ClassAnnotationKey = "{{ .class }}"
+	{{if gt (.classes | len) 1 }}// Deprecated: Use ClassAnnotationKeys given multiple keys exist
+	{{end -}}
+	ClassAnnotationKey = "{{ index .classes 0 }}"
 	{{end}}
 )
 
+{{if gt (.classes | len) 1 }}
+var (
+	// ClassAnnotationKeys points to the annotation for the class of this resource.
+	ClassAnnotationKeys = []string{
+		{{range $class := .classes}}"{{$class}}",
+		{{end}}
+	}
+)
+{{end}}
+
 // NewImpl returns a {{.controllerImpl|raw}} that handles queuing and feeding work from
 // the queue through an implementation of {{.controllerReconciler|raw}}, delegating to
 // the provided Interface and optional Finalizer methods. OptionsFn is used to return

codegen/cmd/injection-gen/generators/reconciler_controller_stub.go

@@ -35,7 +35,7 @@ type reconcilerControllerStubGenerator struct {
 
 	reconcilerPkg       string
 	informerPackagePath string
-	reconcilerClass     string
+	reconcilerClasses   []string
 	hasReconcilerClass  bool
 }
 
@@ -64,7 +64,7 @@ func (g *reconcilerControllerStubGenerator) GenerateType(c *generator.Context, t
 
 	m := map[string]interface{}{
 		"type":     t,
-		"class":    g.reconcilerClass,
+		"classes":  g.reconcilerClasses,
 		"hasClass": g.hasReconcilerClass,
 		"informerGet": c.Universe.Function(types.Name{
 			Package: g.informerPackagePath,
@@ -91,10 +91,18 @@ func (g *reconcilerControllerStubGenerator) GenerateType(c *generator.Context, t
 			Package: g.reconcilerPkg,
 			Name:    "ClassAnnotationKey",
 		}),
+		"classAnnotationKeys": c.Universe.Variable(types.Name{
+			Package: g.reconcilerPkg,
+			Name:    "ClassAnnotationKeys",
+		}),
 		"annotationFilterFunc": c.Universe.Function(types.Name{
 			Package: "knative.dev/pkg/reconciler",
 			Name:    "AnnotationFilterFunc",
 		}),
+		"orFilterFunc": c.Universe.Function(types.Name{
+			Package: "knative.dev/pkg/reconciler",
+			Name:    "Or",
+		}),
 		"filterHandler": c.Universe.Type(types.Name{
 			Package: "k8s.io/client-go/tools/cache",
 			Name:    "FilteringResourceEventHandler",
@@ -120,7 +128,17 @@ func NewController(
 
 	{{if .hasClass}}
 	classValue := "default" // TODO: update this to the appropriate value.
+	{{if len .classes | eq 1 }}
 	classFilter := {{.annotationFilterFunc|raw}}({{.classAnnotationKey|raw}}, classValue, false /*allowUnset*/)
+	{{else if gt (len .classes) 1 }}
+	filterFuncs := make([]func(interface{}) bool, 0, len({{.classAnnotationKeys|raw}}))
+
+	for _, key := range {{.classAnnotationKeys|raw}} {
+		filterFuncs = append(filterFuncs, {{.annotationFilterFunc|raw}}(key, classValue, false /*allowUnset*/))
+	}
+
+	classFilter := {{.orFilterFunc|raw}}(filterFuncs...)
+	{{end}}
 	{{end}}
 
 	// TODO: setup additional informers here.