OCPBUGS-44199: Allow spaces in AWS tags

[patrickdillon]

An incorrect regex validation prevents users from specifying AWS tag keys or values that include spaces, which are
allowed by AWS's official regex:

https://docs.aws.amazon.com/directoryservice/latest/devguide/API_Tag.html

This bug is affecting users, so it would be good to backport to at least 4.17.

[openshift-ci]

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-ci-robot]

@patrickdillon: This pull request references Jira Issue OCPBUGS-44199, which is invalid:

• expected the bug to target either version "4.19." or "openshift-4.19.", but it targets "4.18.0" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

An incorrect regex validation prevents users from specifying AWS tag keys or values that include spaces, which are
allowed by AWS's official regex:

https://docs.aws.amazon.com/directoryservice/latest/devguide/API_Tag.html

This bug is affecting users, so it would be good to backport to at least 4.17.

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]

/jira refresh

[openshift-ci-robot]

@patrickdillon: This pull request references Jira Issue OCPBUGS-44199, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target version (4.19.0) matches configured target version for branch (4.19.0)
• bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (yunjiang@redhat.com), skipping review request.

In response to this:

/jira refresh

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.

[barbacbd]

/lgtm

[openshift-ci]

New changes are detected. LGTM label has been removed.

[patrickdillon]

@everettraven @JoelSpeed I think the API changes are complete, but I am blocked trying to add tests. I have added a commit with some of the tests I have been experimenting with, but here is what I have found:

1. It seems that specifying a featuregate is required. If I don't specify a featuregate, the tests fail:

the following fields were expected to match but did not:
  [(status.platformStatus.aws.cloudLoadBalancerConfig/status.platformStatus.aws.cloudLoadBalancerConfig)]
  Expected
      <*unstructured.Unstructured | 0xc00008c800>: {
          Object: {
....

Not sure what I should do here, as there is no associated feature gate. Right now I've added an arbitrary feature gate to get the one test I can write to pass. This seems like a bug.

2. Another issue is that when I test the aws:- prefix validation (expected error) with onCreate, I get the same error. So next I try testing with onUpdate. If I put the aws:key key in the initial config, the test fails:

  �[38;5;9m[FAILED] initial object status should update successfully
   ...
              Message: "Infrastructure.config.openshift.io \"test-xw97d\" is invalid: status.platformStatus.aws.resourceTags[0].key: Invalid value: \"string\": the prefix 'aws:' is reserved for AWS system usage and cannot be used at the beginning of a key",

This error message actually demonstrates the validation seems to be working., I just can't figure out a test to confirm it. When I try to update the test so that the bad key is only used during updated, no error is returned. I wonder if this has something to do with how for this object, the values in question are in the status and not the spec?

Please LMK what you think and next steps.

[JoelSpeed]

It seems that specifying a featuregate is required. If I don't specify a featuregate, the tests fail:

Can you push the tests as you think they should look, and we can verify the formatting. Typically, tests that aren't gated, should be run from the AAA_ungated test file. Otherwise they should be in a file named after the gate.

Another issue is that when I test the aws:- prefix validation (expected error) with onCreate, I get the same error. So next I try testing with onUpdate. If I put the aws:key key in the initial config, the test fails:

For status tests, we typically use onUpdate tests, have a look at the readme in the tests dir and it explains this.
You create an initial state, with some base status, then updatedis used to add the field for the validation you want to test, and the you canexpectedStatusError` to check against an error in updating the status subresource

[patrickdillon]

Thanks Joel! You cleared up both of my issues. As you suggested, the problem for the second part was that I was using expectedError instead of expectedStatusError 🤦

All feedback has been addressed so this should be ready for another look.

[everettraven]

A few more things. Other than this, I think the changes look good.

[patrickdillon]

Two steps forward, one step back...

Not sure what to do about these test failures for the cloudLoadBalancerConfig...

[patrickdillon]

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

[patrickdillon]

@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?

[JoelSpeed]

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

[patrickdillon]

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.

[JoelSpeed]

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

[patrickdillon]

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.

[JoelSpeed]

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 🤔

[everettraven]

API changes and test cases LGTM. Tagging in @JoelSpeed for a review.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: barbacbd, everettraven, patrickdillon
Once this PR has been reviewed and has the lgtm label, please assign bparees for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[JoelSpeed]

Since we are adding the new restriction for not starting with aws:, I'd like to see some ratcheting tests added that show that a value persisted that is incompatible with this validation does not negatively affect writes to the rest of the object.

You can see an example of how I'm doing that elsewhere in #2142

You'd have to remove the XValidation, persist an invalid value, and then try various updates to check that they still pass (or fail if appropriate)

[patrickdillon]

Since we are adding the new restriction for not starting with aws:, I'd like to see some ratcheting tests added that show that a value persisted that is incompatible with this validation does not negatively affect writes to the rest of the object.

I have created the test in a784650 but the validation does seem to fail on any updates to the object. I tried adding another resourceTag, adding a service endpoint,and finally just persisting the object as is (which is in the commit). All of these fail with

  �[38;5;9m[FAILED] unexpected error updating status
  Unexpected error:
      <*errors.StatusError | 0xc0043bd7c0>: 
      Infrastructure.config.openshift.io "test-crmjf" is invalid: status.platformStatus.aws.resourceTags[0].key: Invalid value: "string": the prefix 'aws:' is reserved for AWS system usage and cannot be used at the beginning of a key

AFAICT the tests are correct, so it seems like the new validation could be breaking updates?

[everettraven]

@patrickdillon I had missed that this was a field on the status subresource so you are likely running into a bug that openshift/kubernetes#2167 is supposed to fix

[openshift-ci]

@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/integration	a784650	link	true	/test integration
ci/prow/verify	a784650	link	true	/test verify
ci/prow/okd-scos-e2e-aws-ovn	a784650	link	false	/test okd-scos-e2e-aws-ovn

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.