pkg/types/installconfig: Add an (*InstallConfig).Tags() helper

[wking]

Some AWS resources created by the cluster (e.g. some elastic load-balancers) are currently missing the tectonicClusterID tag, which makes cleanup difficult. This helper makes it easy for those external tools to set the appropriate tags without needing local logic to generate them.

CC @joelddiaz

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wking

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

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

[abhinavdahiya]

I'm not convinced we need this added to the API.

the terraform controls the tags. eg: kubernetes.op/cluster/cluster-name: owned...
I think clustermetadata should be used by users trying to find out their identifiers.

[wking]

the terraform controls the tags. eg: kubernetes.op/cluster/cluster-name: owned...

True, but many resources are created by operators, etc. in the launched cluster and not through Terraform. This helper is intended to make life easier for those operators. For example, the machine-API operator could slot the value in here (where it looks like it's currently ignoring UserTags?).

[abhinavdahiya]

Like I said we are removing the machinesets from machine-api-operator.

Also the InstallConfig will not hit the cluster. so no operator can use this api after cluster install.

[wking]

Like I said we are removing the machinesets from machine-api-operator.

What about folks creating load-balanced services?

Also the InstallConfig will not hit the cluster. so no operator can use this api after cluster install.

Well, that sinks this pretty clearly ;). Do we have a timeline for this? It would be nice to have tag-based cleanup not broken for too long :p.

[abhinavdahiya]

What about folks creating load-balanced services?

That we cannot control even if we add this API.

It would be nice to have tag-based cleanup not broken for too long. I think the hive team was working on moving to using the kubernetes.io/cluster/cluster-name: owned tag to identify everything...

[wking]

What about folks creating load-balanced services?

That we cannot control even if we add this API.

No, but the easier we make it for folks to add the tags we want, the more likely they are to do it ;). More thoughts about how we're going to handle destroy-cluster in the face of user-created resources over here.

It would be nice to have tag-based cleanup not broken for too long.

I think the hive team was working on moving to using the kubernetes.io/cluster/cluster-name: owned tag to identify everything...

Yeah, this PR is spun out from #458, where @joelddiaz was asking for kubernetes.io/cluster/CLUSTER_NAME on everything. But I haven't turned up upstream docs for that tag, and I'm concerned about unintentional side effects from using it as a deletion marker without a clearer understanding of its intended semantics. I'm also slightly worried about using the less-unique cluster name for deletion (vs. the UUID we use for tectonicClusterID).

[joelddiaz]

@aaronlevy who could we talk to that could educate us on what the consequences might be if we started tagging things not meant for kubernetes (eg route53 zones, s3 buckets, etc) with the 'kubernetes.io/cluster/CLUSTER_NAME' tag?

[abhinavdahiya]

@wking

of its intended semantics. I'm also slightly worried about using the less-unique cluster name for deletion

The cluster name is already required to be unique. If it collides with already existing, then all the dnses will be overwritten causing wierd errors, or terraform would complain and exit.

[wking]

The cluster name is already required to be unique. If it collides with already existing, then all the dnses will be overwritten causing wierd errors, or terraform would complain and exit.

I don't think DNS would be an issue if you used a different base domain. I'm less clear on whether there would be other Terraform collisions, but if there are, they're probably bugs. You should be able to create a.example.com and a.example.org in the same AWS account (or in libvirt, etc.), right?

[aaronlevy]

@joelddiaz I would expect the behavior to fall under cloud-providers, so maybe @enxebre?

@wking https://github.com/kubernetes/kubernetes/blob/v1.12.1/pkg/cloudprovider/providers/aws/tags.go#L44-L51 is best descriptor I've found.

For some of these resources we probably want to be using shared. Maybe cleanup could ask about shared resources?

[joelddiaz]

@enxebre the TL;DR of this can be summarized as:

Would it be inappropriate to label things out in AWS that are not for kubernetes to inspect or be aware of with the kubernetes-specific label kubernetes.io/cluster/CLUSTER_NAME? Specifically we're talking about labeling S3 buckets (used for ignition), route53 zones (set up by the installer)

The purpose of this would be to be able to query AWS for all objects with tag kubernetes.io/cluster/CLUSTER_NAME to 1) simply see all the cloud pieces that make up cluster CLUSTER_NAME 2) be able to point the uninstaller at this tag, and have it delete the objects with the matching tag

[abhinavdahiya]

@wking do we need this anymore?

[wking]

@wking do we need this anymore?

I think so (or something similar); we're still not setting the user-configured tags on all the resources we create. If we want to throw in the towel on that front, I think we'd want to drop UserTags instead of providing a partial implementation for that property.

[abhinavdahiya]

The goal is for the InstallConfig to have no consumers outside installer (and maybe hive, maybe). So @wking if you think we have enough consumers in the installer codebase that can use this function, lets include them in this PR and move forward else, maybe close it.

[wking]

The goal is for the InstallConfig to have no consumers outside installer...

I thought you'd said that AWS tag information was going to move into a *.config.openshift.io object? Once that object materializes, I was going to reroll this helper function so you could feed it a Kubernetes client and get the tags back out. But if we want to store the namespace, config-name, unpacking logic in each consuming client (vs. them vendoring our Tags unpacker function), I agree that we can close this PR.

[openshift-ci-robot]

@wking: PR needs rebase.

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]

@wking: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/rhel-images	9a2a91e	link	/test rhel-images
ci/prow/e2e-aws-upgrade	9a2a91e	link	/test e2e-aws-upgrade

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

[wking]

Obsolete. See openshift/api#231 and openshift/api#266.

/close

[openshift-ci-robot]

@wking: Closed this PR.

In response to this:

Obsolete. See openshift/api#231 and openshift/api#266.

/close

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.