Add "owned" tag for volumes and instances created with launch templates

[hakman]

Using launch templates for instance groups make adds the kops & k8s tags to instances and root volumes.

For volumes, the cluster delete function checks if the resource is "shared" or "owned", otherwise it displays the following warning:

W0229 23:33:33.176831    8549 aws.go:2148] (new) cluster tag not found on volume:vol-038ab86f6fcd93336

kops/pkg/resources/aws/aws.go

Lines 2134 to 2136 in 632b426

	// HasSharedTag looks for the shared tag indicating that the cluster does not own the resource
	func HasSharedTag(description string, tags []*ec2.Tag, clusterName string) bool {
	tagKey := "kubernetes.io/cluster/" + clusterName

[hakman]

/cc @rifelpet

[johngmyers]

For volumes, the cluster delete function checks if the cluster is "shared" or "owned", otherwise

Perhaps you meant that it checks if the resource is "shared" or "owned"?

[hakman]

Perhaps you meant that it checks if the resource is "shared" or "owned"?

Yes. Updated. Thanks.

[johngmyers]

Perhaps call b.CloudTags("", false) ?

What would be the downside to moving that to CloudTagsFroInstanceGroup before LaunchConfigurations are removed? Why don't instances/volumes created by LaunchConfigurations throw the warning?

[hakman]

What would be the downside to moving that to CloudTagsFroInstanceGroup before LaunchConfigurations are removed?

I don't know if it would affect any current workflow if this tag is applied at ASG level. Launch templates are still experimental in Kops and shouldn't be an issue.

Why don't instances/volumes created by LaunchConfigurations throw the warning?

Instances created with launch configurations don't tag root volumes at all. Because of this, the volumes are not tracked as part of the cluster by the delete function. They are still deleted automatically when the instance is deleted.

May also be a possibility to not track root volumes at all with the deleted function, even if they have the other tags.

[johngmyers]

I like the idea of being consistent in what tags we apply.

I could see the KubernetesCluster tag being useful for cost control and leaving it out lacks consistency, so I think we should call CloudTags rather than setting kubernetes.io/cluster/ directly.

For consistency, I'm in favor of moving things into CloudTagsForInstanceGroup sooner. Perhaps we should have CloudTagsForInstanceGroup call CloudTags. I don't know what the consequence of adding the ownership tags to the CloudTags template function would be, though.

[hakman]

I actually tried this as the first iteration, but the changes in tests were bigger than I expected.
I don't think should affect any workflow, but preferred not to change so much.

[johngmyers]

/lgtm

[rifelpet]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman, rifelpet

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

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

[hakman]

The warnings are gone:
https://prow.k8s.io/view/gcs/kubernetes-jenkins/logs/e2e-kops-aws-misc-launchtemplates/1234559136599904258#1:build-log.txt%3A69940