Add tagging support for AWS Keypairs #9533

rifelpet · 2020-07-09T04:17:16Z

AWS recently added tagging support for keypairs as well as autogenerated IDs. This adds the usual cloud tags to keypairs as seen in the integration test outputs.

WIP while I figure out how to handle keys that are shared with the cluster rather than created by kops- would we tag it with "shared" like with do VPCs and Subnets or do we not tag it at all?

Also need to double check how kops deletes keypairs during a delete cluster and make sure these tags don't affect that process.

ref: #9640

At this point we could probably drop the cluster name prefix from key names but that's probably not necessary.

k8s-ci-robot · 2020-07-09T04:17:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 · 2020-07-09T05:21:43Z

/test pull-kops-e2e-kubernetes-aws

justinsb · 2020-07-12T17:44:05Z

cloudmock/aws/mockec2/keypairs.go

@@ -50,18 +50,25 @@ func (m *MockEC2) ImportKeyPair(request *ec2.ImportKeyPairInput) (*ec2.ImportKey
 		return nil, err
 	}

+	n := len(m.DhcpOptions) + 1


Do you want len(m.KeyPairs) ?

justinsb · 2020-07-12T17:45:35Z

cloudmock/aws/mockec2/keypairs.go

-	o := m.KeyPairs[id]
-	if o == nil {
-		return nil, fmt.Errorf("KeyPairs %q not found", id)
+	name := aws.StringValue(request.KeyName)


Can we / should we now delete by id?

justinsb · 2020-07-12T17:46:29Z

tests/integration/update_cluster/existing_iam/kubernetes.tf

+  tags = {
+    "KubernetesCluster"                              = "existing-iam.example.com"
+    "Name"                                           = "existing-iam.example.com"
+    "kubernetes.io/cluster/existing-iam.example.com" = "owned"


Yes, I think this should be shared, because we don't delete it (we couldn't safely delete it before); so it does behave more like "shared"

Actually, scratch that - I'm wrong! We do delete the keypairs, which is why we have the cluster name in the prefix, so owned is correct.

justinsb · 2020-07-12T17:51:44Z

Generally LGTM. I was a little confused as to whether we delete keys or not; because/if we do, it should be 'owned', which is what you've done 👍

At this point we could probably drop the cluster name prefix from key names but that's probably not necessary.

Can we have duplicate SSH key names now? I assume we can't, so I would imagine if we dropped the prefix that we might get collisions if someone used the same SSH key for two clusters in the same region?

rifelpet · 2020-07-28T17:15:47Z

This is ready for review.

I had to make the SSHKey task aware of whether it is Shared or not. The intent is to not add any tags to keys that are specified in the ClusterSpec.sshKeyName. These tags aren't consumed by anything else (unlike shared VPC and Subnet tags) and may collide with tags from other clusters that also use the same sshKeyName, so I thought it was better to just not add the tags at all. In order to do that, we have to specify whether the key is shared or not. Previously we had the same task behavior regardless of whether a key was defined via sshKeyName or via a SSHPublicKey secret.

I tested the following scenarios:

create new cluster with old kops and local ssh key file specified
update cluster with new kops: correctly applies tags
update cluster with new kops: no changes reported

create new cluster with new kops and local ssh key file specified: correctly applies tags
update cluster with new kops: no changes reported

create new cluster with old kops and existing AWS sshKeyName specified in ClusterSpec:
update cluster with new kops: no changes reported (key remains untagged)

create new cluster with new kops and existing AWS sshKeyName specified in ClusterSpec:
update cluster with new kops: no changes reported (key remains untagged)

create new cluster with new kops and existing AWS sshKeyName specified in ClusterSpec
update cluster --target terraform with new kops: no keypair resource defined (LaunchTemplate hardcodes key name)

create new cluster with new kops and local ssh key file specified
update cluster --target terraform with new kops: keypair resource is defined with tags

pkg/model/sshkey.go

hakman · 2020-07-29T01:54:26Z

/lgtm

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 9, 2020

k8s-ci-robot requested review from hakman and olemarkus July 9, 2020 04:17

k8s-ci-robot added area/provider/aws Issues or PRs related to aws provider approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 9, 2020

justinsb reviewed Jul 12, 2020

View reviewed changes

rifelpet mentioned this pull request Jul 27, 2020

Improve tagging story on AWS #9640

Closed

rifelpet force-pushed the keypair-tags branch from 323c1dc to a1f2dc6 Compare July 28, 2020 17:08

rifelpet changed the title ~~WIP Add tagging support for AWS Keypairs~~ Add tagging support for AWS Keypairs Jul 28, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2020

hakman approved these changes Jul 28, 2020

View reviewed changes

pkg/model/sshkey.go Outdated Show resolved Hide resolved

rifelpet force-pushed the keypair-tags branch from a1f2dc6 to d114a57 Compare July 28, 2020 18:34

rifelpet added 3 commits July 28, 2020 13:35

Add keypair tag and ID support to cloudmock

690f2e3

Add cloud tags to AWS SSH Keys

a17581e

Update integration test outputs with new ssh key tags

f3e6ac8

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/addons and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 28, 2020

rifelpet force-pushed the keypair-tags branch from d114a57 to f3e6ac8 Compare July 28, 2020 18:35

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 28, 2020

k8s-ci-robot assigned hakman Jul 29, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2020

k8s-ci-robot merged commit 31b03c4 into kubernetes:master Jul 29, 2020

k8s-ci-robot added this to the v1.19 milestone Jul 29, 2020

rifelpet mentioned this pull request Oct 28, 2020

Set minimum Terraform version to 0.12.26/0.13.0 #10109

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tagging support for AWS Keypairs #9533

Add tagging support for AWS Keypairs #9533

rifelpet commented Jul 9, 2020 •

edited

Loading

k8s-ci-robot commented Jul 9, 2020

hakman commented Jul 9, 2020

justinsb Jul 12, 2020

justinsb Jul 12, 2020

justinsb Jul 12, 2020

justinsb Jul 12, 2020

justinsb commented Jul 12, 2020

rifelpet commented Jul 28, 2020 •

edited

Loading

hakman commented Jul 29, 2020

Add tagging support for AWS Keypairs #9533

Add tagging support for AWS Keypairs #9533

Conversation

rifelpet commented Jul 9, 2020 • edited Loading

k8s-ci-robot commented Jul 9, 2020

hakman commented Jul 9, 2020

justinsb Jul 12, 2020

Choose a reason for hiding this comment

justinsb Jul 12, 2020

Choose a reason for hiding this comment

justinsb Jul 12, 2020

Choose a reason for hiding this comment

justinsb Jul 12, 2020

Choose a reason for hiding this comment

justinsb commented Jul 12, 2020

rifelpet commented Jul 28, 2020 • edited Loading

hakman commented Jul 29, 2020

rifelpet commented Jul 9, 2020 •

edited

Loading

rifelpet commented Jul 28, 2020 •

edited

Loading