diff --git a/pkg/apis/admissionregistration/types.go b/pkg/apis/admissionregistration/types.go index 037c0757709..a294deead58 100644 --- a/pkg/apis/admissionregistration/types.go +++ b/pkg/apis/admissionregistration/types.go @@ -64,6 +64,7 @@ type Initializer struct { // Rules describes what resources/subresources the initializer cares about. // The initializer cares about an operation if it matches _any_ Rule. + // Rule.Resources must not include subresources. Rules []Rule // FailurePolicy defines what happens if the responsible initializer controller @@ -97,7 +98,10 @@ type Rule struct { // '*/scale' means all scale subresources. // '*/*' means all resources and their subresources. // - // If '*' or '*/*' is present, the length of the slice must be one. + // If wildcard is present, the validation rule will ensure resources do not + // overlap with each other. + // + // Depending on the enclosing object, subresources might not be allowed. // Required. Resources []string } diff --git a/pkg/apis/admissionregistration/v1alpha1/types.go b/pkg/apis/admissionregistration/v1alpha1/types.go index 67a76dff981..e5456c97a30 100644 --- a/pkg/apis/admissionregistration/v1alpha1/types.go +++ b/pkg/apis/admissionregistration/v1alpha1/types.go @@ -66,6 +66,7 @@ type Initializer struct { // Rules describes what resources/subresources the initializer cares about. // The initializer cares about an operation if it matches _any_ Rule. + // Rule.Resources must not include subresources. Rules []Rule `json:"rules,omitempty" protobuf:"bytes,2,rep,name=rules"` // FailurePolicy defines what happens if the responsible initializer controller @@ -99,7 +100,10 @@ type Rule struct { // '*/scale' means all scale subresources. // '*/*' means all resources and their subresources. // - // If '*' or '*/*' is present, the length of the slice must be one. + // If wildcard is present, the validation rule will ensure resources do not + // overlap with each other. + // + // Depending on the enclosing object, subresources might not be allowed. // Required. Resources []string `json:"resources,omitempty" protobuf:"bytes,3,rep,name=resources"` } diff --git a/pkg/apis/admissionregistration/validation/validation.go b/pkg/apis/admissionregistration/validation/validation.go index aa8a0e0a68f..4c478f457e0 100644 --- a/pkg/apis/admissionregistration/validation/validation.go +++ b/pkg/apis/admissionregistration/validation/validation.go @@ -49,7 +49,8 @@ func validateInitializer(initializer *admissionregistration.Initializer, fldPath } for i, rule := range initializer.Rules { - allErrors = append(allErrors, validateRule(&rule, fldPath.Child("rules").Index(i))...) + notAllowSubresources := false + allErrors = append(allErrors, validateRule(&rule, fldPath.Child("rules").Index(i), notAllowSubresources)...) } // TODO: relax the validation rule when admissionregistration is beta. if initializer.FailurePolicy != nil && *initializer.FailurePolicy != admissionregistration.Ignore { @@ -102,10 +103,10 @@ func validateResources(resources []string, fldPath *field.Path) field.ErrorList } res, sub := parts[0], parts[1] if _, ok := resourcesWithWildcardSubresoures[res]; ok { - allErrors = append(allErrors, field.Invalid(fldPath.Index(i), resources[i], fmt.Sprintf("if '%s/*' is present, must not specify %s", res, resources[i]))) + allErrors = append(allErrors, field.Invalid(fldPath.Index(i), resSub, fmt.Sprintf("if '%s/*' is present, must not specify %s", res, resSub))) } if _, ok := subResoucesWithWildcardResource[sub]; ok { - allErrors = append(allErrors, field.Invalid(fldPath.Index(i), resources[i], fmt.Sprintf("if '*/%s' is present, must not specify %s", sub, resources[i]))) + allErrors = append(allErrors, field.Invalid(fldPath.Index(i), resSub, fmt.Sprintf("if '*/%s' is present, must not specify %s", sub, resSub))) } if sub == "*" { resourcesWithWildcardSubresoures[res] = struct{}{} @@ -123,7 +124,26 @@ func validateResources(resources []string, fldPath *field.Path) field.ErrorList return allErrors } -func validateRule(rule *admissionregistration.Rule, fldPath *field.Path) field.ErrorList { +func validateResourcesNoSubResources(resources []string, fldPath *field.Path) field.ErrorList { + var allErrors field.ErrorList + if len(resources) == 0 { + allErrors = append(allErrors, field.Required(fldPath, "")) + } + for i, resource := range resources { + if resource == "" { + allErrors = append(allErrors, field.Required(fldPath.Index(i), "")) + } + if strings.Contains(resource, "/") { + allErrors = append(allErrors, field.Invalid(fldPath.Index(i), resource, "must not specify subresources")) + } + } + if len(resources) > 1 && hasWildcard(resources) { + allErrors = append(allErrors, field.Invalid(fldPath, resources, "if '*' is present, must not specify other resources")) + } + return allErrors +} + +func validateRule(rule *admissionregistration.Rule, fldPath *field.Path, allowSubResource bool) field.ErrorList { var allErrors field.ErrorList if len(rule.APIGroups) == 0 { allErrors = append(allErrors, field.Required(fldPath.Child("apiGroups"), "")) @@ -143,7 +163,11 @@ func validateRule(rule *admissionregistration.Rule, fldPath *field.Path) field.E allErrors = append(allErrors, field.Required(fldPath.Child("apiVersions").Index(i), "")) } } - allErrors = append(allErrors, validateResources(rule.Resources, fldPath.Child("resources"))...) + if allowSubResource { + allErrors = append(allErrors, validateResources(rule.Resources, fldPath.Child("resources"))...) + } else { + allErrors = append(allErrors, validateResourcesNoSubResources(rule.Resources, fldPath.Child("resources"))...) + } return allErrors } @@ -154,7 +178,7 @@ func ValidateInitializerConfigurationUpdate(newIC, oldIC *admissionregistration. func ValidateExternalAdmissionHookConfiguration(e *admissionregistration.ExternalAdmissionHookConfiguration) field.ErrorList { allErrors := genericvalidation.ValidateObjectMeta(&e.ObjectMeta, false, genericvalidation.NameIsDNSSubdomain, field.NewPath("metadata")) for i, hook := range e.ExternalAdmissionHooks { - allErrors = append(allErrors, validateExternalAdmissionHook(&hook, field.NewPath("externalAdmissionHook").Index(i))...) + allErrors = append(allErrors, validateExternalAdmissionHook(&hook, field.NewPath("externalAdmissionHooks").Index(i))...) } return allErrors } @@ -212,7 +236,8 @@ func validateRuleWithOperations(ruleWithOperations *admissionregistration.RuleWi allErrors = append(allErrors, field.NotSupported(fldPath.Child("operations").Index(i), operation, supportedOperations.List())) } } - allErrors = append(allErrors, validateRule(&ruleWithOperations.Rule, fldPath)...) + allowSubResource := true + allErrors = append(allErrors, validateRule(&ruleWithOperations.Rule, fldPath, allowSubResource)...) return allErrors } diff --git a/pkg/apis/admissionregistration/validation/validation_test.go b/pkg/apis/admissionregistration/validation/validation_test.go index d25cb880a7b..a864ac3a866 100644 --- a/pkg/apis/admissionregistration/validation/validation_test.go +++ b/pkg/apis/admissionregistration/validation/validation_test.go @@ -181,7 +181,7 @@ func TestValidateInitializerConfiguration(t *testing.T) { expectedError: "resources[1]: Required value", }, { - name: "wildcard cannot be mixed with other strings for APIGroups or APIVersions", + name: "wildcard cannot be mixed with other strings for APIGroups or APIVersions or Resources", config: getInitializerConfiguration( []admissionregistration.Initializer{ { @@ -190,15 +190,15 @@ func TestValidateInitializerConfiguration(t *testing.T) { { APIGroups: []string{"a", "*"}, APIVersions: []string{"a", "*"}, - Resources: []string{"a"}, + Resources: []string{"a", "*"}, }, }, }, }), - expectedError: `initializers[0].rules[0].apiGroups: Invalid value: []string{"a", "*"}: if '*' is present, must not specify other API groups, initializers[0].rules[0].apiVersions: Invalid value: []string{"a", "*"}: if '*' is present, must not specify other API versions`, + expectedError: `[initializers[0].rules[0].apiGroups: Invalid value: []string{"a", "*"}: if '*' is present, must not specify other API groups, initializers[0].rules[0].apiVersions: Invalid value: []string{"a", "*"}: if '*' is present, must not specify other API versions, initializers[0].rules[0].resources: Invalid value: []string{"a", "*"}: if '*' is present, must not specify other resources]`, }, { - name: `resource "*" can co-exist with resources that have subresources`, + name: "Subresource not allowed", config: getInitializerConfiguration( []admissionregistration.Initializer{ { @@ -207,95 +207,12 @@ func TestValidateInitializerConfiguration(t *testing.T) { { APIGroups: []string{"a"}, APIVersions: []string{"a"}, - Resources: []string{"*", "a/b", "a/*", "*/b"}, + Resources: []string{"a/b"}, }, }, }, }), - }, - { - name: `resource "*" cannot mix with resources that don't have subresources`, - config: getInitializerConfiguration( - []admissionregistration.Initializer{ - { - Name: "initializer.k8s.io", - Rules: []admissionregistration.Rule{ - { - APIGroups: []string{"a"}, - APIVersions: []string{"a"}, - Resources: []string{"*", "a"}, - }, - }, - }, - }), - expectedError: `if '*' is present, must not specify other resources without subresources`, - }, - { - name: "resource a/* cannot mix with a/x", - config: getInitializerConfiguration( - []admissionregistration.Initializer{ - { - Name: "initializer.k8s.io", - Rules: []admissionregistration.Rule{ - { - APIGroups: []string{"a"}, - APIVersions: []string{"a"}, - Resources: []string{"a/*", "a/x"}, - }, - }, - }, - }), - expectedError: `initializers[0].rules[0].resources[1]: Invalid value: "a/x": if 'a/*' is present, must not specify a/x`, - }, - { - name: "resource a/* can mix with a", - config: getInitializerConfiguration( - []admissionregistration.Initializer{ - { - Name: "initializer.k8s.io", - Rules: []admissionregistration.Rule{ - { - APIGroups: []string{"a"}, - APIVersions: []string{"a"}, - Resources: []string{"a/*", "a"}, - }, - }, - }, - }), - }, - { - name: "resource */a cannot mix with x/a", - config: getInitializerConfiguration( - []admissionregistration.Initializer{ - { - Name: "initializer.k8s.io", - Rules: []admissionregistration.Rule{ - { - APIGroups: []string{"a"}, - APIVersions: []string{"a"}, - Resources: []string{"*/a", "x/a"}, - }, - }, - }, - }), - expectedError: `initializers[0].rules[0].resources[1]: Invalid value: "x/a": if '*/a' is present, must not specify x/a`, - }, - { - name: "resource */* cannot mix with other resources", - config: getInitializerConfiguration( - []admissionregistration.Initializer{ - { - Name: "initializer.k8s.io", - Rules: []admissionregistration.Rule{ - { - APIGroups: []string{"a"}, - APIVersions: []string{"a"}, - Resources: []string{"*/*", "a"}, - }, - }, - }, - }), - expectedError: `initializers[0].rules[0].resources: Invalid value: []string{"*/*", "a"}: if '*/*' is present, must not specify other resources`, + expectedError: ` "a/b": must not specify subresources`, }, { name: "FailurePolicy can only be \"Ignore\"", @@ -357,7 +274,7 @@ func TestValidateExternalAdmissionHookConfiguration(t *testing.T) { Name: "", }, }), - expectedError: `externalAdmissionHook[1].name: Invalid value: "k8s.io": should be a domain with at least two dots, externalAdmissionHook[2].name: Required value`, + expectedError: `externalAdmissionHooks[1].name: Invalid value: "k8s.io": should be a domain with at least two dots, externalAdmissionHooks[2].name: Required value`, }, { name: "Operations must not be empty or nil", @@ -385,7 +302,7 @@ func TestValidateExternalAdmissionHookConfiguration(t *testing.T) { }, }, }), - expectedError: `externalAdmissionHook[0].rules[0].operations: Required value, externalAdmissionHook[0].rules[1].operations: Required value`, + expectedError: `externalAdmissionHooks[0].rules[0].operations: Required value, externalAdmissionHooks[0].rules[1].operations: Required value`, }, { name: "\"\" is NOT a valid operation", @@ -428,7 +345,7 @@ func TestValidateExternalAdmissionHookConfiguration(t *testing.T) { expectedError: `Unsupported value: "PATCH"`, }, { - name: "wildcard cannot be mixed with other strings", + name: "wildcard operation cannot be mixed with other strings", config: getExternalAdmissionHookConfiguration( []admissionregistration.ExternalAdmissionHook{ { @@ -447,12 +364,130 @@ func TestValidateExternalAdmissionHookConfiguration(t *testing.T) { }), expectedError: `if '*' is present, must not specify other operations`, }, + { + name: `resource "*" can co-exist with resources that have subresources`, + config: getExternalAdmissionHookConfiguration( + []admissionregistration.ExternalAdmissionHook{ + { + Name: "webhook.k8s.io", + Rules: []admissionregistration.RuleWithOperations{ + { + Operations: []admissionregistration.OperationType{"CREATE"}, + Rule: admissionregistration.Rule{ + APIGroups: []string{"a"}, + APIVersions: []string{"a"}, + Resources: []string{"*", "a/b", "a/*", "*/b"}, + }, + }, + }, + }, + }), + }, + { + name: `resource "*" cannot mix with resources that don't have subresources`, + config: getExternalAdmissionHookConfiguration( + []admissionregistration.ExternalAdmissionHook{ + { + Name: "webhook.k8s.io", + Rules: []admissionregistration.RuleWithOperations{ + { + Operations: []admissionregistration.OperationType{"CREATE"}, + Rule: admissionregistration.Rule{ + APIGroups: []string{"a"}, + APIVersions: []string{"a"}, + Resources: []string{"*", "a"}, + }, + }, + }, + }, + }), + expectedError: `if '*' is present, must not specify other resources without subresources`, + }, + { + name: "resource a/* cannot mix with a/x", + config: getExternalAdmissionHookConfiguration( + []admissionregistration.ExternalAdmissionHook{ + { + Name: "webhook.k8s.io", + Rules: []admissionregistration.RuleWithOperations{ + { + Operations: []admissionregistration.OperationType{"CREATE"}, + Rule: admissionregistration.Rule{ + APIGroups: []string{"a"}, + APIVersions: []string{"a"}, + Resources: []string{"a/*", "a/x"}, + }, + }, + }, + }, + }), + expectedError: `externalAdmissionHooks[0].rules[0].resources[1]: Invalid value: "a/x": if 'a/*' is present, must not specify a/x`, + }, + { + name: "resource a/* can mix with a", + config: getExternalAdmissionHookConfiguration( + []admissionregistration.ExternalAdmissionHook{ + { + Name: "webhook.k8s.io", + Rules: []admissionregistration.RuleWithOperations{ + { + Operations: []admissionregistration.OperationType{"CREATE"}, + Rule: admissionregistration.Rule{ + APIGroups: []string{"a"}, + APIVersions: []string{"a"}, + Resources: []string{"a/*", "a"}, + }, + }, + }, + }, + }), + }, + { + name: "resource */a cannot mix with x/a", + config: getExternalAdmissionHookConfiguration( + []admissionregistration.ExternalAdmissionHook{ + { + Name: "webhook.k8s.io", + Rules: []admissionregistration.RuleWithOperations{ + { + Operations: []admissionregistration.OperationType{"CREATE"}, + Rule: admissionregistration.Rule{ + APIGroups: []string{"a"}, + APIVersions: []string{"a"}, + Resources: []string{"*/a", "x/a"}, + }, + }, + }, + }, + }), + expectedError: `externalAdmissionHooks[0].rules[0].resources[1]: Invalid value: "x/a": if '*/a' is present, must not specify x/a`, + }, + { + name: "resource */* cannot mix with other resources", + config: getExternalAdmissionHookConfiguration( + []admissionregistration.ExternalAdmissionHook{ + { + Name: "webhook.k8s.io", + Rules: []admissionregistration.RuleWithOperations{ + { + Operations: []admissionregistration.OperationType{"CREATE"}, + Rule: admissionregistration.Rule{ + APIGroups: []string{"a"}, + APIVersions: []string{"a"}, + Resources: []string{"*/*", "a"}, + }, + }, + }, + }, + }), + expectedError: `externalAdmissionHooks[0].rules[0].resources: Invalid value: []string{"*/*", "a"}: if '*/*' is present, must not specify other resources`, + }, { name: "FailurePolicy can only be \"Ignore\"", config: getExternalAdmissionHookConfiguration( []admissionregistration.ExternalAdmissionHook{ { - Name: "initializer.k8s.io", + Name: "webhook.k8s.io", FailurePolicy: func() *admissionregistration.FailurePolicyType { r := admissionregistration.Fail return &r