From 428466f0e794082b31f57fd48f15c9051032fe2d Mon Sep 17 00:00:00 2001 From: Bharath B Date: Thu, 1 Sep 2022 18:58:35 +0530 Subject: [PATCH 1/5] CFE-583 : Update install-config CRD to support azure userTags. --- data/data/install.openshift.io_installconfigs.yaml | 7 +++++++ pkg/types/azure/platform.go | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/data/data/install.openshift.io_installconfigs.yaml b/data/data/install.openshift.io_installconfigs.yaml index 199520f07dd..d29bca6615d 100644 --- a/data/data/install.openshift.io_installconfigs.yaml +++ b/data/data/install.openshift.io_installconfigs.yaml @@ -1916,6 +1916,13 @@ spec: description: VirtualNetwork specifies the name of an existing VNet for the installer to use type: string + userTags: + additionalProperties: + type: string + description: UserTags additional keys and values that the installer + will add as tags to all resources that it creates. Resources + created by the cluster itself may not include these tags. + type: object required: - region type: object diff --git a/pkg/types/azure/platform.go b/pkg/types/azure/platform.go index b32b10aa41e..8d43677252f 100644 --- a/pkg/types/azure/platform.go +++ b/pkg/types/azure/platform.go @@ -86,6 +86,12 @@ type Platform struct { // // +optional ResourceGroupName string `json:"resourceGroupName,omitempty"` + + // UserTags additional keys and values that the installer will add + // as tags to all resources that it creates. Resources created by the + // cluster itself may not include these tags. + // +optional + UserTags map[string]string `json:"userTags,omitempty"` } // CloudEnvironment is the name of the Azure cloud environment From b42db85e2a1691d656f7141ac64e1ec3b3702653 Mon Sep 17 00:00:00 2001 From: Bharath B Date: Mon, 5 Sep 2022 23:11:35 +0530 Subject: [PATCH 2/5] verify-codegen updated changes --- data/data/install.openshift.io_installconfigs.yaml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/data/data/install.openshift.io_installconfigs.yaml b/data/data/install.openshift.io_installconfigs.yaml index 1ddc51cf33f..d122fe030b5 100644 --- a/data/data/install.openshift.io_installconfigs.yaml +++ b/data/data/install.openshift.io_installconfigs.yaml @@ -1912,10 +1912,6 @@ spec: cluster. If empty, a new resource group will created for the cluster. type: string - virtualNetwork: - description: VirtualNetwork specifies the name of an existing - VNet for the installer to use - type: string userTags: additionalProperties: type: string @@ -1923,6 +1919,10 @@ spec: will add as tags to all resources that it creates. Resources created by the cluster itself may not include these tags. type: object + virtualNetwork: + description: VirtualNetwork specifies the name of an existing + VNet for the installer to use + type: string required: - region type: object From 60060dfffbc7a481f44fb1f8eb85dcc7ac19e79e Mon Sep 17 00:00:00 2001 From: Bharath B Date: Thu, 12 Jan 2023 13:20:31 +0530 Subject: [PATCH 3/5] update description of UserTags --- data/data/install.openshift.io_installconfigs.yaml | 7 ++++--- pkg/types/azure/platform.go | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/data/data/install.openshift.io_installconfigs.yaml b/data/data/install.openshift.io_installconfigs.yaml index 04fba1a7c34..eaebe7dae75 100644 --- a/data/data/install.openshift.io_installconfigs.yaml +++ b/data/data/install.openshift.io_installconfigs.yaml @@ -1947,9 +1947,10 @@ spec: userTags: additionalProperties: type: string - description: UserTags additional keys and values that the installer - will add as tags to all resources that it creates. Resources - created by the cluster itself may not include these tags. + description: UserTags has additional keys and values that the + installer will add as tags to all resources that it creates. + Resources created by the cluster itself may not include these + tags. type: object virtualNetwork: description: VirtualNetwork specifies the name of an existing diff --git a/pkg/types/azure/platform.go b/pkg/types/azure/platform.go index fe4a9aa3902..cec872087a7 100644 --- a/pkg/types/azure/platform.go +++ b/pkg/types/azure/platform.go @@ -87,7 +87,7 @@ type Platform struct { // +optional ResourceGroupName string `json:"resourceGroupName,omitempty"` - // UserTags additional keys and values that the installer will add + // UserTags has additional keys and values that the installer will add // as tags to all resources that it creates. Resources created by the // cluster itself may not include these tags. // +optional From 2ca24f5b34d4cc754c6a5e1f8208b7bf6fe9f8db Mon Sep 17 00:00:00 2001 From: Bharath B Date: Thu, 12 Jan 2023 16:31:29 +0530 Subject: [PATCH 4/5] add validation for userTags --- pkg/types/azure/validation/platform.go | 61 ++++++++++++ pkg/types/azure/validation/platform_test.go | 104 ++++++++++++++++++++ 2 files changed, 165 insertions(+) diff --git a/pkg/types/azure/validation/platform.go b/pkg/types/azure/validation/platform.go index c1a90bf3192..7a07b70685c 100644 --- a/pkg/types/azure/validation/platform.go +++ b/pkg/types/azure/validation/platform.go @@ -2,6 +2,7 @@ package validation import ( "fmt" + "regexp" "sort" "k8s.io/apimachinery/pkg/util/validation/field" @@ -28,6 +29,21 @@ var ( }() ) +var ( + // tagKeyRegex is for verifying that the tag key contains only allowed characters. + tagKeyRegex = regexp.MustCompile(`^[a-zA-Z][0-9A-Za-z_.=+\-@]{1,127}$`) + + // tagValueRegex is for verifying that the tag value contains only allowed characters. + tagValueRegex = regexp.MustCompile(`^[0-9A-Za-z_.=+\-@]{1,256}$`) + + // tagKeyPrefixRegex is for verifying that the tag value does not contain restricted prefixes + tagKeyPrefixRegex = regexp.MustCompile(`^(?i)(name$|kubernetes\.io|openshift\.io|microsoft|azure|windows)`) +) + +// maxUserTagLimit is the maximum userTags that can be configured as defined in openshift/api +// https://github.com/openshift/api/blob/068483260288d83eac56053e202761b1702d46f5/config/v1/types_infrastructure.go#L482-L488 +const maxUserTagLimit = 10 + // ValidatePlatform checks that the specified platform is valid. func ValidatePlatform(p *azure.Platform, publish types.PublishingStrategy, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} @@ -72,6 +88,9 @@ func ValidatePlatform(p *azure.Platform, publish types.PublishingStrategy, fldPa allErrs = append(allErrs, field.Invalid(fldPath.Child("outboundType"), p.OutboundType, fmt.Sprintf("%s is only allowed when installing to pre-existing network", azure.UserDefinedRoutingOutboundType))) } + // check if configured userTags are valid + allErrs = append(allErrs, validateUserTags(p.UserTags, fldPath.Child("userTags"))...) + switch cloud := p.CloudName; cloud { case azure.StackCloud: allErrs = append(allErrs, validateAzureStack(p, fldPath)...) @@ -87,6 +106,48 @@ func ValidatePlatform(p *azure.Platform, publish types.PublishingStrategy, fldPa return allErrs } +// validateUserTags verifies if configured number of UserTags is not more than +// allowed limit and the tag keys and values are valid +func validateUserTags(tags map[string]string, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if len(tags) == 0 { + return allErrs + } + + if len(tags) > maxUserTagLimit { + allErrs = append(allErrs, field.TooMany(fldPath, len(tags), maxUserTagLimit)) + } + + for key, value := range tags { + if err := validateTag(key, value); err != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Key(key), value, err.Error())) + } + } + return allErrs +} + +// validateTag checks the following to ensure that the tag configured is acceptable +// - The key and value contain only allowed characters. +// - The key is not empty and at most 128 characters and starts with an alphabet. +// - The value is not empty and at most 256 characters. +// Note: Although azure allows empty value, the tags may be applied to resources +// in services that do not accept empty tag values. Consequently, OpenShift cannot +// accept empty tag values. +// - The key cannot be Name or have kubernetes.io, openshift.io, microsoft, azure, +// windows prefixes. +func validateTag(key, value string) error { + if !tagKeyRegex.MatchString(key) { + return fmt.Errorf("key is invalid or contains invalid characters") + } + if !tagValueRegex.MatchString(value) { + return fmt.Errorf("value is invalid or contains invalid characters") + } + if tagKeyPrefixRegex.MatchString(key) { + return fmt.Errorf("key contains restricted prefix") + } + return nil +} + var ( validOutboundTypes = map[azure.OutboundType]struct{}{ azure.LoadbalancerOutboundType: {}, diff --git a/pkg/types/azure/validation/platform_test.go b/pkg/types/azure/validation/platform_test.go index 7b36c52b5d6..f7d7d3e182e 100644 --- a/pkg/types/azure/validation/platform_test.go +++ b/pkg/types/azure/validation/platform_test.go @@ -167,3 +167,107 @@ func TestValidatePlatform(t *testing.T) { }) } } + +func TestValidateUserTags(t *testing.T) { + fieldPath := "spec.platform.azure.userTags" + cases := []struct { + name string + userTags map[string]string + wantErr bool + }{ + { + name: "userTags not configured", + userTags: map[string]string{}, + wantErr: false, + }, + { + name: "userTags configured", + userTags: map[string]string{ + "key1": "value1", "key_2": "value_2", "key.3": "value.3", "key=4": "value=4", "key+5": "value+5", + "key-6": "value-6", "key@7": "value@7", "key8_": "value8-", "key9=": "value9+", "key10-": "value10@"}, + wantErr: false, + }, + { + name: "userTags configured is more than max limit", + userTags: map[string]string{ + "key1": "value1", "key2": "value2", "key3": "value3", "key4": "value4", "key5": "value5", + "key6": "value6", "key7": "value7", "key8": "value8", "key9": "value9", "key10": "value10", + "key11": "value11"}, + wantErr: true, + }, + { + name: "userTags contains key starting a number", + userTags: map[string]string{"1key": "1value"}, + wantErr: true, + }, + { + name: "userTags contains empty key", + userTags: map[string]string{"": "value"}, + wantErr: true, + }, + { + name: "userTags contains key length greater than 128", + userTags: map[string]string{ + "thisisaverylongkeywithmorethan128characterswhichisnotallowedforazureresourcetagkeysandthetagkeyvalidationshouldfailwithinvalidfieldvalueerror": "value"}, + wantErr: true, + }, + { + name: "userTags contains key with invalid character", + userTags: map[string]string{"key/test": "value"}, + wantErr: true, + }, + { + name: "userTags contains value length greater than 256", + userTags: map[string]string{"key": "thisisaverylongvaluewithmorethan256characterswhichisnotallowedforazureresourcetagvaluesandthetagvaluevalidationshouldfailwithinvalidfieldvalueerrorrepeatthisisaverylongvaluewithmorethan256characterswhichisnotallowedforazureresourcetagvaluesandthetagvaluevalidationshouldfailwithinvalidfieldvalueerror"}, + wantErr: true, + }, + { + name: "userTags contains empty value", + userTags: map[string]string{"key": ""}, + wantErr: true, + }, + { + name: "userTags contains value with invalid character", + userTags: map[string]string{"key": "value*^%"}, + wantErr: true, + }, + { + name: "userTags contains key as name", + userTags: map[string]string{"name": "value"}, + wantErr: true, + }, + { + name: "userTags contains allowed key name123", + userTags: map[string]string{"name123": "value"}, + wantErr: false, + }, + { + name: "userTags contains key with prefix kubernetes.io", + userTags: map[string]string{"kubernetes.io_cluster": "value"}, + wantErr: true, + }, + { + name: "userTags contains allowed key prefix for_openshift.io", + userTags: map[string]string{"for_openshift.io": "azure"}, + wantErr: false, + }, + { + name: "userTags contains key with prefix azure", + userTags: map[string]string{"azure": "microsoft"}, + wantErr: true, + }, + { + name: "userTags contains allowed key resourcename", + userTags: map[string]string{"resourcename": "value"}, + wantErr: false, + }, + } + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + err := validateUserTags(tt.userTags, field.NewPath(fieldPath)) + if (len(err) > 0) != tt.wantErr { + t.Errorf("unexpected error, err: %v", err) + } + }) + } +} From 991e3f48bfb92ee77e9c43c48a32032742e1973a Mon Sep 17 00:00:00 2001 From: Bharath B Date: Tue, 24 Jan 2023 11:51:06 +0530 Subject: [PATCH 5/5] fix ci failures --- pkg/explain/printer_test.go | 3 +++ pkg/types/azure/validation/platform.go | 10 +++++----- pkg/types/azure/validation/platform_test.go | 8 ++++---- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/pkg/explain/printer_test.go b/pkg/explain/printer_test.go index 85fe104febf..4f0f676107d 100644 --- a/pkg/explain/printer_test.go +++ b/pkg/explain/printer_test.go @@ -209,6 +209,9 @@ func Test_PrintFields(t *testing.T) { resourceGroupName ResourceGroupName is the name of an already existing resource group where the cluster should be installed. This resource group should only be used for this specific cluster and the cluster components will assume ownership of all resources in the resource group. Destroying the cluster using installer will delete this resource group. This resource group must be empty with no other resources when trying to use it for creating a cluster. If empty, a new resource group will created for the cluster. + userTags + UserTags has additional keys and values that the installer will add as tags to all resources that it creates. Resources created by the cluster itself may not include these tags. + virtualNetwork VirtualNetwork specifies the name of an existing VNet for the installer to use`, }, { diff --git a/pkg/types/azure/validation/platform.go b/pkg/types/azure/validation/platform.go index 7a07b70685c..171c695a9cf 100644 --- a/pkg/types/azure/validation/platform.go +++ b/pkg/types/azure/validation/platform.go @@ -36,11 +36,11 @@ var ( // tagValueRegex is for verifying that the tag value contains only allowed characters. tagValueRegex = regexp.MustCompile(`^[0-9A-Za-z_.=+\-@]{1,256}$`) - // tagKeyPrefixRegex is for verifying that the tag value does not contain restricted prefixes + // tagKeyPrefixRegex is for verifying that the tag value does not contain restricted prefixes. tagKeyPrefixRegex = regexp.MustCompile(`^(?i)(name$|kubernetes\.io|openshift\.io|microsoft|azure|windows)`) ) -// maxUserTagLimit is the maximum userTags that can be configured as defined in openshift/api +// maxUserTagLimit is the maximum userTags that can be configured as defined in openshift/api. // https://github.com/openshift/api/blob/068483260288d83eac56053e202761b1702d46f5/config/v1/types_infrastructure.go#L482-L488 const maxUserTagLimit = 10 @@ -88,7 +88,7 @@ func ValidatePlatform(p *azure.Platform, publish types.PublishingStrategy, fldPa allErrs = append(allErrs, field.Invalid(fldPath.Child("outboundType"), p.OutboundType, fmt.Sprintf("%s is only allowed when installing to pre-existing network", azure.UserDefinedRoutingOutboundType))) } - // check if configured userTags are valid + // check if configured userTags are valid. allErrs = append(allErrs, validateUserTags(p.UserTags, fldPath.Child("userTags"))...) switch cloud := p.CloudName; cloud { @@ -107,7 +107,7 @@ func ValidatePlatform(p *azure.Platform, publish types.PublishingStrategy, fldPa } // validateUserTags verifies if configured number of UserTags is not more than -// allowed limit and the tag keys and values are valid +// allowed limit and the tag keys and values are valid. func validateUserTags(tags map[string]string, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if len(tags) == 0 { @@ -126,7 +126,7 @@ func validateUserTags(tags map[string]string, fldPath *field.Path) field.ErrorLi return allErrs } -// validateTag checks the following to ensure that the tag configured is acceptable +// validateTag checks the following to ensure that the tag configured is acceptable. // - The key and value contain only allowed characters. // - The key is not empty and at most 128 characters and starts with an alphabet. // - The value is not empty and at most 256 characters. diff --git a/pkg/types/azure/validation/platform_test.go b/pkg/types/azure/validation/platform_test.go index f7d7d3e182e..36c4f84bcc3 100644 --- a/pkg/types/azure/validation/platform_test.go +++ b/pkg/types/azure/validation/platform_test.go @@ -181,11 +181,11 @@ func TestValidateUserTags(t *testing.T) { wantErr: false, }, { - name: "userTags configured", + name: "userTags configured", userTags: map[string]string{ "key1": "value1", "key_2": "value_2", "key.3": "value.3", "key=4": "value=4", "key+5": "value+5", "key-6": "value-6", "key@7": "value@7", "key8_": "value8-", "key9=": "value9+", "key10-": "value10@"}, - wantErr: false, + wantErr: false, }, { name: "userTags configured is more than max limit", @@ -206,10 +206,10 @@ func TestValidateUserTags(t *testing.T) { wantErr: true, }, { - name: "userTags contains key length greater than 128", + name: "userTags contains key length greater than 128", userTags: map[string]string{ "thisisaverylongkeywithmorethan128characterswhichisnotallowedforazureresourcetagkeysandthetagkeyvalidationshouldfailwithinvalidfieldvalueerror": "value"}, - wantErr: true, + wantErr: true, }, { name: "userTags contains key with invalid character",