Merge pull request #547 from RainbowMango/pr_validate_skipped_resource

Enable validation for --skipped-propagating-apis flag

cmd/controller-manager/app/controllermanager.go

@@ -96,6 +96,9 @@ func Run(ctx context.Context, opts *options.Options) error {
 	return nil
 }
 
+// setupControllers initialize controllers and setup one by one.
+// Note: ignore cyclomatic complexity check(by gocyclo) because it will not effect readability.
+//nolint:gocyclo
 func setupControllers(mgr controllerruntime.Manager, opts *options.Options, stopChan <-chan struct{}) {
 	resetConfig := mgr.GetConfig()
 	dynamicClientSet := dynamic.NewForConfigOrDie(resetConfig)
@@ -103,8 +106,11 @@ func setupControllers(mgr controllerruntime.Manager, opts *options.Options, stop
 	objectWatcher := objectwatcher.NewObjectWatcher(mgr.GetClient(), mgr.GetRESTMapper(), util.NewClusterDynamicClientSet)
 	overrideManager := overridemanager.New(mgr.GetClient())
 	skippedResourceConfig := util.NewSkippedResourceConfig()
-	// TODO(pigletfly): add SkippedPropagatingAPIs validation
+	if err := skippedResourceConfig.Parse(opts.SkippedPropagatingAPIs); err != nil {
-	skippedResourceConfig.Parse(opts.SkippedPropagatingAPIs)
+		// TODO(RainbowMango): the parameter should be checked earlier.
+		// Consider add validation to 'options.Options'
+		panic(err)
+	}
 	skippedPropagatingNamespaces := map[string]struct{}{}
 	for _, ns := range opts.SkippedPropagatingNamespaces {
 		skippedPropagatingNamespaces[ns] = struct{}{}

pkg/util/apigroup.go

@@ -1,10 +1,10 @@
 package util
 
 import (
+	"fmt"
 	"strings"
 
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/klog/v2"
 
 	clusterv1alpha1 "github.com/karmada-io/karmada/pkg/apis/cluster/v1alpha1"
 	policyv1alpha1 "github.com/karmada-io/karmada/pkg/apis/policy/v1alpha1"
@@ -35,34 +35,36 @@ func NewSkippedResourceConfig() *SkippedResourceConfig {
 	return r
 }
 
-// Parse to parse tokens to SkippedResourceConfig
+// Parse parses the --skipped-propagating-apis input.
-func (r *SkippedResourceConfig) Parse(c string) {
+func (r *SkippedResourceConfig) Parse(c string) error {
-	// v1/Node,Pod;networking.k8s.io/v1beta1/Ingress,IngressClass
+	// default(empty) input
-	// group networking.k8s.io
-	// group version networking.k8s.io/v1
-	// group version kind networking.k8s.io/v1/Ingress
-	// corev1 has no group
 	if c == "" {
-		return
+		return nil
 	}
 
 	tokens := strings.Split(c, ";")
 	for _, token := range tokens {
-		r.parseSingle(token)
+		if err := r.parseSingle(token); err != nil {
+			return fmt.Errorf("parse --skipped-propagating-apis %w", err)
+		}
 	}
+
+	return nil
 }
 
-func (r *SkippedResourceConfig) parseSingle(token string) {
+func (r *SkippedResourceConfig) parseSingle(token string) error {
 	switch strings.Count(token, "/") {
+	// Assume user don't want to skip the 'core'(no group name) group.
+	// So, it should be the case "<group>".
 	case 0:
-		// Group
 		r.Groups[token] = struct{}{}
+	// it should be the case "<group>/<version>"
 	case 1:
+		// for core group which don't have the group name, the case should be "v1/<kind>" or "v1/<kind>,<kind>..."
 		if strings.HasPrefix(token, "v1") {
-			// core v1
+			var kinds []string
-			kinds := []string{}
 			for _, k := range strings.Split(token, ",") {
-				if strings.Contains(k, "/") {
+				if strings.Contains(k, "/") { // "v1/<kind>"
 					s := strings.Split(k, "/")
 					kinds = append(kinds, s[1])
 				} else {
@@ -76,20 +78,22 @@ func (r *SkippedResourceConfig) parseSingle(token string) {
 				}
 				r.GroupVersionKinds[gvk] = struct{}{}
 			}
-		} else {
+		} else { // case "<group>/<version>"
-			// GroupVersion
+			parts := strings.Split(token, "/")
-			i := strings.Index(token, "/")
+			if len(parts) != 2 {
+				return fmt.Errorf("invalid token: %s", token)
+			}
 			gv := schema.GroupVersion{
-				Group:   token[:i],
+				Group:   parts[0],
-				Version: token[i+1:],
+				Version: parts[1],
 			}
 			r.GroupVersions[gv] = struct{}{}
 		}
+	// parameter format: "<group>/<version>/<kind>" or "<group>/<version>/<kind>,<kind>..."
 	case 2:
-		// GroupVersionKind
 		g := ""
 		v := ""
-		kinds := []string{}
+		var kinds []string
 		for _, k := range strings.Split(token, ",") {
 			if strings.Contains(k, "/") {
 				s := strings.Split(k, "/")
@@ -109,8 +113,10 @@ func (r *SkippedResourceConfig) parseSingle(token string) {
 			r.GroupVersionKinds[gvk] = struct{}{}
 		}
 	default:
-		klog.Error("Unsupported SkippedPropagatingAPIs: ", token)
+		return fmt.Errorf("invalid parameter: %s", token)
 	}
+
+	return nil
 }
 
 // GroupVersionDisabled returns whether GroupVersion is disabled.

pkg/util/apigroup_test.go

@@ -11,32 +11,36 @@ func TestSkippedResourceConfigGVKParse(t *testing.T) {
 		input string
 		out   []schema.GroupVersionKind
 	}{
-		{input: "v1/Node,Pod;networking.k8s.io/v1beta1/Ingress,IngressClass", out: []schema.GroupVersionKind{
+		{
-			{
+			input: "v1/Node,Pod;networking.k8s.io/v1beta1/Ingress,IngressClass",
-				Group:   "",
+			out: []schema.GroupVersionKind{
-				Version: "v1",
+				{
-				Kind:    "Node",
+					Group:   "",
-			},
+					Version: "v1",
-			{
+					Kind:    "Node",
-				Group:   "",
+				},
-				Version: "v1",
+				{
-				Kind:    "Pod",
+					Group:   "",
-			},
+					Version: "v1",
-			{
+					Kind:    "Pod",
-				Group:   "networking.k8s.io",
+				},
-				Version: "v1beta1",
+				{
-				Kind:    "Ingress",
+					Group:   "networking.k8s.io",
-			},
+					Version: "v1beta1",
-			{
+					Kind:    "Ingress",
-				Group:   "networking.k8s.io",
+				},
-				Version: "v1beta1",
+				{
-				Kind:    "IngressClass",
+					Group:   "networking.k8s.io",
-			},
+					Version: "v1beta1",
-		}},
+					Kind:    "IngressClass",
+				},
+			}},
 	}
 	for _, test := range tests {
 		r := NewSkippedResourceConfig()
-		r.Parse(test.input)
+		if err := r.Parse(test.input); err != nil {
+			t.Fatalf("Unexpected error: %v", err)
+		}
 		for i, o := range test.out {
 			ok := r.GroupVersionKindDisabled(o)
 			if !ok {
@@ -50,28 +54,32 @@ func TestResourceConfigGVParse(t *testing.T) {
 		input string
 		out   []schema.GroupVersion
 	}{
-		{input: "networking.k8s.io/v1;test/v1beta1", out: []schema.GroupVersion{
+		{
-			{
+			input: "networking.k8s.io/v1;test/v1beta1",
-				Group:   "networking.k8s.io",
+			out: []schema.GroupVersion{
-				Version: "v1",
+				{
-			},
+					Group:   "networking.k8s.io",
-			{
+					Version: "v1",
-				Group:   "networking.k8s.io",
+				},
-				Version: "v1",
+				{
-			},
+					Group:   "networking.k8s.io",
-			{
+					Version: "v1",
-				Group:   "test",
+				},
-				Version: "v1beta1",
+				{
-			},
+					Group:   "test",
-			{
+					Version: "v1beta1",
-				Group:   "test",
+				},
-				Version: "v1beta1",
+				{
-			},
+					Group:   "test",
-		}},
+					Version: "v1beta1",
+				},
+			}},
 	}
 	for _, test := range tests {
 		r := NewSkippedResourceConfig()
-		r.Parse(test.input)
+		if err := r.Parse(test.input); err != nil {
+			t.Fatalf("Unexpected error: %v", err)
+		}
 		for i, o := range test.out {
 			ok := r.GroupVersionDisabled(o)
 			if !ok {
@@ -86,13 +94,17 @@ func TestSkippedResourceConfigGroupParse(t *testing.T) {
 		input string
 		out   []string
 	}{
-		{input: "networking.k8s.io;test", out: []string{
+		{
-			"networking.k8s.io", "test",
+			input: "networking.k8s.io;test",
-		}},
+			out: []string{
+				"networking.k8s.io", "test",
+			}},
 	}
 	for _, test := range tests {
 		r := NewSkippedResourceConfig()
-		r.Parse(test.input)
+		if err := r.Parse(test.input); err != nil {
+			t.Fatalf("Unexpected error: %v", err)
+		}
 		for i, o := range test.out {
 			ok := r.GroupDisabled(o)
 			if !ok {
@@ -107,43 +119,47 @@ func TestSkippedResourceConfigMixedParse(t *testing.T) {
 		input string
 		out   SkippedResourceConfig
 	}{
-		{input: "v1/Node,Pod;networking.k8s.io/v1beta1/Ingress,IngressClass;networking.k8s.io;test.com/v1", out: SkippedResourceConfig{
+		{
-			Groups: map[string]struct{}{
+			input: "v1/Node,Pod;networking.k8s.io/v1beta1/Ingress,IngressClass;networking.k8s.io;test.com/v1",
-				"networking.k8s.io": {},
+			out: SkippedResourceConfig{
-			},
+				Groups: map[string]struct{}{
-			GroupVersions: map[schema.GroupVersion]struct{}{
+					"networking.k8s.io": {},
-				{
+				},
-					Group:   "test.com",
+				GroupVersions: map[schema.GroupVersion]struct{}{
-					Version: "v1",
+					{
-				}: {},
+						Group:   "test.com",
-			},
+						Version: "v1",
-			GroupVersionKinds: map[schema.GroupVersionKind]struct{}{
+					}: {},
-				{
+				},
-					Group:   "",
+				GroupVersionKinds: map[schema.GroupVersionKind]struct{}{
-					Version: "v1",
+					{
-					Kind:    "Node",
+						Group:   "",
-				}: {},
+						Version: "v1",
-				{
+						Kind:    "Node",
-					Group:   "",
+					}: {},
-					Version: "v1",
+					{
-					Kind:    "Pod",
+						Group:   "",
-				}: {},
+						Version: "v1",
-				{
+						Kind:    "Pod",
-					Group:   "networking.k8s.io",
+					}: {},
-					Version: "v1beta1",
+					{
-					Kind:    "Ingress",
+						Group:   "networking.k8s.io",
-				}: {},
+						Version: "v1beta1",
-				{
+						Kind:    "Ingress",
-					Group:   "networking.k8s.io",
+					}: {},
-					Version: "v1beta1",
+					{
-					Kind:    "IngressClass",
+						Group:   "networking.k8s.io",
-				}: {},
+						Version: "v1beta1",
-			},
+						Kind:    "IngressClass",
-		}},
+					}: {},
+				},
+			}},
 	}
 	for i, test := range tests {
 		r := NewSkippedResourceConfig()
-		r.Parse(test.input)
+		if err := r.Parse(test.input); err != nil {
+			t.Fatalf("Unexpected error: %v", err)
+		}
 		for g := range r.Groups {
 			ok := r.GroupDisabled(g)
 			if !ok {
@@ -172,13 +188,17 @@ func TestDefaultSkippedResourceConfigGroupParse(t *testing.T) {
 		input string
 		out   []string
 	}{
-		{input: "", out: []string{
+		{
-			"cluster.karmada.io", "policy.karmada.io", "work.karmada.io",
+			input: "",
-		}},
+			out: []string{
+				"cluster.karmada.io", "policy.karmada.io", "work.karmada.io",
+			}},
 	}
 	for _, test := range tests {
 		r := NewSkippedResourceConfig()
-		r.Parse(test.input)
+		if err := r.Parse(test.input); err != nil {
+			t.Fatalf("Unexpected error: %v", err)
+		}
 		for i, o := range test.out {
 			ok := r.GroupDisabled(o)
 			if !ok {