Bug 1950113: config: add aws usertags to infrastructure #864

staebler · 2021-03-15T21:05:50Z

Add a new field userTags to .status.aws of the infrastructure.config.openshift.io type. Tags included in the userTags field will be applied to new resources created for the cluster.

https://issues.redhat.com/browse/CORS-1657

staebler · 2021-03-16T00:33:12Z

Would you mind giving this a review, Jeremiah?

/cc @jstuever

gregsheremeta · 2021-03-22T14:54:03Z

ping, ETA on this?

This is temporary; will drop when 864 merges.

JoelSpeed · 2021-03-23T15:37:33Z

config/v1/types_infrastructure.go

+	// userTags is a list of additional tags to apply to AWS resources created for the cluster. Changes made to userTags
+	// will not be reflected in existing AWS resources. The userTags are only applied to an AWS resource when the
+	// resource is created.


@alexander-demichev is this how it works upstream or do they reconcile as well? I think we will want to mimic their behaviour ideally so we can make the behaviour consistent across various openshift topologies

This also feels like we're implicitly saying we shouldn't change existing resources, and we're stating we won't. This might require special handling depending on the component in the future.

michaelgugino

We should have an enhancement for this.

michaelgugino · 2021-03-23T15:37:28Z

config/v1/0000_10_config-operator_01_infrastructure.crd.yaml

+                            created for the cluster.
+                          type: object
+                          properties:
+                            key:


What if users want to have different tags for the machine-api and other components? This API does not allow for this facility.

Also, having this be platform specific means we need special logic for each platform in the future. Why would we define centralized cluster tags for one platform and not another?

What if users want to have different tags for the machine-api and other components? This API does not allow for this facility.

The user still retains the capability of adding additional tags to specific machinesets/machines just like they do now. Those tags would be additive with the global tags, where the global tags have precedence.

Those tags would be additive with the global tags, where the global tags have precedence.

This is not stated in the CRD description and should be. Also, this runs contrary to how most config is typically applied (specific should override the general).

Also, since this mechanism is being tied IAM permissions in a particular use case, we need to make sure this mechanism is granular.

ricardomaraschini · 2021-03-23T16:16:15Z

I am giving my LGTM on this (looking from the internal image registry point of view).

gregsheremeta · 2021-03-23T20:18:15Z

previous discussion on a similar enhancement
#231 (comment)

Miciah · 2021-03-23T20:22:17Z

Should the new UserTags field be added to AWSPlatformStatus rather than to AWSPlatformSpec? If I understand correctly, the field is going to be set at installation time by the installer, and changing it later on will produce inconsistent results, so it seems more like a status field (implying it is set by the installer or a controller) than a spec field (implying it is set by the user).

If we added a status field now, that would make it easier to add a day-2 configurable later on, with more time to implement consistent behavior and have some controller validate the spec and copy it to status.

frobware · 2021-03-24T09:48:00Z

Where is validation being done for the key and values?
It would be better if this was not done on a component by component basis.

AWS docs call out the following tag restrictions (https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Tags.html):

The following basic restrictions apply to tags:

Maximum number of tags per resource – 50
For each resource, each tag key must be unique, and each tag key can have only one value.
Maximum key length – 128 Unicode characters in UTF-8
Maximum value length – 256 Unicode characters in UTF-8

Although EC2 allows for any character in its tags, other services are more restrictive. The allowed characters across services are: letters, numbers, and spaces representable in UTF-8, and the following characters: + - = . _ : / @.

Tag keys and values are case-sensitive.

The aws: prefix is reserved for AWS use. If a tag has a tag key with this prefix, then you can't edit or delete the tag's key or value. Tags with the aws: prefix do not count against your tags per resource limit.

staebler · 2021-04-09T15:18:43Z

37d7b08...00709ec

Updated to move the new field to status.

staebler · 2021-04-09T15:25:50Z

00709ec...268cd65

forgot to update swagger

JoelSpeed · 2021-04-12T16:18:41Z

Based on what is described in the enhancement, I think this fits the description
/lgtm

frobware · 2021-04-13T15:10:56Z

Cross reference enhancement proposal: openshift/enhancements#706.

frobware · 2021-04-13T17:37:29Z

/lgtm
Anything to stop this merging if we're also good with the EP?

config/v1/types_infrastructure.go

deads2k · 2021-04-15T15:37:08Z

The low level pieces of the API look good. The final higher level question I have is about having user input here on status and not on spec. I would expect this on spec.

We're discussing in slack at the moment.

Add a new field resourceTags to `.status.aws` of the infrastructure.config.openshift.io type. Tags included in the resourceTags field will be applied to new resources created for the cluster. https://issues.redhat.com/browse/CORS-1657

staebler · 2021-04-15T18:29:47Z

The low level pieces of the API look good. The final higher level question I have is about having user input here on status and not on spec. I would expect this on spec.

We're discussing in slack at the moment.

The long-term sketch of the design on this is to add a resourceTags field to the spec. The field in the spec will be open for the user to mutate. Changes in the field in the spec will be reconciled to the status field. Tag tuples that are removed from the field in the spec will be reconciled to a different status field. Operators will be expected to apply to their AWS resources those tags that are in the current status field and remove from their AWS resources those tags that are in the future removed field.

deads2k · 2021-04-15T18:36:34Z

/lgtm

openshift-ci-robot · 2021-04-15T18:36:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, frobware, JoelSpeed, staebler

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~~ [deads2k]

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

openshift-ci-robot · 2021-04-15T19:06:21Z

@staebler: This pull request references Bugzilla bug 1950113, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

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

No GitHub users were found matching the public email listed for the QA contact in Bugzilla ([email protected]), skipping review request.

Details

In response to this:

Bug 1950113: config: add aws usertags to infrastructure

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.

openshift-ci-robot · 2021-04-15T19:07:16Z

@staebler: All pull requests linked via external trackers have merged:

openshift/api#864

Bugzilla bug 1950113 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1950113: config: add aws usertags to infrastructure

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.

Pickup openshift/api#864

michaelgugino · 2021-04-16T13:06:37Z

Tag tuples that are removed from the field in the spec will be reconciled to a different status field. Operators will be expected to apply to their AWS resources those tags that are in the current status field and remove from their AWS resources those tags that are in the future removed field.

This is not reflected in the enhancement, nor did anyone agree to removing tags from resources. Changing and not applying to new resources is not the same as removing tags.

bparees · 2021-04-16T13:58:09Z

This is not reflected in the enhancement, nor did anyone agree to removing tags from resources. Changing and not applying to new resources is not the same as removing tags.

in fact the enhancement explicitly says we must NOT remove tags from the underlying cloud resources. (That was a request from Clayton). Additive only.

staebler · 2021-04-27T17:02:20Z

/cherry-pick release-4.7

openshift-cherrypick-robot · 2021-04-27T17:02:59Z

@staebler: new pull request created: #914

Details

In response to this:

/cherry-pick release-4.7

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.

openshift-ci-robot requested review from bparees and sttts March 15, 2021 21:07

staebler mentioned this pull request Mar 15, 2021

Bug 1950113: manifests: populate aws user tags in infrastructure openshift/installer#4755

Merged

staebler force-pushed the aws_user_tags branch 3 times, most recently from aa79a73 to 37d7b08 Compare March 15, 2021 23:52

openshift-ci-robot requested a review from jstuever March 16, 2021 00:33

frobware added a commit to frobware/cluster-ingress-operator that referenced this pull request Mar 23, 2021

Patch api with openshift/api#864

f0431d3

This is temporary; will drop when 864 merges.

JoelSpeed reviewed Mar 23, 2021

View reviewed changes

michaelgugino suggested changes Mar 23, 2021

View reviewed changes

frobware mentioned this pull request Mar 24, 2021

Bug 1950284: Annotate services of type LoadBalancer with user tags (AWS only) openshift/cluster-ingress-operator#578

Merged

michaelgugino mentioned this pull request Mar 25, 2021

apply user defined tags to all resources openshift/enhancements#706

Merged

ricardomaraschini mentioned this pull request Mar 25, 2021

WIP - Setting user provided tags on bucket creation openshift/cluster-image-registry-operator#675

Closed

jsafrane added a commit to jsafrane/aws-ebs-csi-driver-operator that referenced this pull request Mar 29, 2021

WIP: import github.com/openshift/api/pull/864/ to get AWS user tags

56cdac8

jsafrane mentioned this pull request Mar 29, 2021

Bug 1956411: Add custom tags from Infrastructure openshift/aws-ebs-csi-driver-operator#116

Merged

staebler force-pushed the aws_user_tags branch from 37d7b08 to 00709ec Compare April 9, 2021 15:18

staebler force-pushed the aws_user_tags branch from 00709ec to 268cd65 Compare April 9, 2021 15:25

openshift-ci-robot assigned JoelSpeed Apr 12, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 12, 2021

deads2k reviewed Apr 15, 2021

View reviewed changes

config/v1/types_infrastructure.go Show resolved Hide resolved

staebler force-pushed the aws_user_tags branch from 9eb9046 to 77880e5 Compare April 15, 2021 15:34

staebler force-pushed the aws_user_tags branch from 77880e5 to 3359357 Compare April 15, 2021 16:35

config: add aws resourceTags to infrastructure

a1c745f

Add a new field resourceTags to `.status.aws` of the infrastructure.config.openshift.io type. Tags included in the resourceTags field will be applied to new resources created for the cluster. https://issues.redhat.com/browse/CORS-1657

staebler force-pushed the aws_user_tags branch from 3359357 to a1c745f Compare April 15, 2021 16:49

openshift-ci-robot assigned deads2k Apr 15, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 15, 2021

staebler changed the title ~~config: add aws usertags to infrastructure~~ Bug 1950113: config: add aws usertags to infrastructure Apr 15, 2021

openshift-ci-robot added bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Apr 15, 2021

openshift-merge-robot merged commit 38058be into openshift:master Apr 15, 2021

ricardomaraschini mentioned this pull request Apr 16, 2021

Bug 1954095: Setting user provided tags on bucket creation openshift/cluster-image-registry-operator#679

Merged

frobware added a commit to frobware/cluster-ingress-operator that referenced this pull request Apr 16, 2021

bump: openshift/api

8be304a

Pickup openshift/api#864

frobware added a commit to frobware/cluster-ingress-operator that referenced this pull request Apr 16, 2021

bump: openshift/api

f01a00e

Pickup openshift/api#864

frobware added a commit to frobware/cluster-ingress-operator that referenced this pull request Apr 16, 2021

bump: openshift/api

dad4a28

Pickup openshift/api#864

openshift-ci-robot mentioned this pull request Apr 26, 2021

Bug 1950113: vendor: bump to the latest openshift/api openshift/cluster-config-operator#199

Merged

frobware mentioned this pull request Apr 27, 2021

[release-4.7] Bug 1954097: Annotate services of type LoadBalancer with user tags (AWS only) openshift/cluster-ingress-operator#606

Merged

openshift-cherrypick-robot mentioned this pull request Apr 27, 2021

[release-4.7] Bug 1954152: config: add aws usertags to infrastructure #914

Merged

ricardomaraschini mentioned this pull request Apr 29, 2021

[release-4.7] Bug 1955176: Setting user provided tags on bucket creation openshift/cluster-image-registry-operator#682

Merged

Bug 1950113: config: add aws usertags to infrastructure #864

Bug 1950113: config: add aws usertags to infrastructure #864

Uh oh!

Conversation

staebler commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staebler commented Mar 16, 2021

Uh oh!

gregsheremeta commented Mar 22, 2021

Uh oh!

JoelSpeed Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

michaelgugino Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

michaelgugino left a comment

Choose a reason for hiding this comment

Uh oh!

michaelgugino Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

staebler Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

michaelgugino Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

ricardomaraschini commented Mar 23, 2021

Uh oh!

gregsheremeta commented Mar 23, 2021

Uh oh!

Miciah commented Mar 23, 2021

Uh oh!

frobware commented Mar 24, 2021

Uh oh!

staebler commented Apr 9, 2021

Uh oh!

staebler commented Apr 9, 2021

Uh oh!

JoelSpeed commented Apr 12, 2021

Uh oh!

frobware commented Apr 13, 2021

Uh oh!

frobware commented Apr 13, 2021

Uh oh!

Uh oh!

deads2k commented Apr 15, 2021

Uh oh!

staebler commented Apr 15, 2021

Uh oh!

deads2k commented Apr 15, 2021

Uh oh!

openshift-ci-robot commented Apr 15, 2021

Uh oh!

openshift-ci-robot commented Apr 15, 2021

Uh oh!

openshift-ci-robot commented Apr 15, 2021

Uh oh!

michaelgugino commented Apr 16, 2021

Uh oh!

bparees commented Apr 16, 2021

Uh oh!

staebler commented Apr 27, 2021

Uh oh!

openshift-cherrypick-robot commented Apr 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

staebler commented Mar 15, 2021 •

edited

Loading