-
Notifications
You must be signed in to change notification settings - Fork 606
no-jira: Validate AWS resource tag keys with aws: prefix #2183
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -529,6 +529,7 @@ type AWSPlatformSpec struct { | |
| } | ||
|
|
||
| // AWSPlatformStatus holds the current status of the Amazon Web Services infrastructure provider. | ||
| // +kubebuilder:validation:XValidation:rule="has(oldSelf.resourceTags) == has(self.resourceTags)",message="resourceTags may only be configured during installation" | ||
| type AWSPlatformStatus struct { | ||
| // region holds the default AWS region for new AWS resources created by the cluster. | ||
| Region string `json:"region"` | ||
|
|
@@ -545,6 +546,7 @@ type AWSPlatformStatus struct { | |
| // AWS supports a maximum of 50 tags per resource. OpenShift reserves 25 tags for its use, leaving 25 tags | ||
| // available for the user. | ||
| // +kubebuilder:validation:MaxItems=25 | ||
| // +kubebuilder:validation:XValidation:rule="self.all(x, x in oldSelf) && oldSelf.all(x, x in self)",message="resourceTags are immutable and may only be configured during installation" | ||
| // +listType=atomic | ||
| // +optional | ||
| ResourceTags []AWSResourceTag `json:"resourceTags,omitempty"` | ||
|
|
@@ -581,9 +583,11 @@ type AWSResourceTag struct { | |
| // 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 '@'. | ||
| // Key must not start with 'aws:'. | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=128 | ||
| // +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 (' '), '_', '.', '/', '=', '+', '-', ':', '@'" | ||
| // +kubebuilder:validation:XValidation:rule=`!self.startsWith('aws:')`,message="the prefix 'aws:' is reserved for AWS system usage and cannot be used at the beginning of a key" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to consider casing on the AWS prefix?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my opinion: no. The docs say literally |
||
| // +required | ||
| Key string `json:"key"` | ||
| // value sets the value of the AWS resource tag key-value pair. Value is required when defining an AWS resource tag. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1387,6 +1387,7 @@ spec: | |
| 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 '@'. | ||
| Key must not start with 'aws:'. | ||
| maxLength: 128 | ||
| minLength: 1 | ||
| type: string | ||
|
|
@@ -1396,6 +1397,9 @@ spec: | |
| space (' '), '_', '.', '/', '=', '+', '-', ':', | ||
| '@' | ||
| rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$') | ||
| - message: the prefix 'aws:' is reserved for AWS system | ||
| usage and cannot be used at the beginning of a key | ||
| rule: '!self.startsWith(''aws:'')' | ||
|
Comment on lines
+1400
to
+1402
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: Yes — CRD CEL uses RE2 regex, so you can make matches() case‑insensitive either by:
References: Kubernetes CEL docs (regex/matches and regex library). [1] RE2 docs showing (?i) case‑insensitive flag. [2] Sources 🌐 Web query:
💡 Result: Short answer:
Sources:
Harden reserved-prefix check to be case-insensitive. Block all case variants like "AWS:", "Aws:", etc. The Apply: - rule: '!self.startsWith(''aws:'')'
+ rule: '!self.matches(''(?i)^aws:'')'🤖 Prompt for AI Agents |
||
| value: | ||
| description: |- | ||
| value sets the value of the AWS resource tag key-value pair. Value is required when defining an AWS resource tag. | ||
|
|
@@ -1419,6 +1423,10 @@ spec: | |
| maxItems: 25 | ||
| type: array | ||
| x-kubernetes-list-type: atomic | ||
| x-kubernetes-validations: | ||
| - message: resourceTags are immutable and may only be configured | ||
| during installation | ||
| rule: self.all(x, x in oldSelf) && oldSelf.all(x, x in self) | ||
|
Comment on lines
+1426
to
+1429
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Array immutability can be bypassed via duplicates; add size equality Current rule compares membership only; Apply this diff: x-kubernetes-validations:
- - message: resourceTags are immutable and may only be configured during installation
- rule: self.all(x, x in oldSelf) && oldSelf.all(x, x in self)
+ rule: size(self) == size(oldSelf) &&
+ self.all(x, x in oldSelf) &&
+ oldSelf.all(x, x in self)
🤖 Prompt for AI Agents |
||
| serviceEndpoints: | ||
| description: |- | ||
| serviceEndpoints list contains custom endpoints which will override default | ||
|
|
@@ -1447,6 +1455,9 @@ spec: | |
| type: array | ||
| x-kubernetes-list-type: atomic | ||
| type: object | ||
| x-kubernetes-validations: | ||
| - message: resourceTags may only be configured during installation | ||
| rule: has(oldSelf.resourceTags) == has(self.resourceTags) | ||
| azure: | ||
| description: azure contains settings specific to the Azure infrastructure | ||
| provider. | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1190,6 +1190,7 @@ spec: | |||||||||||||
| 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 '@'. | ||||||||||||||
| Key must not start with 'aws:'. | ||||||||||||||
| maxLength: 128 | ||||||||||||||
| minLength: 1 | ||||||||||||||
| type: string | ||||||||||||||
|
|
@@ -1199,6 +1200,9 @@ spec: | |||||||||||||
| space (' '), '_', '.', '/', '=', '+', '-', ':', | ||||||||||||||
| '@' | ||||||||||||||
| rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$') | ||||||||||||||
| - message: the prefix 'aws:' is reserved for AWS system | ||||||||||||||
| usage and cannot be used at the beginning of a key | ||||||||||||||
| rule: '!self.startsWith(''aws:'')' | ||||||||||||||
|
Comment on lines
+1203
to
+1205
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: Short answer: You may not use the "aws:" prefix in any user-defined tag key or value in any case (e.g., "aws:", "AWS:", "Aws:", etc. are all reserved). In general tag keys and values are case‑sensitive (though some IAM resource tags have special case rules). [1][2] Sources:
🌐 Web query:
💡 Result: Short answer: AWS reserves the aws: prefix case‑insensitively. You cannot create/edit/delete tag keys or values that begin with aws: in any case variant (e.g., aws:, AWS:, Aws:, aWs:, etc.); those tags are reserved for AWS, won’t count toward tag quotas, and are protected. Tag keys otherwise remain case‑sensitive. [1][2] Sources:
🌐 Web query:
💡 Result: No — AWS requires each tag key to be unique per resource. If you add a tag with an existing key on the same resource, the new value overwrites the old value (duplicate keys are not allowed). [1][2][3] Sources:
Update validation rule to enforce AWS's case-insensitive prefix restriction. AWS reserves the - rule: '!self.startsWith(''aws:'')'
+ rule: '!self.lowerAscii().startsWith(''aws:'')'This ensures the validation aligns with AWS's actual tag restrictions. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| value: | ||||||||||||||
| description: |- | ||||||||||||||
| value sets the value of the AWS resource tag key-value pair. Value is required when defining an AWS resource tag. | ||||||||||||||
|
|
@@ -1222,6 +1226,10 @@ spec: | |||||||||||||
| maxItems: 25 | ||||||||||||||
| type: array | ||||||||||||||
| x-kubernetes-list-type: atomic | ||||||||||||||
| x-kubernetes-validations: | ||||||||||||||
| - message: resourceTags are immutable and may only be configured | ||||||||||||||
| during installation | ||||||||||||||
| rule: self.all(x, x in oldSelf) && oldSelf.all(x, x in self) | ||||||||||||||
| serviceEndpoints: | ||||||||||||||
|
Comment on lines
+1229
to
1233
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Immutability rule can be bypassed via duplicates; add size equality
- rule: self.all(x, x in oldSelf) && oldSelf.all(x, x in self)
+ rule: size(self) == size(oldSelf) &&
+ self.all(x, x in oldSelf) &&
+ oldSelf.all(x, x in self)🤖 Prompt for AI Agents |
||||||||||||||
| description: |- | ||||||||||||||
| serviceEndpoints list contains custom endpoints which will override default | ||||||||||||||
|
|
@@ -1250,6 +1258,9 @@ spec: | |||||||||||||
| type: array | ||||||||||||||
| x-kubernetes-list-type: atomic | ||||||||||||||
| type: object | ||||||||||||||
| x-kubernetes-validations: | ||||||||||||||
| - message: resourceTags may only be configured during installation | ||||||||||||||
| rule: has(oldSelf.resourceTags) == has(self.resourceTags) | ||||||||||||||
| azure: | ||||||||||||||
| description: azure contains settings specific to the Azure infrastructure | ||||||||||||||
| provider. | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1387,6 +1387,7 @@ spec: | |
| 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 '@'. | ||
| Key must not start with 'aws:'. | ||
| maxLength: 128 | ||
| minLength: 1 | ||
| type: string | ||
|
|
@@ -1396,6 +1397,9 @@ spec: | |
| space (' '), '_', '.', '/', '=', '+', '-', ':', | ||
| '@' | ||
| rule: self.matches('^[0-9A-Za-z_.:/=+-@ ]+$') | ||
| - message: the prefix 'aws:' is reserved for AWS system | ||
| usage and cannot be used at the beginning of a key | ||
| rule: '!self.startsWith(''aws:'')' | ||
| value: | ||
|
Comment on lines
+1401
to
1403
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainConfirm case sensitivity of the reserved prefix. Rule forbids only lowercase 🌐 Web query: 💡 Result: AWS reserves the prefix in a case-insensitive way — you must not use "aws:", "AWS:", or any upper/lowercase combination as a tag key prefix. [1][2] Sources:
References: Fix case-insensitive handling of AWS reserved tag prefix. The rule currently forbids only lowercase 🤖 Prompt for AI Agents |
||
| description: |- | ||
| value sets the value of the AWS resource tag key-value pair. Value is required when defining an AWS resource tag. | ||
|
|
@@ -1419,6 +1423,10 @@ spec: | |
| maxItems: 25 | ||
| type: array | ||
| x-kubernetes-list-type: atomic | ||
| x-kubernetes-validations: | ||
| - message: resourceTags are immutable and may only be configured | ||
| during installation | ||
| rule: self.all(x, x in oldSelf) && oldSelf.all(x, x in self) | ||
| serviceEndpoints: | ||
| description: |- | ||
| serviceEndpoints list contains custom endpoints which will override default | ||
|
|
@@ -1447,6 +1455,9 @@ spec: | |
| type: array | ||
| x-kubernetes-list-type: atomic | ||
| type: object | ||
| x-kubernetes-validations: | ||
| - message: resourceTags may only be configured during installation | ||
| rule: has(oldSelf.resourceTags) == has(self.resourceTags) | ||
| azure: | ||
| description: azure contains settings specific to the Azure infrastructure | ||
| provider. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add a ratcheting test (see readme in the tests dir) to prove that existing values are not affected by this change. You should show that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I added the ratcheting validation tests, but am stuck on the first one (the other two work ok). In order to allow existing bad values, I updated the validation in 34f8d72 but apparently this is invalid, as the tests fail with:
I see in these k8s docs that this means the rule cannot be applied:
I'm not sure how to resolve this issue. Any insight?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JoelSpeed claude helped me figure out that I needed to change ResourceTags to listType=map, but now the actual validation of the aws: prefix does not seem to be working
Stuck on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and side-stepped the issue by validating for immutability. IMHO that makes more sense, as post-install updates are not supported. On the other hand, I'm not sure if there are issues with making a field immutable post-GA; the intention was always for these fields to be immutable, it just wasn't validated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need a ratcheting test to show that anyone who already has a value with
aws:in the prefix won't be broken by this changeUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, that makes sense: so we don't brick the rest of the aws platform status from being updated. Because I made resourceTags immutable, we can't cover the three expected cases in your original message. Does the check added in d582954 look good/sufficient? Or what else would you have in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that covers it, thanks