Adding additional tags and shared to various AWS components

[chrislovecnm]

This PR adds the base tags to DHCP Options, IGW, and Route Tables.
These components are not tagged correctly, and this can cause issues
with deletion. Name tags are not added to shared resources, as we allow
shared resources to have maintained names. An owned/shared tags with the
syntax "kubernetes.io/cluster/$CLUSTERNAME" = "owned" is added to the
resources as well. We are maintaining the Name tag value for private
route tables, as these resources do not use the standard value.

We may want to add these tags to NAT GW and Elastic IPs as well....

[chrislovecnm]

I have some bugs with private topology and this PR, need to figure out why the second route table is misbehaving.

[chrislovecnm]

@mikesplain @blakebarnett can you double check that the tag renames are correct? I think some of our tags have been named incorrectly. The weave test is passing, for instance, please let me know if we need tweaks to that.

/assign @mikesplain

@mikesplain I have a few more darn tests to update, but otherwise, the PR is good to review as of now.

[chrislovecnm]

@blakebarnett / @mikesplain notice the change in the Name tag. Just want to verify that the change is correct. Do we need the private-us-test-1a.privatecalico.example.com?

[mikesplain]

@chrislovecnm I think private-us-test-1a.privatecalico.example.com is actually the correct name, that's how our calico route tables are currently named across the board.

If that's correct, then the tests below are actually incorrect but that all depends on how detailed you want to be in these tests. It looks like the rest of the tests below do not specify the full subnet name, as the name tag, so maybe this is fine for testing with your change?

[chrislovecnm]

I am wondering if the names are actually wrong ;(

[chrislovecnm]

Let me be more clear. I am wondering if I came across a bug, as I am uncertain why the name is different from every other component.

[mikesplain]

I believe they are incorrect / bug as well. Any thoughts @robinpercy @blakebarnett?

[chrislovecnm]

@mikesplain we may have to maintain two tags not certain. We have to have the correct tag format:

 "kubernetes.io/cluster/$CLUSTERNAME" = "owned"

or

 "kubernetes.io/cluster/$CLUSTERNAME" = "shared"

These tags allow for not deleting shared resources, which is currently happening in our code :(

So maybe I leave the Name tag alone and just add the new tag kubernetes.io/cluster/ correctly. I think only the private route table is the only one where the Name tag is different.

[blakebarnett]

They certainly seem to be wrong in the tests, but yeah, it's just the name, not crucial. Consistency is good though.

[mikesplain]

Besides the test discussion, I believe the rest of the code looks good.

[mikesplain]

Very nice, good fix.

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm, mikesplain

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

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

[jdnurmi]

One nit I ran into - and I'm not 100% sure it's all due to this change, but saw the NAT gateway reference:

Cloudformation doesn't actually support tags on all these resources:
EIP's don't appear to support them at all;
Nat Gateways support them, but the rendered syntax doesn't seem quite correct (lower tags and some dict work going on):

      tags: {KubernetesCluster: us-east-2.k8s.some.co, Name: us-east-2b.us-east-2.k8s.some.co,
        kubernetes.io/cluster/us-east-2.k8s.some.co: owned}

So when using --target cloudformation , you get a template that almost works if your topology uses EIP's or NAT

[ Version 1.9.0 (git-cccd71e67) ]

All other parts for my build deployed fine