CFE-581 : Update Infrastructure CRD to support azure tags in status

[bharath-b-rh]

PR is for adding new field resourceTags in platformStatus for azure platform type, which will be used by in-cluster operators for adding the tags to the created azure resources.

[openshift-ci]

Hello @bharath-b-rh! 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.

For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard lgtm and approved labels this repository requires either:

bugzilla/valid-bug - applied if your PR references a valid bugzilla bug

OR

qe-approved, docs-approved, and px-approved - these labels can be applied by anyone in the openshift org via the /label <labelname> command.

Who should apply these qe/docs/px labels?

• For a no-Feature-Freeze team who is merging a feature before code freeze, they need to get those labels applied to their api repo PR by the appropriate teams (i.e. qe, docs, px)
• For a Feature Freeze (traditional) team who is merging a feature before FF, they can self-apply the labels (via /label commands), they are basically irrelevant for those teams
• For a Feature Freeze team who is merging a feature after FF, the PR should be rejected barring an exception

[bharath-b-rh]

/cc @JoelSpeed

[bharath-b-rh]

/label tide/merge-method-squash

[bharath-b-rh]

/unhold

[bharath-b-rh]

Thank you @JoelSpeed for the comments!!

[JoelSpeed]

Could you please squash the commits manually? We are not supposed to use merge-method-squash in OpenShift as it causes issues copying repos to the ART build system

[bharath-b-rh]

/remove-label tide/merge-method-squash

[JoelSpeed]

I think some changes have happened since we had discussion about whether to include this in the spec or status.
Before we merge this, I'd like to understand what is happening with the AWS tagging updates enhancement, whether that is ever going to be implemented or whether it's not possible to implement it.
If it's not possible, and we think the same is true for Azure, then we must revert the AWS enhancement update and continue with this in the status. If we think it will eventually be implemented, I'd like to reopen the discussion about putting this in the spec so that we can allow users to update the tags on Azure in the future, without the API pain that the AWS platform is having to deal with

[JoelSpeed]

CC @TrilokGeer could you please weigh in with your thoughts related to my above comment

[TrilokGeer]

The AWS tags update enhancement is presently not considerable as is due to inherent problems resulting in some resources not being tagged with latest updates. Because of this limitation, infrastructure can need not have spec fields, but related controller managing the resource would have override spec respectively, similar to machineset.

[bharath-b-rh]

commit addresses an issue reported during pre-merge tests, where it was found DNS Zone resource allows to add tag only if key contains letters, numbers, underscores, hyphens and periods and start with letter, end with letter, number or an underscore.

openshift/enhancements#1329 is created for updating the enhancement with the observations/changes made during the course of implementation.

[bharath-b-rh]

/cc @JoelSpeed

[JoelSpeed]

I've discussed with @deads2k and we've concluded that this field should reside on the status for now. If we ever need to evolve this API to allow modification, you will need a controller to own the spec field, validate the input and then update the status field if the platform accepts the update. Consumers should always consume from the status field, as these are the accepted values.

If I understand correctly, the initial intention is that this field is supposed to be immutable? We should enforce that immutability with CEL validation, please check the example API (example/v1) immutableField for an example of that

[bharath-b-rh]

I've discussed with @deads2k and we've concluded that this field should reside on the status for now. If we ever need to evolve this API to allow modification, you will need a controller to own the spec field, validate the input and then update the status field if the platform accepts the update. Consumers should always consume from the status field, as these are the accepted values.

If I understand correctly, the initial intention is that this field is supposed to be immutable? We should enforce that immutability with CEL validation, please check the example API (example/v1) immutableField for an example of that

CEL validation would not be required, because we can't modify status sub-resource, am I correct?

[JoelSpeed]

CEL validation would not be required, because we can't modify status sub-resource, am I correct?

No, that's no longer true. Kubectl added a subresource flag in 1.24 that allows users to edit the status of objects directly, so CEL validation should be added

[bharath-b-rh]

PR was marked closed while resolving conflicts and removing the duplicate commits.

[JoelSpeed]

Apologies, noticed another couple of small bits

[JoelSpeed]

API looks good for me, so we can make sure all the relevant PRs are updated

It would be good to get some integration tests to test the validations, are you familiar with how we are testing those? There's files ending .testsuite.yaml in the folder, which have some tests that can be written, more details here

I'd like to test the creation and immutability as well as the string validation patterns please

[JoelSpeed]

/approve

I'd like to see some pre-merge testing on the installer PR before we merge this to make sure the flow of install works correctly and that the tags modification works as expected. Please ask whoever QEs this for the installer to also add a QE approved label on this PR

[jinyunma]

pre-merge testing is passed, details see [comments in CFE-581](https://issues.redhat.com/browse/CFE-581?focusedCommentId=21682592&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-21682592).
/label qe-approved

[JoelSpeed]

/hold I left a comment on the QE validation, I don't think the API has been tested correctly as the edit was silently dropped, it should have returned an error

[JoelSpeed]

/hold cancel

[Senthamilarasu-STA]

/label px-approved

[xenolinux]

/label docs-approved

[JoelSpeed]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bharath-b-rh, JoelSpeed

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

The pull request process is described here

Details

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

[openshift-ci]

@bharath-b-rh: all tests passed!

Full PR test history. Your PR dashboard.

Details

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/test-infra repository. I understand the commands that are listed here.