no-jira: Validate AWS resource tag keys with aws: prefix #2183

patrickdillon · 2025-01-30T19:17:24Z

AWS docs indicate that tag keys cannot be prefix with aws:. See:
https://docs.aws.amazon.com/directoryservice/latest/devguide/API_Tag.html

Using this key prefix leads to an AWS API error indicating the prefix
is reserved for AWS system usage. This commit adds API validation
and ratecheting tests, as the key was previously allowed.

This was originally included in #2124 but cannot land due to a bug addressed by openshift/kubernetes#2167, so

Depends on #2124
Depends on openshift/kubernetes#2167

openshift-ci-robot · 2025-01-30T19:17:28Z

@patrickdillon: This pull request explicitly references no jira issue.

In response to this:

AWS docs indicate that tag keys cannot be prefix with aws:. See:
https://docs.aws.amazon.com/directoryservice/latest/devguide/API_Tag.html

Using this key prefix leads to an AWS API error indicating the prefix
is reserved for AWS system usage. This commit adds API validation
and ratecheting tests, as the key was previously allowed.

This was originally included in #2124 but cannot land due to a bug addressed by openshift/kubernetes#2167, so

Depends on #2124
Depends on openshift/kubernetes#2167

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

patrickdillon · 2025-01-30T19:17:34Z

/hold
for dependencies

openshift-ci · 2025-01-30T19:17:35Z

Hello @patrickdillon! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

openshift-bot · 2025-08-14T01:00:26Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

JoelSpeed · 2025-09-05T14:06:37Z

@patrickdillon Did you intend to move this PR forward?

patrickdillon · 2025-09-15T18:12:49Z

@patrickdillon Did you intend to move this PR forward?

Yes. I see now the dependent work has merged. So I can rebase this; I am tied up with a conference this week so I will try to get this once I'm back next week.

JoelSpeed · 2025-10-09T14:31:45Z

@patrickdillon Just a reminder to move this one along when you have a moment

patrickdillon · 2025-10-09T18:42:16Z

/remove-lifecycle stale

patrickdillon · 2025-10-09T18:56:31Z

/hold cancel

@JoelSpeed finally ready, thanks!

JoelSpeed · 2025-10-13T09:31:14Z

config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yaml

                value: value*
            type: AWS
      expectedStatusError: "invalid AWS resource tag value. The string can contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', '@'"
+    - name: Should not be able to create an aws resourcetag with aws prefix in key


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

Existing entries persist when a new item is added to the list

Additional bad entries are not allowed when there is an existing bad entry

The bad entry can be updated to a valid entry

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:

Invalid value: "self == oldSelf || !self.startsWith('aws:')": oldSelf cannot be used on the uncorrelatable portion of the schema within spec.validation.openAPIV3Schema.properties[status].properties[platformStatus].properties[aws].properties[resourceTags]

I see in these k8s docs that this means the rule cannot be applied:

Errors will be generated on CRD writes if a schema node contains a transition rule that can never be applied, e.g. "oldSelf cannot be used on the uncorrelatable portion of the schema within path".

I'm not sure how to resolve this issue. Any insight?

@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

[config.openshift.io/v1, Resource=infrastructures][ClusterProfiles=Hypershift,SelfManagedHA][FeatureSet="Default"][FeatureGate=-AWSClusterHostedDNSInstall][File=0000_10_config-operator_01_infrastructures-Default.crd.yaml] Infrastructure On Update [It] Should not be able to create an aws resourcetag with aws prefix in key /Users/padillon/go/src/github.com/openshift/api/tests/generator.go:345 [FAILED] Expected an error, got nil In [It] at: /Users/padillon/go/src/github.com/openshift/api/tests/generator.go:321 @ 10/14/25 22:00:52.111

Stuck on this.

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.

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 change

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.

Yep, that covers it, thanks

config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yaml

patrickdillon · 2025-10-15T02:03:52Z

/hold

config/v1/types_infrastructure.go

patrickdillon · 2025-10-15T10:48:37Z

@JoelSpeed these fields are supposed to be immutable. Should i just add validation to enforce that?

patrickdillon · 2025-10-15T18:07:26Z

/hold cancel

config/v1/types_infrastructure.go

JoelSpeed · 2025-10-21T10:08:11Z

config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yaml

                value: value*
            type: AWS
      expectedStatusError: "invalid AWS resource tag value. The string can contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', '@'"
+    - name: Should not be able to create an aws resourcetag with aws prefix in key


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 change

JoelSpeed

/lgtm
/verified by new integration tests

JoelSpeed · 2025-10-22T09:58:56Z

config/v1/tests/infrastructures.config.openshift.io/AAA_ungated.yaml

                value: value*
            type: AWS
      expectedStatusError: "invalid AWS resource tag value. The string can contain only the set of alphanumeric characters, space (' '), '_', '.', '/', '=', '+', '-', ':', '@'"
+    - name: Should not be able to create an aws resourcetag with aws prefix in key


Yep, that covers it, thanks

openshift-ci-robot · 2025-10-22T10:00:19Z

Scheduling tests matching the pipeline_run_if_changed or not excluded by pipeline_skip_if_only_changed parameters:
/test e2e-aws-ovn
/test e2e-aws-ovn-hypershift
/test e2e-aws-ovn-hypershift-conformance
/test e2e-aws-ovn-techpreview
/test e2e-aws-serial-1of2
/test e2e-aws-serial-2of2
/test e2e-aws-serial-techpreview-1of2
/test e2e-aws-serial-techpreview-2of2
/test e2e-azure
/test e2e-gcp
/test e2e-upgrade
/test e2e-upgrade-out-of-change

openshift-ci · 2025-10-22T10:00:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

patrickdillon · 2025-10-23T17:00:49Z

/verified by e2e-aws

openshift-ci-robot · 2025-10-23T17:01:01Z

@patrickdillon: This PR has been marked as verified by e2e-aws.

In response to this:

/verified by e2e-aws

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2025-10-23T17:06:53Z

/retest-required

Remaining retests: 0 against base HEAD f4cd2bb and 2 for PR HEAD d582954 in total

openshift-ci-robot · 2025-10-23T21:06:52Z

/retest-required

Remaining retests: 0 against base HEAD 8691c30 and 1 for PR HEAD d582954 in total

openshift-ci-robot · 2025-10-27T15:27:50Z

/retest-required

Remaining retests: 0 against base HEAD 9a9f303 and 0 for PR HEAD d582954 in total

openshift-ci · 2025-10-27T15:53:43Z

@patrickdillon: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-serial-techpreview	`9ae0e4a`	link	true	`/test e2e-aws-serial-techpreview`
ci/prow/e2e-aws-serial	`9ae0e4a`	link	true	`/test e2e-aws-serial`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci-robot · 2025-10-27T16:27:59Z

/hold

Revision d582954 was retested 3 times: holding

AWS docs indicate that tag keys cannot be prefix with aws:. See: https://docs.aws.amazon.com/directoryservice/latest/devguide/API_Tag.html Using this key prefix leads to an AWS API error indicating the prefix is reserved for AWS system usage. This commit adds API validation and ratecheting tests, as the key was previously allowed. Adds validation to enforce immutability on AWS resourcetags, in the same manner as Azure & GCP. Updating resourcetags post-install is not supported.

Generated with PROTO_OPTIONAL=true make update

openshift-ci · 2025-11-07T19:29:31Z

New changes are detected. LGTM label has been removed.

patrickdillon · 2025-11-07T19:30:38Z

/hold cancel

@JoelSpeed I just noticed this never merged due to essentially a rebase conflict. I rebased and reran make update. PTAL

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 30, 2025

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 30, 2025

openshift-ci bot requested review from cheesesashimi and knobunc January 30, 2025 19:18

patrickdillon mentioned this pull request Jan 30, 2025

OCPBUGS-44199: Allow spaces in AWS tags #2124

Merged

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 31, 2025

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 14, 2025

patrickdillon force-pushed the aws-tag-prefix branch from 9ae0e4a to aee6105 Compare October 9, 2025 18:41

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2025

openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 9, 2025

openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 9, 2025

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2025

JoelSpeed reviewed Oct 13, 2025

View reviewed changes

patrickdillon commented Oct 15, 2025

View reviewed changes

config/v1/types_infrastructure.go Show resolved Hide resolved

patrickdillon force-pushed the aws-tag-prefix branch from 7219772 to 0eddb96 Compare October 15, 2025 18:06

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 15, 2025

JoelSpeed reviewed Oct 21, 2025

View reviewed changes

patrickdillon force-pushed the aws-tag-prefix branch 2 times, most recently from 8f7a956 to d582954 Compare October 21, 2025 18:05

JoelSpeed reviewed Oct 22, 2025

View reviewed changes

openshift-ci bot assigned JoelSpeed Oct 22, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2025

openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Oct 23, 2025

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 27, 2025

patrickdillon added 3 commits November 7, 2025 14:26

refactor validation and add ratcheting validation

dc25f93

Generate CRDs: for AWS prefix validation

a9187b1

Generated with PROTO_OPTIONAL=true make update

patrickdillon force-pushed the aws-tag-prefix branch from d582954 to a9187b1 Compare November 7, 2025 19:29

openshift-ci-robot removed the verified Signifies that the PR passed pre-merge verification criteria label Nov 7, 2025

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2025

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 7, 2025

no-jira: Validate AWS resource tag keys with aws: prefix #2183

Are you sure you want to change the base?

no-jira: Validate AWS resource tag keys with aws: prefix #2183

Uh oh!

Conversation

patrickdillon commented Jan 30, 2025

Uh oh!

openshift-ci-robot commented Jan 30, 2025

Uh oh!

patrickdillon commented Jan 30, 2025

Uh oh!

openshift-ci bot commented Jan 30, 2025

Uh oh!

openshift-bot commented Aug 14, 2025

Uh oh!

JoelSpeed commented Sep 5, 2025

Uh oh!

patrickdillon commented Sep 15, 2025

Uh oh!

JoelSpeed commented Oct 9, 2025

Uh oh!

patrickdillon commented Oct 9, 2025

Uh oh!

patrickdillon commented Oct 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickdillon Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patrickdillon commented Oct 15, 2025

Uh oh!

Uh oh!

patrickdillon commented Oct 15, 2025

Uh oh!

patrickdillon commented Oct 15, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Oct 22, 2025

Uh oh!

openshift-ci bot commented Oct 22, 2025

Uh oh!

patrickdillon commented Oct 23, 2025

Uh oh!

openshift-ci-robot commented Oct 23, 2025

Uh oh!

openshift-ci-robot commented Oct 23, 2025

Uh oh!

openshift-ci-robot commented Oct 23, 2025

Uh oh!

openshift-ci-robot commented Oct 27, 2025

Uh oh!

openshift-ci bot commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Oct 27, 2025

Uh oh!

openshift-ci bot commented Nov 7, 2025

Uh oh!

patrickdillon commented Nov 7, 2025

Uh oh!

patrickdillon Oct 21, 2025 •

edited

Loading

openshift-ci bot commented Oct 27, 2025 •

edited

Loading