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

Jump to bottom

Use the rbac API when creating and deleting policy #556

Merged
merged 7 commits into from Nov 29, 2017
Merged

Use the rbac API when creating and deleting policy #556

merged 7 commits into from Nov 29, 2017

Conversation

rthallisey
Copy link
Contributor

@rthallisey rthallisey commented Nov 15, 2017 •
edited

Describe what this PR does and why we need it:
Use the rbac API to create Roles.

Changes proposed in this pull request

  • Switch from using a file to create RBAC policy to using the kubernetes API
  • Setup the runtime pkg to abstract resource calls from the broker

depends-on: #561
depends-on: #562
Partially-fixes: #233

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 15, 2017
@rthallisey rthallisey added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 15, 2017
@coveralls
Copy link

coveralls commented Nov 15, 2017 •
edited

Coverage Status

Changes Unknown when pulling 145fe5a on rthallisey:rbac-api into ** on openshift:master**.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 16, 2017
@coveralls
Copy link

coveralls commented Nov 16, 2017 •
edited

Coverage Status

Changes Unknown when pulling 8db28ad on rthallisey:rbac-api into ** on openshift:master**.

djzager
djzager requested changes Nov 16, 2017
View reviewed changes
Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

If you could have a separate PR for the vendor stuff, that would be fantastic. Tough to see the meaningful changes when 269 files are involved.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 17, 2017
@rthallisey
Copy link
Contributor Author

(25) Agnostic Runtime phase 2

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9876882 on rthallisey:rbac-api into ** on openshift:master**.

shawn-hurley
shawn-hurley approved these changes Nov 27, 2017
View reviewed changes
Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

VISACK

djzager
djzager approved these changes Nov 27, 2017
View reviewed changes
Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

LGTM

@rthallisey rthallisey changed the title [WIP] Use the rbac API when creating policy Use the rbac API when creating policy Nov 27, 2017
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 27, 2017
@rthallisey
Copy link
Contributor Author

@djzager @shawn-hurley I made a bunch of changes since your reviews. Going to leave the do-not-merge tag around until you get a chance to review it again.

@coveralls
Copy link

coveralls commented Nov 27, 2017 •
edited

Coverage Status

Changes Unknown when pulling 9c7c5a4 on rthallisey:rbac-api into ** on openshift:master**.

shawn-hurley
shawn-hurley approved these changes Nov 28, 2017
View reviewed changes
Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

CONDACK, want to discuss the thoughts that I had.

pkg/apb/svc_acct.go Outdated
}

return rFilePath, nil
p := runtime.Provider{Log: s.log}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the runtime provider just be an apb package variable? Also, should we remove this function and just call the provider from the execute APB? These are just thoughts, all around this looks good.

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 agree that the runtime var should be an available variable and it needs to be wherever we make cluster calls. I haven't figured out yet how I'm going to approach providing it yet. You're ahead of the game :).

Agree that this function doesn't need to be called. All the guts are now in the runtime pkg.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling acf58e2 on rthallisey:rbac-api into ** on openshift:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 2224602 on rthallisey:rbac-api into ** on openshift:master**.

@rthallisey rthallisey removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 29, 2017
@rthallisey rthallisey changed the title Use the rbac API when creating policy Use the rbac API when creating and deleting policy Nov 29, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b1aab44 on rthallisey:rbac-api into ** on openshift:master**.

@coveralls
Copy link

coveralls commented Nov 29, 2017 •
edited

Coverage Status

Changes Unknown when pulling bd9395f on rthallisey:rbac-api into ** on openshift:master**.

jmrodri
jmrodri approved these changes Nov 29, 2017
View reviewed changes
Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

ACK

@rthallisey rthallisey merged commit b8eb864 into openshift:master Nov 29, 2017
@rthallisey rthallisey deleted the rbac-api branch November 29, 2017 20:42
@rthallisey rthallisey mentioned this pull request Dec 24, 2017
Closed
jianzhangbjz pushed a commit to jianzhangbjz/ansible-service-broker that referenced this pull request May 17, 2018
Use the rbac API when creating and deleting policy (openshift#556)
d025979 
* Use the rbac API when creating policy

* Use the client pkg to hide some of the resource creation

* Add the runtime pkg for additional abstraction

* Fix tests

* Distribute provide variable

* Use the api to destroy the apb sandbox

* Add destroysandbox to the fake provider
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmrodri jmrodri jmrodri approved these changes

@djzager djzager djzager approved these changes

@shawn-hurley shawn-hurley shawn-hurley approved these changes

Assignees
No one assigned
Labels
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.

Proposal: Support both Openshift and Kubernetes
6 participants
@rthallisey @coveralls @jmrodri @djzager @shawn-hurley @openshift-ci-robot