feat(cmd/kops/create_cluster): default to kubelet.anonymousAuth false on k8s versions >=1.10

[jaredallard]

What this PR does: Sets kubelet.anonymousAuth to false by default on new cluster creation

Why: It's extremely dangerous to have this be undefined by default (which defaults to true) and not have it in bold letters on the README.md that it's off by default. It's very easy to overlook this as a "newbie" or somebody who skims through stuff (not saying that's OK, but people will be that way regardless). So, this introduces setting it to false by default on new cluster creation.

Notes: I'm sure this probably isn't the best place to do this, but it seemed like it probably was. I could also see it being moved to pkg/apis/kops/apiserver.go as well.

[k8s-ci-robot]

Hi @jaredallard. 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.

[jaredallard]

Seems like this is a dupe of #4307, but it does it in a slightly different way by enforcing it only on new cluster creation.

EDIT: Anticipating I'm going to have to update some test files to make this work properly, will get on that.

[justinsb]

I like the idea of tightening this up for new clusters - we can verify that it all works now :-)

/ok-to-test

The other piece would be to recommend it on kops upgrade or just change the default for clusters >= 1.X. I'm also a little confused about whether this should be false or true, but I'll take a look!

[justinsb]

So I looked at the code, and I think you mean to default to false (?)

I agree with the sentiment, and I suggest an easier way of doing this might be just to say that when installing k8s >= 1.12 (say) that we default it to secure. So nil means "default" and maps to allow-anonymous for k8s <= 1.11, but disallow-anonymous for k8s >= 1.12. kops users can still explicitly set the field to true or false, to force it one way or the other.

Going to move to milestone 1.12 as I think this is a little late for a change of this magnitude for 1.11, but I'd love to see this in 1.12.

[jaredallard]

@justinsb Yeah, it would definitely be better to approach it that way. I'll see if I can get that done over the weekend

[jaredallard]

Not sure what's up with the gofmt test, that's passing locally, so going to ignore that. Fixing the bazel one

[jaredallard]

It's working as per the tests, just stuck with that gofmt issues, any ideas @justinsb?

[justinsb]

This looks good - the gofmt thing is really annoying - it's because go switched the behaviour of gofmt in go 1.11. But k8s is only moving to go 1.11 in k8s 1.13, so until then we need to build with go 1.10, and so we verify gofmt with that version. But of course most people have go 1.11 installed now. If you have go 1.10 you could use that to gofmt, or I can probably just force-merge it and update gofmt.

Although I agree it's an important issue, we still don't change the behaviour of existing clusters unless we have to - we want everyone to upgrade to the latest kops version and not stay on older versions. If we start changing behaviours on released clusters we jeopardize that (although, to be fair, addons are in a weird place, which we hope to improve with bundles). Can we default for >= 1.11 ? The mechanisms we have are that we can have a release note, and/or we could add a warning that is logged on every kops command.

[jaredallard]

@justinsb gotchaaaa makes sense, I'll fix that (both cases). Thanks!

[jaredallard]

@justinsb So, I've made it so that it defaults (on create cluster only) on >=1.11, and it warns on update cluster (just like a k8s version update) on >=1.10. It doesn't modify it itself, just recommends that the user should do it. I think this is probably the best way to handle it.

[justinsb]

Thanks @jaredallard - looks good and much more secure!

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jaredallard, justinsb

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

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

[tsuna]

@jaredallard thanks for the recommendation on update cluster, that definitely helped bring my attention to this issue. One thing that isn't clear to me yet though is that kops security docs say:

Note on an existing cluster with 'anonymousAuth' unset you would need to first roll out the masters and then update the node instance groups.

So it wasn't clear to me whether this could be safely done as part of the upgrade or whether it had to be done separately after the upgrade has completed.

[jaredallard]

@tsuna 🎉 Glad it was helpful! That's a really good point, I don't think that is actually 100% true that the masters need to be rolled out first when changing that parameter. Regardless, it's usually (at least in my mind) best practice to roll the masters first when doing any changes.

This could probably be refined to state that in a clearer way so that nobody accidentally runs into some sort of issues if my deductions are wrong. Thoughts @justinsb?