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.

Sign up for GitHub

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

Jump to bottom

remove abac #39092

Merged
merged 2 commits into from
Jan 3, 2017
Merged

remove abac #39092

merged 2 commits into from
Jan 3, 2017

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Dec 21, 2016 •
edited

Remove the abac authorizer as an authorizer for e2e.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 21, 2016
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Dec 21, 2016
@deads2k deads2k assigned cjcullen and unassigned ixdy Dec 21, 2016
@deads2k
Copy link
Contributor Author

deads2k commented Dec 21, 2016

rebased on #39094

I'd like to merge this on January 3.

@deads2k deads2k changed the title [WIP] remove abac remove abac Dec 21, 2016
@deads2k deads2k added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Dec 21, 2016
@deads2k
Copy link
Contributor Author

deads2k commented Dec 21, 2016

@cjcullen @kubernetes/sig-auth-misc @sttts @ncdc @liggitt @smarterclayton This pull disabled the ABAC authorizer, ran with RBAC instead and passed all e2e. I'd like to merge prereqs this week and merge this January 3.

@deads2k deads2k mentioned this pull request Dec 22, 2016
Closed
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2016
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 22, 2016
justinsb
justinsb suggested changes Dec 27, 2016
View reviewed changes
Copy link
Member

@justinsb justinsb left a comment

Choose a reason for hiding this comment

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

I'd love to see ABAC gone. But as we shipped ABAC (and not in alpha AFAICT), we need to keep supporting it right? So we should keep testing it...

Is it possible to create a separate e2e job instead?

@deads2k
Copy link
Contributor Author

deads2k commented Dec 28, 2016

I'd love to see ABAC gone. But as we shipped ABAC (and not in alpha AFAICT), we need to keep supporting it right? So we should keep testing it...

Is it possible to create a separate e2e job instead?

The abac policy being tested is "allow all", so there's not really value in keeping it in the test.

@smarterclayton
Copy link
Contributor

An integration test for abac would be more appropriate than a new e2e suite.

@deads2k
Copy link
Contributor Author

deads2k commented Jan 3, 2017

@k8s-bot test this

@deads2k
Copy link
Contributor Author

deads2k commented Jan 3, 2017

@sttts you're probably up, can you take a look? Had to add some more permissions for a kubectl test. Went ahead and tidied up the implementation.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 3, 2017
@deads2k deads2k mentioned this pull request Jan 3, 2017
Merged
sttts
sttts reviewed Jan 3, 2017
View reviewed changes
test/e2e/pre_stop.go
return
}
framework.ExpectNoError(err)
framework.BindClusterRole(f.ClientSet.Rbac(), "cluster-admin", f.Namespace.Name,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether it wouldn't be cleaner to ignore the error here on the test level, no inside BindClusterRole

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder whether it wouldn't be cleaner to ignore the error here on the test level, no inside BindClusterRole

That's going to mean boiler plate everywhere and an error that's effectively always ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then make it clear in the name of the func that errors are ignored.

@deads2k
Copy link
Contributor Author

deads2k commented Jan 3, 2017

comments addressed.

@deads2k
Copy link
Contributor Author

deads2k commented Jan 3, 2017

Tagged this as multiple rebases. Every time someone writes an e2e test or a controller which needs more permissions, I keep having to update on non-conflicting conflicts.

@sttts
Copy link
Contributor

sttts commented Jan 3, 2017

Just the func name comment left. Otherwise, lgtm

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 997e01f35a3e1ba9c506247baddca6fb282f4d58. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@deads2k
Copy link
Contributor Author

deads2k commented Jan 3, 2017

@k8s-bot gci gke e2e test this

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 997e01f35a3e1ba9c506247baddca6fb282f4d58. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit 997e01f35a3e1ba9c506247baddca6fb282f4d58. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 997e01f35a3e1ba9c506247baddca6fb282f4d58. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@deads2k deads2k added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/P1 labels Jan 3, 2017
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 834f193 into kubernetes:master Jan 3, 2017
@deads2k deads2k mentioned this pull request Jan 3, 2017
Closed
@euank euank mentioned this pull request Jan 4, 2017
Closed
euank added a commit to euank/kubernetes that referenced this pull request Jan 5, 2017
@euank
cluster/cl: move abac to rbac
c1afc4a 
See kubernetes#39092

We based off of GCI in the brief time where it was using abac.
@euank euank mentioned this pull request Jan 5, 2017
Merged
k8s-github-robot pushed a commit that referenced this pull request Jan 5, 2017
Merge pull request #39451 from euank/remove-abac
8ef6902 
Automatic merge from submit-queue

cluster/cl: move abac to rbac

See #39092

We based off of GCI in the brief time where it was using abac.

fixes #39395

cc @yifan-gu 

**Release note**:
```release-note
NONE
```
@ixdy ixdy mentioned this pull request Jan 6, 2017
Closed
@Random-Liu Random-Liu mentioned this pull request Jan 10, 2017
Closed
@deads2k deads2k deleted the rbac-31-remove-abac branch February 1, 2017 17:29
berryjam pushed a commit to berryjam/kubernetes that referenced this pull request Aug 18, 2017
@euank @berryjamcoding
cluster/cl: move abac to rbac
2d0376c 
See kubernetes#39092

We based off of GCI in the brief time where it was using abac.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@justinsb justinsb justinsb requested changes

Assignees

@cjcullen cjcullen

Labels
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. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@deads2k @k8s-reviewable @smarterclayton @sttts @k8s-ci-robot @k8s-github-robot @justinsb @cjcullen @ixdy