Update enhancement proposal to support update of user-defined AWS tags

[TrilokGeer]

This PR addresses design considerations to support update of user-defined AWS tags.
Also, details are listed for graduation criteria for tech preview and GA.

[TrilokGeer]

/assign @tjungblu

[openshift-ci]

Hi @TrilokGeer. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[TrilokGeer]

This PR is in continuation to update based on review suggestions in : #1000

[JoelSpeed]

This is looking a look better, thanks for the updates!

It definitely needs some input from the installer team before it merges, might also be worth giving a heads up to the other teams this affects, ingress, cloud credential, storage and image registry. I can approve on behalf of the Machine API team

[JoelSpeed]

Might be worth noting that to remove the status tags, you need support to help you, right?

[TrilokGeer]

It is listed as one of the drawbacks.

[JoelSpeed]

I believe there was a comment before about trying to make limited to cluster level a bit clearer, can you expand on what this actually means within this sentence please

[TrilokGeer]

rephrased to make it less ambiguous

[JoelSpeed]

This is more of an implementation detail than a user story, we need to know why the user wants to create/update/delete tags in user stories here, what are the use cases, what will they achieve?

[TrilokGeer]

I think, it is better listed under limitations section.

[JoelSpeed]

Just as an example of how this user story might be improved

As a cluster adiministrator of OpenShift, I would like to be able to remove a centrally managed tag from resources created by the cluster so that I can prune old tags that are no longer required by my organisation

We don't need the extra context you've added here really, this should be part of the implementation details or goals

[TrilokGeer]

The extra content should be ok to hep understand why the user story is required as the previous version does not allow removal of tags.

[JoelSpeed]

The Go and yaml blocks you have above should be under this title really, this is where API reviewers jump to when reviewing enhancements

[TrilokGeer]

Correct, updated the same.

[TrilokGeer]

cc @Miciah @sinnykumari @dmage @staebler @patrickdillon @tkashem , can you please give a read on behalf of respective components to help check for any component-specific caveats?

[staebler]

This is looking much nicer. Thanks for the persistence on this, @TrilokGeer.

[staebler]

Are we certain that there is no use case for a user wanting a tag with an empty value on a resource?

[TrilokGeer]

Generally, the usecase that I am aware of is, cluster admins tend to set tags with default value set to empty string during creation of resources. Later based on the usecase, the values are updated in these tags or set to empty string. The OpenShift installer fails on validation for using empty strings.

@staebler ,

1. do you suggest to keep the behaviour same with updates? i.e, fail validation on empty strings?
2. is it better to consider value as an optional field and consider "nil"/null value field for deletion?
   In case of cloud providers, the value of a tag can be set to empty string but cannot be null.

[staebler]

My suggestion would be to separate the tags to delete entirely from the tags to maintain. That way it is clearer to the user and less likely from them to experience surprises.

[TrilokGeer]

Do you think it is better to delete tags on AWS resources when removed from spec? This behavior will be consistent with usual CRUD-like object handling.

As tag update/delete can be overridden by entries in kube resource objects, making tags "unmanaged" wiithout deleting requires tricky methods to handle delete and maintain the limits for the number of tags. It is better to NOT support the following user story.

- As a cluster administrator of OpenShift, I would like to be able to remove user-defined tags from `.spec.platformSpec.aws.resourceTags` without deleting for AWS resources.
  This enables, user-defined tags to be modified on AWS without being overridden by OpenShift operators. The user can adopt new 3rd party component to manage tags.

@JoelSpeed @staebler , Do you foresee any usecase that might require the above user story? Third-party tools operate without any OpenShift specific awareness of tag management, the user story does not seem to add much benefit. In this case, user would be required to delete the tag from Openshift and re-tag AWS resources using the third-party tools.

[JoelSpeed]

Do you think it is better to delete tags on AWS resources when removed from spec?

This sounds incredibly hard to track, unless you want to ensure that the only tags on the resource are those applied by OpenShift, in which case we can delete any that are extra on the AWS side. But that's not how things work today, and I'm not sure that's what we want to do here?

I think @staebler's suggestion is good, if we want to be able to support users removing tags, there should be a list of tags to be removed, that would be clearer from a UX perspective than the current empty string approach

[staebler]

I agree with Joel. I don't think that we can support removal of AWS tags when a tag is removed from the Infrastructure's resourceTags.

[TrilokGeer]

Yes, from UX perspective, it definitely helps user if there is more detailed and discrete tag information.

Following reasons user may find different lists confusing

1. There is no deterministic time to remove the entry from delete list.
2. Conflicting entries are possible which adds confusion to user if the delete list tag entry is not removed.
3. Conflict scenarios with override is possible. e.g: One of the scenarios is, when the kube resource overriding entry is removed and there is no entry in maintain list and delete list.

The following might help user to track tag history or requests,

1. For tag history, velero backups can be used to save.
2. For monitoring operations on tags, events are generated for tag add/update/delete requests.
   Also, based on specific events, alerts can be triggered.
3. For monitoring tag changes on AWS resource, AWS CloudWatch Events can be explored.

It is less complex to remove entry from spec for deletion and let user opt for one of the above methods to track tags.

@JoelSpeed, Do you think it is ok to go ahead with the above methods for tracking tag?

[TrilokGeer]

Based on the discussion with @JoelSpeed and @staebler , the change is going to be ambiguous to the user. It may also involve versioning the tags. User can opt to delete the tags using external tools.

[staebler]

I would like to see an overview of how we would expect a user to go about updating non-managed AWS resources. I expect that we are going to need to provide guidance in the docs anyway.

[TrilokGeer]

For specific to AWS, tag editor can be used for updating non-managed AWS resources https://docs.aws.amazon.com/ARG/latest/userguide/tag-editor.html . Is it required to document on external vendor tools?

[tkashem]

/assign @p0lyn0mial @sanchezl

[JoelSpeed]

You need to explain what happens when a tag is removed within this comment. Think of this block as user facing documentation.

[TrilokGeer]

Updated the api extensions .

[JoelSpeed]

As OpenShift reserves 5, should this not be MaxItems=5?

[TrilokGeer]

the MaxItems includes both Openshift and user tags as a combined limit. Specific validations are updated in installer platform validation checks https://github.com/staebler/installer/blob/d5016a08312a971130ab8ea9377da5d5bcc7ec88/pkg/types/aws/validation/platform.go#L51

@staebler is there a doc link which can be referred for the reservations?

[staebler]

I do not know of any. OpenShift uses the kubernetes.io/cluster/... tag on all resources. Some operators may add other tags. The 5 reservations is for future use so that we could add more tags if needed.

[JoelSpeed]

the MaxItems includes both Openshift and user tags as a combined limit.

Can you explain why it includes both? At the moment, as a user, I could add 10 tags here and go over the limit of 5 that I'm allowed. I think what we want to do is limit this list to 5 items which allows the operators to add their own tags (eg the cluster tag and name tags) without going over the 10 tag limit.

[JoelSpeed]

This still needs to be fixed, it should be 5 items

[LalatenduMohanty]

Does it also include resources created or used by OLM operators? I guess it is not. We should clearly mention that we are not trying to track resources from OLM operators.

[LalatenduMohanty]

Also saw User-defined tags cannot be updated to an AWS resource which is not managed by an operator. In this proposal, the changes proposed and developed will be part of openshift-* namespace. External operators are not in scope , so please this to the non goals

[LalatenduMohanty]

s/in an atomic operation/as an atomic operation/

[TrilokGeer]

Thanks for the review, updated the EP accordingly.

[LalatenduMohanty]

When you say the tags can be updated post creation, who will be updating those ? I am assuming some controller will update those? Because if we let user edit these tags then it would be hard to keep track of these.

[staebler]

The controller that owns the AWS resource would be the one that updates the tags on the AWS resource, on behalf of a spec change by the user to either the Infrastructure resource or the kube resource for the AWS resource.

[LalatenduMohanty]

Sounds good to me.

[LalatenduMohanty]

Also saw User-defined tags cannot be updated to an AWS resource which is not managed by an operator. In this proposal, the changes proposed and developed will be part of openshift-* namespace. External operators are not in scope , so please this to the non goals

[staebler]

LGTM

[dmage]

Should operators monitor created AWS resources and stomp manual changes of their tags?

[staebler]

Should operators monitor created AWS resources and stomp manual changes of their tags?

The expectation of an operator is that it will eventually stomp manual changes. The operator is not expected to actively monitor the AWS resources. The operator would be expected to check the tags on the AWS resources as part of its regular validation that the AWS resource is aligned with the spec of the kube resource.

See Caveat (1). https://github.com/openshift/enhancements/pull/1051/files#diff-9e7c9d80b69a46de2319af1610f54e1c0f04896901283d2e878bef4ecf915240R227

[frobware]

LGTM

[dmage]

So .status.platformStatus.aws.resourceTags will have some stale values on upgraded clusters and will be empty on new clusters?

[TrilokGeer]

Correct.

[sanchezl]

Negative look-ahead (?! ... ) is not supported by the Go standard regexp library. OpenShift provides a CRD admission plugin library that can be used to validate this field.

[TrilokGeer]

Ok, updated the same in openshift/api#1064.

[openshift-ci]

@TrilokGeer: all tests passed!

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

[sanchezl]

lgtm

[akhil-rane]

lgtm

[tjungblu]

Glad to see this is finally done. Thanks for your review folks, and thanks for your persistence @TrilokGeer.

/lgtm
/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sanchezl, tjungblu

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 [tjungblu]

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

[bparees]

these two values are the same. should the first one be "value_compupdate1" and the second one "value_infraupdate1"?

[staebler]

It is intentional that the two values are the same. This scenario is the following.

There is already a tag with the same key and value present for AWS resource

This scenario is demonstrating that adding the tag to the Infrastructure resource will not cause an update of the AWS resource when the AWS resource already has the matching tag.

We could argue whether this scenario is an interesting enough scenario to describe for a level-driven system.

[jewzaam]

What about a new property like .spec.platformSpec.aws.resourceTagsDelete where user enumerates tags specifically to be deleted? If there's something set on a kube resource the deletion request is ignored for that resource.

I worry we'll get to a point somewhere in the future where we need to change how we tag resources for ROSA. If we have to build a tool to delete tags it negates the value of this enhancement.

[bparees]

@deads2k sketched out an api that would allow for deletion as:

spec.tags -- this is the current user desire to create.
spec.orphanTagsOnDeletion -- this is a list of tags for the platform to stop managing and not remove
status.tags -- this is the list of tags for the platform to create
status.tagsToRemove -- this is the list of tags for the platform to remove

a user adds to spec.tags and it is vetted and moved to status.tags

a user removes from spec.tags, it is compared against status.tags, and if there is no entry in orphanedTags, it is removed from status.tags and added to status.tagsToRemove.  if there is a matching entry in orphanTags, it is simply removed from status.tags

[jewzaam]

@bparees is that something to be addressed in another enhancement or just dropped? I don't see delete handled in this enhancement hence my comment. Deletion with logic to identify what should be managed by the platform vs external tools and having the ability to transition to an external tool is what is keeping delete from being included, from what I can tell. If we then step away from having any logic and leave it to users to explicitly state what tags need deleted then the decision on what to keep vs delete is left to the administrator.

[bparees]

@bparees is that something to be addressed in another enhancement or just dropped?

neither, it's something that came up for discussion after this EP update had merged, and when the api changes were being reviewed. I pasted it here so you could have some idea of how delete would work, if we were to include it as a capability and see what thoughts you'd have on it if that was the approach we took.

[jewzaam]

Thanks. Seems a reasonable approach, just inverse of my initial post. But the key thing is giving the control to the user with explicit list of tags to orphan. Sure.

[bparees]

@sdodson @deads2k I had a conversation w/ SD about their needs/expectations and in general how we make this an api that an admin won't be surprised by the behavior of.

one of the thoughts would be rather than having a single "tags" abstraction field that:

1. will only update tags on some resources (ones we have active operator management of)
2. has to be constrained by the tag limit of the lowest resource tag limit (10)
3. needs to compete w/ machine spec which considers itself the only owner of ec2 tags, if you provide tags in that api (though possibly this could be changed)

we should instead break this out into specific api fields for each resource type we can actually manage the tags of (and no resource field for the ones we cannot).

it would make for a bit of an ugly api (and beg the next question of: why not just manage the tags via a config resource for each operator, which we already do in some cases, rather than specify them here where it's an action at a distance thing)

Another thing we discussed(possibly independent of that api shape decision) would be whether the operators that actually manage the resources should go degraded if they can't apply the requested tags. This would at least give some feedback to the admin that the changes they tried to assert via the api did not get applied. Unfortunately it's "action at a distance". It would be better if the infrastructure resource reported the errors back, but introducing that sort of two way dependency between whatever manages the infra resource object, and whatever manages the aws resources, doesn't feel great to me.

thoughts? Fundamentally I think it's important that if we're going to provide a tag-updating api interface, it be obvious to the users of that interface what aws resources will/won't be updated and what, in general, the limitations of that api are. Otherwise they are going to add tags and then be surprised when some resources get the tags and others don't. Which makes me think a generic "tag your aws resources via this field" api(regardless of how it is documented) is problematic.

ideas?

[sdodson]

That sounds like a pretty horrible user experience. What became of the approach that we set one tag, everywhere we can, and then additional tags are managed outside of OpenShift however you prefer?

[bparees]

What became of the approach that we set one tag, everywhere we can, and then additional tags are managed outside of OpenShift however you prefer?

it's all still on the table, but the problem w/ that approach is that it's not obvious when interacting with the api, which resources will get the new/updated tag and which will not. It requires reading the documentation, assuming the documentation is correct and exhaustive to understand if a resource falls into the category of:

1. we can fully manage/update the tags
2. we can create the tags at install time, but we can't update tags
3. we can't manage the tags at all (not even create)
4. we can manage the tags as long as you don't exceed N of them, so even though you'd normally expect this tag you added to get applied to X, it didn't because X was over its tag limit, while Y did get the tag because it's not over the limit.

a single "put your tag desires here" api field doesn't get us there, not without at least a related "status" field that tells you what you actually got. And maintaining a status field on the infra resource will be difficult since it needs to go ask each operator that manages a resource what tags the operator has actually applied to the resources. (infra resource becomes an aggregation over the various resource-managing operators, for resources that have an operator that manages them)

[sdodson]

I meant we stop allowing admin defined tags entirely. Instead the cluster manages its one cluster unique tag and all supplemental tags are no longer the cluster's responsibility, something external is responsible for finding resources tagged kubernetes.io/.../cluster=owned and then managing supplemental tags if possible.

[bparees]

ah, yeah i sort of pitched that to SD. They weren't a huge fan (basically punting tooling to them, and i think there were some more explicit concerns about how it would be done), but I think it's technically still on the table, depending on what SD and customers come back and tell us are their actual requirements.

@jewzaam @jharrington22 can better speak to their thoughts/concerns on that approach, though.

[jewzaam]

@jharrington22 and I talked over this and we'd like to keep tagging as a part of OCP. Not having delete is fine initially. At some point it's possible we care about this. Our requirement is we can set a tag on all resources created by the installer and created by OCP after installation. We require a single tag and it's the same on all resources. Anything beyond that is a customer use case and I'm personally OK with not tackling those at this time. Having a tag set on all resources will allow us to create policies limiting permissions in the customer AWS account to just the cluster created/managed resources.