Use correct tag when creating node labels from azure cloud tags

[NickSchleicher]

Fixes first bug mentioned here #10412 (comment)
nodeLabels specified in the InstanceGroup spec weren't being labeled on nodes created in Azure

[k8s-ci-robot]

Welcome @NickSchleicher!

It looks like this is your first PR to kubernetes/kops 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kops has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @NickSchleicher. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[rifelpet]

/ok-to-test

[rifelpet]

/cc @kenji-cloudnatix

[k8s-ci-robot]

@rifelpet: GitHub didn't allow me to request PR reviews from the following users: kenji-cloudnatix.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @kenji-cloudnatix

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.

[kenji-cloudnatix]

This looks good to me, but I believe we also need to convert "" back to "/" in v as we convert "/" to "" in

kops/pkg/model/azuremodel/context.go

Line 108 in 309dc9f

// Replace all "/" with "_" as "/" is not an allowed key character in Azure.

?

If we do that, I guess we also need to convert "" to "__" or something else in CloudTagsForInstanceGroup so that we can distinguish (a) original "" and (b) "_" after conversion.

[NickSchleicher]

hey @kenji-cloudnatix, i think the formatting got messed up on your message, i'm seeing empty quotes
but I remember one of the concerns i had about converting _ back to / was if someone had this in their instanceGroup spec
which is part of what i think you're trying to describe

  nodeLabels:
    disk_type: ssd

I feel like we would almost have to convert user defined / into some pattern other than _ in order to convert back to / as k8s labels without any side effects
that way they don't end up with disk/type: ssd as a label on their node, in this example

[kenji-cloudnatix]

Ah, sorry. Yes, that's what I meant!

I believe something like below work?

• Convert _ to __.
• Convert / to _s

Then we can apply the following rule to get the original string:

• First convert _s back to /
• Then convert __ back to _

[NickSchleicher]

thanks @kenji-cloudnatix! I don't think i see any issues going that route

also @kenji-cloudnatix and @rifelpet i can't get to the _ / issue atm with a lot of work needed for my org, but I do intend to since my org will potentially have an azure based contract in the coming months. If possible I would like to get this fix in so we at least won't have to fork kops to get nodeLabels applied when that time comes.

To recap:
Right now node labels applied through nodeLabels in instance group spec are being created as tags in the cloud, but aren't being applied to nodes as k8s labels due to this error in the controller

I0120 18:14:43.199009       1 node_controller.go:144] sending patch for node "web000001": "{\"metadata\":{\"labels\":{\"\":\"web\"}}}"
W0120 18:14:43.204913       1 node_controller.go:110] failed to patch node labels on web000001: error applying patch to node: Node "web000001" is invalid: [metadata.labels: Invalid value: "": name part must be non-empty, metadata.labels: Invalid value: "": name part must consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyName',  or 'my.name',  or '123-abc', regex used for validation is '([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9]')]

This pr fixes that issue. I can create another issue with the relevant notes, for the / conversion that needs to happen in the preset cloud tags applied by kops, along with the ones that can be created through nodeLabels now that it will work again. And then get to that issue as soon as I can, I already know we will eventually run into it. Let me know if any concerns!

[kenji-cloudnatix]

Thanks for the update, @NickSchleicher ! Works for me.

[rifelpet]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: NickSchleicher, 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

[fejta-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

[kenji-cloudnatix]

FYI: Just created #10935 for the remaining issue discussed in this PR.