Skip to content

OCPBUGS-44199: Allow spaces in AWS tags #2124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 13, 2025
Merged

OCPBUGS-44199: Allow spaces in AWS tags #2124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96a465d
OCPBUGS-44199: Allow spaces in AWS resource tags
patrickdillon Feb 6, 2025
d5168f3
Generate infrastructure CRDs for aws tags with spaces
patrickdillon Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 162 additions & 0 deletions config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
apiVersion: apiextensions.k8s.io/v1 # Hack because controller-gen complains if we don't have this
name: "Infrastructure"
crdName: infrastructures.config.openshift.io
featureGates:
- -AWSClusterHostedDNS
tests:
onCreate:
- name: Should be able to create a minimal Infrastructure
Expand Down Expand Up @@ -1675,3 +1677,163 @@ tests:
serviceEndpoints:
- name: DNSServices
url: https://abc
- name: Should be able to create an aws resourcetag with spaces
initial: |
apiVersion: config.openshift.io/v1
kind: Infrastructure
spec:
platformSpec:
aws: {}
type: AWS
status:
controlPlaneTopology: HighlyAvailable
cpuPartitioning: None
infrastructureTopology: HighlyAvailable
platform: AWS
platformStatus:
aws:
region: us-east-1
resourceTags:
- key: key with space
value: value with space
type: AWS
updated: |
apiVersion: config.openshift.io/v1
kind: Infrastructure
spec:
platformSpec:
type: AWS
aws: {}
status:
controlPlaneTopology: HighlyAvailable
cpuPartitioning: None
infrastructureTopology: HighlyAvailable
platform: AWS
platformStatus:
aws:
region: us-east-1
resourceTags:
- key: key with space
value: value with space
type: AWS
expected: |
apiVersion: config.openshift.io/v1
kind: Infrastructure
spec:
platformSpec:
type: AWS
aws: {}
status:
controlPlaneTopology: HighlyAvailable
cpuPartitioning: None
infrastructureTopology: HighlyAvailable
platform: AWS
platformStatus:
aws:
Comment on lines +1731 to +1732
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what to do here with the cloudLoadBalancerConfig. The tests seem to be failing both when I include it and when I don't. I am not including it, because the field is feature gated

https://github.com/patrickdillon/api/blob/f4e9b1854e85290d8adb4ae4494496e1a639eb33/config/v1/types_infrastructure.go#L523

And that feature gate is not in the default feature set, so the field shouldn't be included here, right?

But the failing test complains about it:

�[0m �[38;5;243m[config.openshift.io/v1, Resource=infrastructures][ClusterProfiles=Hypershift,SelfManagedHA][FeatureSet="DevPreviewNoUpgrade"][FeatureGate=][File=0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml] Infrastructure �[0mOn Update �[38;5;9m�[1m[It] Should be able to create an aws resourcetag with characters '_', '.', '/', '=', '+', '-', ':', '@'�[0m
�[38;5;243m/home/padillon/go/src/github.com/openshift/api/tests/generator.go:314�[0m

  �[38;5;9m[FAILED] the following fields were expected to match but did not:
  [(status.platformStatus.aws.cloudLoadBalancerConfig/status.platformStatus.aws.cloudLoadBalancerConfig)]

So I try adding it like below, and that also fails...

      expected: |
        apiVersion: config.openshift.io/v1
        kind: Infrastructure
        spec:
          platformSpec:
            type: AWS
            aws: {}
        status:
          controlPlaneTopology: HighlyAvailable
          cpuPartitioning: None
          infrastructureTopology: HighlyAvailable
          platform: AWS
          platformStatus:
            aws:
              cloudLoadBalancerConfig:
                dnsType: PlatformDefault
              region: us-east-1
              resourceTags:
              - key: key with space
                value: value with space
            type: AWS

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoelSpeed it seems that the problem is that tests for TechPreview (etc) as well as the Default feature set are run on the AAA_ungated tests. So if I don't include the cloudLoadBalancerConfig the TechPreview (etc) tests fail, but if I do include the cloudLoadBalancerConfig tests for the Default set fail (current status of PR). I'm probably doing something wrong, but not sure what that is... WDYT?

Copy link
Contributor

@JoelSpeed JoelSpeed Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which gate is cloudLoadBalancerConfig behind? You will need to either include or exclude the gate in your test featuregates list.

As you aren't gating this change, you would exclude the gate -GateName within the list of feature gates for your test

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I did that in 0d10a56 and it seemed to resolve the issue. Thanks!

As this is in the AAA_ungated.yaml test file, it seems like we should potentially skip the non-default feature sets? Not sure if that is possible. A downside of doing that, though, is the tests would need to be updated upon graduation of features to the default feature set...

I'm definitely fine with this, if you are.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general we want the tests to run against all versions so that we ensure the ungated tests do not break because of a new gated addition

Copy link
Contributor Author

@patrickdillon patrickdillon Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, that is impossible, right? I suppose if the gate did not have default statuses, then it might not break the tests; but would it be exercising/proving anything?

It's probably my own lack of knowledge, but I don't understand the testing scenario. Just asking for my own understanding/potential process improvement. It's solved for this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for the tests you're adding, because they leverage part of the API, which has a newly gated default value, it makes it impossible.

If you were to split the new tests into a separate file, the remaining ungated tests should in theory be able to continue to run.

I believe that's possible without anything complaining 🤔

region: us-east-1
resourceTags:
- key: key with space
value: value with space
type: AWS
- name: Should be able to create an aws resourcetag with characters '_', '.', '/', '=', '+', '-', ':', '@'
initial: |
apiVersion: config.openshift.io/v1
kind: Infrastructure
spec:
platformSpec:
aws: {}
type: AWS
updated: |
apiVersion: config.openshift.io/v1
kind: Infrastructure
spec:
platformSpec:
type: AWS
aws: {}
status:
controlPlaneTopology: HighlyAvailable
cpuPartitioning: None
infrastructureTopology: HighlyAvailable
platform: AWS
platformStatus:
aws:
region: us-east-1
resourceTags:
- key: key:_./=+-@
value: value:_./=+-@
type: AWS
expected: |
apiVersion: config.openshift.io/v1
kind: Infrastructure
spec:
platformSpec:
type: AWS
aws: {}
status:
controlPlaneTopology: HighlyAvailable
cpuPartitioning: None
infrastructureTopology: HighlyAvailable
platform: AWS
platformStatus:
aws:
region: us-east-1
resourceTags:
- key: key:_./=+-@
value: value:_./=+-@
type: AWS
- name: Should not be able to create an aws resourcetag with character * in key
initial: |
apiVersion: config.openshift.io/v1
kind: Infrastructure
spec:
platformSpec:
aws: {}
type: AWS
updated: |
apiVersion: config.openshift.io/v1
kind: Infrastructure
spec:
platformSpec:
type: AWS
aws: {}
status:
controlPlaneTopology: HighlyAvailable
cpuPartitioning: None
infrastructureTopology: HighlyAvailable
platform: AWS
platformStatus:
aws:
region: us-east-1
resourceTags:
- key: key:_./=+-@*
value: value
type: AWS
expectedStatusError: "invalid AWS resource tag key. The string can contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', '@'"
- name: Should not be able to create an aws resourcetag with character * in value
initial: |
apiVersion: config.openshift.io/v1
kind: Infrastructure
spec:
platformSpec:
aws: {}
type: AWS
updated: |
apiVersion: config.openshift.io/v1
kind: Infrastructure
spec:
platformSpec:
type: AWS
aws: {}
status:
controlPlaneTopology: HighlyAvailable
cpuPartitioning: None
infrastructureTopology: HighlyAvailable
platform: AWS
platformStatus:
aws:
region: us-east-1
resourceTags:
- key: key
value: value*
type: AWS
expectedStatusError: "invalid AWS resource tag value. The string can contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', '@'"
12 changes: 8 additions & 4 deletions config/v1/types_infrastructure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,18 +528,22 @@ type AWSPlatformStatus struct {

// AWSResourceTag is a tag to apply to AWS resources created for the cluster.
type AWSResourceTag struct {
// key is the key of the tag
// key sets the key of the AWS resource tag key-value pair. Key is required when defining an AWS resource tag.
// Key should consist of between 1 and 128 characters, and may
// contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', and '@'.
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=128
// +kubebuilder:validation:Pattern=`^[0-9A-Za-z_.:/=+-@]+$`
// +kubebuilder:validation:XValidation:rule=`self.matches('^[0-9A-Za-z_.:/=+-@ ]+$')`,message="invalid AWS resource tag key. The string can contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', '@'"
// +required
Key string `json:"key"`
// value is the value of the tag.
// value sets the value of the AWS resource tag key-value pair. Value is required when defining an AWS resource tag.
// Value should consist of between 1 and 256 characters, and may
// contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', and '@'.
// Some AWS service do not support empty values. Since tags are added to resources in many services, the
// length of the tag value must meet the requirements of all services.
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=256
// +kubebuilder:validation:Pattern=`^[0-9A-Za-z_.:/=+-@]+$`
// +kubebuilder:validation:XValidation:rule=`self.matches('^[0-9A-Za-z_.:/=+-@ ]+$')`,message="invalid AWS resource tag value. The string can contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', '@'"
// +required
Value string `json:"value"`
}
Expand Down
23 changes: 19 additions & 4 deletions ...nerated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1305,20 +1305,35 @@ spec:
created for the cluster.
properties:
key:
description: key is the key of the tag
description: |-
key sets the key of the AWS resource tag key-value pair. Key is required when defining an AWS resource tag.
Key should consist of between 1 and 128 characters, and may
contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', and '@'.
maxLength: 128
minLength: 1
pattern: ^[0-9A-Za-z_.:/=+-@]+$
type: string
x-kubernetes-validations:
- message: invalid AWS resource tag key. The string
can contain only the set of alphanumeric characters,
space (' '), '_', '.', '/', '=', '+', '-', ':',
'@'
rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$')
value:
description: |-
value is the value of the tag.
value sets the value of the AWS resource tag key-value pair. Value is required when defining an AWS resource tag.
Value should consist of between 1 and 256 characters, and may
contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', and '@'.
Some AWS service do not support empty values. Since tags are added to resources in many services, the
length of the tag value must meet the requirements of all services.
maxLength: 256
minLength: 1
pattern: ^[0-9A-Za-z_.:/=+-@]+$
type: string
x-kubernetes-validations:
- message: invalid AWS resource tag value. The string
can contain only the set of alphanumeric characters,
space (' '), '_', '.', '/', '=', '+', '-', ':',
'@'
rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$')
required:
- key
- value
Expand Down
23 changes: 19 additions & 4 deletions ...v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1185,20 +1185,35 @@ spec:
created for the cluster.
properties:
key:
description: key is the key of the tag
description: |-
key sets the key of the AWS resource tag key-value pair. Key is required when defining an AWS resource tag.
Key should consist of between 1 and 128 characters, and may
contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', and '@'.
maxLength: 128
minLength: 1
pattern: ^[0-9A-Za-z_.:/=+-@]+$
type: string
x-kubernetes-validations:
- message: invalid AWS resource tag key. The string
can contain only the set of alphanumeric characters,
space (' '), '_', '.', '/', '=', '+', '-', ':',
'@'
rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$')
value:
description: |-
value is the value of the tag.
value sets the value of the AWS resource tag key-value pair. Value is required when defining an AWS resource tag.
Value should consist of between 1 and 256 characters, and may
contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', and '@'.
Some AWS service do not support empty values. Since tags are added to resources in many services, the
length of the tag value must meet the requirements of all services.
maxLength: 256
minLength: 1
pattern: ^[0-9A-Za-z_.:/=+-@]+$
type: string
x-kubernetes-validations:
- message: invalid AWS resource tag value. The string
can contain only the set of alphanumeric characters,
space (' '), '_', '.', '/', '=', '+', '-', ':',
'@'
rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$')
required:
- key
- value
Expand Down
23 changes: 19 additions & 4 deletions ...ted.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1305,20 +1305,35 @@ spec:
created for the cluster.
properties:
key:
description: key is the key of the tag
description: |-
key sets the key of the AWS resource tag key-value pair. Key is required when defining an AWS resource tag.
Key should consist of between 1 and 128 characters, and may
contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', and '@'.
maxLength: 128
minLength: 1
pattern: ^[0-9A-Za-z_.:/=+-@]+$
type: string
x-kubernetes-validations:
- message: invalid AWS resource tag key. The string
can contain only the set of alphanumeric characters,
space (' '), '_', '.', '/', '=', '+', '-', ':',
'@'
rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$')
value:
description: |-
value is the value of the tag.
value sets the value of the AWS resource tag key-value pair. Value is required when defining an AWS resource tag.
Value should consist of between 1 and 256 characters, and may
contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', and '@'.
Some AWS service do not support empty values. Since tags are added to resources in many services, the
length of the tag value must meet the requirements of all services.
maxLength: 256
minLength: 1
pattern: ^[0-9A-Za-z_.:/=+-@]+$
type: string
x-kubernetes-validations:
- message: invalid AWS resource tag value. The string
can contain only the set of alphanumeric characters,
space (' '), '_', '.', '/', '=', '+', '-', ':',
'@'
rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$')
required:
- key
- value
Expand Down
23 changes: 19 additions & 4 deletions ...ed.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1305,20 +1305,35 @@ spec:
created for the cluster.
properties:
key:
description: key is the key of the tag
description: |-
key sets the key of the AWS resource tag key-value pair. Key is required when defining an AWS resource tag.
Key should consist of between 1 and 128 characters, and may
contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', and '@'.
maxLength: 128
minLength: 1
pattern: ^[0-9A-Za-z_.:/=+-@]+$
type: string
x-kubernetes-validations:
- message: invalid AWS resource tag key. The string
can contain only the set of alphanumeric characters,
space (' '), '_', '.', '/', '=', '+', '-', ':',
'@'
rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$')
value:
description: |-
value is the value of the tag.
value sets the value of the AWS resource tag key-value pair. Value is required when defining an AWS resource tag.
Value should consist of between 1 and 256 characters, and may
contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', and '@'.
Some AWS service do not support empty values. Since tags are added to resources in many services, the
length of the tag value must meet the requirements of all services.
maxLength: 256
minLength: 1
pattern: ^[0-9A-Za-z_.:/=+-@]+$
type: string
x-kubernetes-validations:
- message: invalid AWS resource tag value. The string
can contain only the set of alphanumeric characters,
space (' '), '_', '.', '/', '=', '+', '-', ':',
'@'
rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$')
required:
- key
- value
Expand Down
Loading