Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default to enabling legacy ABAC policy in non-test kube-up.sh environments #43544

Merged
merged 1 commit into from
Mar 23, 2017

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Mar 22, 2017

Fixes #43541

In 1.5, we unconditionally stomped the abac policy file if KUBE_USER was set, and unconditionally used ABAC mode pointing to that file.

In 1.6, unless the user opts out (via ENABLE_LEGACY_ABAC=false), we want the same legacy policy included as a fallback to RBAC.

This PR:

  • defaults legacy ABAC on in normal deployments
  • defaults legacy ABAC on in upgrade E2Es (ensures combination of ABAC and RBAC works properly for upgraded clusters)
  • defaults legacy ABAC off in non-upgrade E2Es (ensures e2e tests 1.6+ run with tightened permissions, and that default RBAC roles cover the required core components)

GKE changes to drive the ENABLE_LEGACY_ABAC envvar were made by @cjcullen out of band

`kube-up.sh` using the `gce` provider enables both RBAC authorization and the permissive legacy ABAC policy that makes all service accounts superusers. To opt out of the permissive ABAC policy, export the environment variable `ENABLE_LEGACY_ABAC=false` before running `cluster/kube-up.sh`.

@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 22, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Mar 22, 2017
@mikedanese
Copy link
Member

/approve

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 22, 2017
@liggitt liggitt added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 22, 2017
@liggitt
Copy link
Member Author

liggitt commented Mar 22, 2017

a few open questions remaining, marking do-not-merge

do upgrade tests use config-test.sh? (@krousey?)
do regular CI jobs use config-test.sh?
does GKE need to make similar adjustments here? (@cjcullen?)
do we need to generate the ABAC file in gce/container-linux as well?

@liggitt liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Mar 22, 2017
@krousey
Copy link
Contributor

krousey commented Mar 22, 2017

@liggitt The answer to the first 2 questions should be the same, but I'm unsure which it is. @fejta

@fejta
Copy link
Contributor

fejta commented Mar 22, 2017

I am not super familiar with what config-test is and why/whether anything needs to use it.

It appears as though kubetest --up calls this though:

https://github.com/kubernetes/test-infra/blob/master/kubetest/bash.go#L26
https://github.com/kubernetes/kubernetes/blob/master/hack/e2e-internal/e2e-up.sh#L24

@krousey
Copy link
Contributor

krousey commented Mar 22, 2017

@liggitt I think I found the answer

> grep -r 'config-test.sh' hack
hack/ginkgo-e2e.sh:: ${KUBE_CONFIG_FILE:="config-test.sh"}
hack/e2e-internal/e2e-up.sh:: ${KUBE_CONFIG_FILE:="config-test.sh"}
hack/e2e-internal/e2e-cluster-size.sh:: ${KUBE_CONFIG_FILE:="config-test.sh"}
hack/e2e-internal/e2e-down.sh:: ${KUBE_CONFIG_FILE:="config-test.sh"}
hack/e2e-internal/e2e-status.sh:: ${KUBE_CONFIG_FILE:="config-test.sh"}

so yes

@liggitt
Copy link
Member Author

liggitt commented Mar 22, 2017

@euank @yifan-gu @ethernetdan you're listed as owners of the container-linux kube-up provider... that provider looks to be new in 1.6... is there an expectation that that provider will create a legacy 1.5 ABAC policy by default?

@grodrigues3 grodrigues3 added this to the v1.6 milestone Mar 22, 2017
@cjcullen
Copy link
Member

This will also need a

ENABLE_LEGACY_ABAC: $(yaml-quote ${ENABLE_LEGACY_ABAC:-})

in the master-only section of bulid-kube-env in https://github.com/kubernetes/kubernetes/blob/master/cluster/common.sh#L761.

@liggitt liggitt force-pushed the legacy-abac-kube-up branch from f86a83b to d8c1318 Compare March 23, 2017 00:41
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 23, 2017
@liggitt liggitt force-pushed the legacy-abac-kube-up branch from d8c1318 to 202c884 Compare March 23, 2017 00:42
@yifan-gu
Copy link
Contributor

yifan-gu commented Mar 23, 2017

@liggitt FYI that provider is renamed from the gce/coreos, so it lives before 1.6.
In theory I think the expectation will be the same as the the gce/gci provider.

@liggitt
Copy link
Member Author

liggitt commented Mar 23, 2017

thanks for looking, I updated it to behave the same as the gce/gci one

@cjcullen
Copy link
Member

Confirmed that this works in GCE and in GKE (support for the new var to be able to disable RBAC in GKE will roll out to test tonight ~midnight).

@liggitt liggitt force-pushed the legacy-abac-kube-up branch from 202c884 to 9b3eabb Compare March 23, 2017 02:18
@liggitt liggitt force-pushed the legacy-abac-kube-up branch from 9b3eabb to b95f528 Compare March 23, 2017 02:20
@@ -241,6 +241,17 @@ SCHEDULING_ALGORITHM_PROVIDER="${SCHEDULING_ALGORITHM_PROVIDER:-}"
# Optional: install a default StorageClass
ENABLE_DEFAULT_STORAGE_CLASS="${ENABLE_DEFAULT_STORAGE_CLASS:-true}"

# Optional: Enable legacy ABAC policy that makes all service accounts superusers.
if [[ "${E2E_UPGRADE_TEST:-}" == "true" ]]; then
Copy link
Member Author

@liggitt liggitt Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krousey @fejta can you verify this value from the upgrade CI job env files is visible in this scope?

Other values from those files (like STORAGE_BACKEND) seem to be usable in this context, but I wanted to make sure

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This value indicates that an upgrade step will run prior to a full e2e run. There are CI jobs that do upgrades that don't do this 2 pass behavior, so this will be unset for them.

Also, I'm not sure how closely we should tie the E2E* vars into this config. @fejta thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure e2e-runner.sh leaks this var, but this is not intentional. I plan to delete e2e-runner,.sh and its magical global state and replace it with flags that the job explicitly uses to call kubetest.

Could we solve this by adding ENABLE_LEGACY_ABAC=false to the .env file for the relevant upgrade jobs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Adding ENABLE_LEGACY_ABAC=true in the upgrade jobs would drive the behavior directly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened kubernetes/test-infra#2330 to update upgrade jobs

@liggitt liggitt added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Mar 23, 2017
@liggitt
Copy link
Member Author

liggitt commented Mar 23, 2017

would like an ack from @krousey / @fejta on the envvar visibility from the CI job

@cjcullen can you update the release note with any info needed for GKE?

ready otherwise

@liggitt liggitt removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Mar 23, 2017
@cjcullen
Copy link
Member

GKE behavior should still be the same. Default is ABAC enabled. Set Cluster.LegacyAbac.Enabled or pass --enable-legacy-abac or --no-enable-legacy-abac through gcloud if you don't want the default.

@cjcullen
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjcullen, liggitt, mikedanese

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

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 43546, 43544)

@k8s-github-robot k8s-github-robot merged commit 1e879c6 into kubernetes:master Mar 23, 2017
@k8s-github-robot
Copy link

@liggitt PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2017
@liggitt liggitt deleted the legacy-abac-kube-up branch March 24, 2017 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.