-
Notifications
You must be signed in to change notification settings - Fork 40k
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
Default to enabling legacy ABAC policy in non-test kube-up.sh environments #43544
Conversation
/approve |
I am not super familiar with what config-test is and why/whether anything needs to use it. It appears as though https://github.com/kubernetes/test-infra/blob/master/kubetest/bash.go#L26 |
@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 |
@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? |
This will also need a
in the master-only section of |
f86a83b
to
d8c1318
Compare
d8c1318
to
202c884
Compare
@liggitt FYI that provider is renamed from the gce/coreos, so it lives before 1.6. |
thanks for looking, I updated it to behave the same as the gce/gci one |
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). |
202c884
to
9b3eabb
Compare
9b3eabb
to
b95f528
Compare
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
GKE behavior should still be the same. Default is ABAC enabled. Set |
/lgtm |
[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 |
Automatic merge from submit-queue (batch tested with PRs 43546, 43544) |
@liggitt PR needs rebase |
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:
GKE changes to drive the
ENABLE_LEGACY_ABAC
envvar were made by @cjcullen out of band