Allow the strict IAM policies to be optional

[KashifSaadat]

The stricter IAM policies could potentially cause regression for some edge-cases, or may rely on nodeup image changes that haven't yet been deployed / tagged officially (currently the case on master branch since PR #3158 was merged in).

This PR just wraps the new IAM policy rules around a cluster spec flag, EnableStrictIAM, so will default to the original behaviour (where the S3 policies were completely open). Could also be used to wrap PR #3186 if it progresses any further.

• Or we could reject this and have the policies always strict! :)

[k8s-ci-robot]

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

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. I understand the commands that are listed here.

[KashifSaadat]

/assign @justinsb

[justinsb]

We may yet need this, but I think that pushing a new nodeup (1.7.1.-beta.1) will unbreak master.

We should also get brew not building from HEAD :-)

[KashifSaadat]

@justinsb I've updated this PR with your feedback from #3186, let me know what you think! :)

[justinsb]

This looks great - I think this is absolutely the right approach.

One suggestion, to try to really encourage people to use strict mode.

Should it look like this:

iam:
  legacy: true

And deleting legacy: true or setting it to false would get us strict IAM. Also, in future we can make the type of legacy a BoolOrString or something like that (a similar type is used in memory resources), so that we could have degrees of legacy.

But I think legacy or maybe legacyCompatible would be clear - it isn't exactly insecure, but it is very much recommended to use the newer stricter IAM policies.

The problem though is that because the field's default value will be false, that isn't what we want for existing clusters - and the answer is defaults in apimachinery, i.e. something like this: https://github.com/kubernetes/kops/pull/3190/files#diff-2809c5296bfde5846dec21103f658f3f

(you'll need to do that for both v1alpha1 and v1alpha2, but not internal, because the defaults are applied as part of unmarshalling)

Then I also think kops create cluster should default to legacy: false, i.e. we should default to strict mode for new clusters; it is easy to switch (just a kops edit cluster, or we could expose a flag, or someone on slack was proposing a generic way of specifying fields).

What do you think @KashifSaadat ?

[justinsb]

/ok-to-test

[KashifSaadat]

Hey @justinsb. Thanks for the feedback, all sounds sensible and I agree with the changes. :)

I've had a go at this:

• Changed switch to legacy
• Default to false on cluster creation, so new clusters will use the stricter IAM policies
• Updated apimachinery defaults, setting legacy to true if it's unset
• Updated related tests

Manual tests:

• Create new cluster, validate that strict policies are deployed: OK
• Edit existing cluster, including IAM block and setting legacy to true: OK
• Edit existing cluster, drop the IAM block (existing clusters won't have this API field in their Spec): Fails with the following:

# Found fields that are not recognized
# ...
#         name: c
#       name: events
# -   iam:
# -     legacy: true
#     kubernetesApiAccess:
#     - 0.0.0.0/0
# ...

It looks like those settings are being pulled from defaults correctly, but not sure why it's complaining. Are all users now required to manually include those settings within their spec, or am I missing something to silently include within the spec file?

[KashifSaadat]

Correction: This is working as expected, my manual test was wrong :)

I created a cluster from master, ran edit from my branch and it automatically includes the following within the spec file:

  iam:
    legacy: true

So this PR shouldn't force IAM changes to existing clusters.

[KashifSaadat]

@justinsb by any chance are you able to spot any obvious errors in the work I've done, which may cause the e2e tests to fail? I've tested the cluster creation and operation locally (both with the legacy flag as true and false) and it seems to operate fine.

I've gone through the logs for the api server and the other services on the master node, can't see any specific correlation between the logs at the timestamp of the errors occurring. The only major change in this PR is the inclusion of the API flag. The stricter S3 IAM policies have already been deployed, so I don't believe it's related to that. Any pointers would be appreciated!

[justinsb]

e2e tests are a flake I believe @KashifSaadat - I've seen them on docs PRs. There was a suspicious related PR that went in to k8s earlier today, so I suspect that's it, but I haven't yet had time to dig in. I will do it if it continues though!

/retest

[justinsb]

/lgtm

[justinsb]

My preference is to include omitempty here (false is considered empty), but we can add that later - that's not breaking (I believe)

[KashifSaadat]

I specified omitempty initially to have consistency with the other API fields. I was unsure if it may cause some problems with having the cluster creation default to a legacy value of false (not present in spec), and then the apimachinery defaults check for a nil value and set legacy to true. I should have done more validation before pushing up! :D

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

• OWNERS [justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[justinsb]

Flakiness is being tracked in upstream issue: kubernetes/kubernetes#51128, hopefully fixed by kubernetes/kubernetes#51144

[eparis]

/retest
merged the referenced fix

[k8s-github-robot]

/test all [submit-queue is verifying that this PR is safe to merge]

[k8s-github-robot]

Automatic merge from submit-queue

[KashifSaadat]

Great, thanks guys!