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

removing subject rules review auth and using aggregated rules. #995

Merged

Conversation

shawn-hurley
Copy link
Contributor

@shawn-hurley shawn-hurley commented Jun 21, 2018

Describe what this PR does and why we need it:

Implementation of https://trello.com/c/fiOSsq1Q

Changes proposed in this pull request

  • remove origin package
  • remove rules review
  • use bundle-lib authorizer to determine if user ca

Does this PR depend on another PR (Use this to track when PRs should be merged)

depends-on
automationbroker/bundle-lib#112

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

coveralls commented Jun 21, 2018

Coverage Status

Coverage increased (+0.04%) to 38.907% when pulling 8eec92c on shawn-hurley:remove-subjectrulesreview-auth into 18a43ba on openshift:master.

Gopkg.toml Outdated
@@ -37,8 +37,10 @@
version = "~1.3.0"

[[constraint]]
version = "~0.2.0"
#version = "~0.2.0"
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming we don't want these changes in the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This depends on the bundle lib PR so will update once we get a release of bundle lib

@djzager
Copy link
Member

djzager commented Jun 26, 2018

If you are using aggregated reviews, does that impact our need for https://github.com/shawn-hurley/ansible-service-broker/blob/a85d2a365ac1fb293dae0951ee61320038649523/apb/tasks/main.yml#L8 ?

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.

A couple of apb/ changes, the rest LGTM.

kind: ClusterRole
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: asb-user-access
Copy link
Member

Choose a reason for hiding this comment

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

I think this name, since it's cluster scope, should be related to the {{ broker_name }}

Copy link
Member

Choose a reason for hiding this comment

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

or...maybe not, but at least not asb-user-access 😎

metadata:
name: asb-user-access
labels:
{% if broker_sandbox_role == 'admin' %}
Copy link
Member

Choose a reason for hiding this comment

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

I think it needs to be formatted more like:

labels:
{% if broker_sandbox_role == 'admin' %}
    rbac.authorization.k8s.io/aggregate-to-admin: "true"
{% endif %}
...

Otherwise, I think the whitespace is included.

@@ -0,0 +1,17 @@
---
kind: ClusterRole
Copy link
Member

Choose a reason for hiding this comment

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

The only thing I don't see is this template being used in deployment.

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 am very confused by this statement sorry can you explain what I need to do?

Copy link
Member

Choose a reason for hiding this comment

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

I would expect to see broker-user-auth.yaml somewhere in this list (https://github.com/shawn-hurley/ansible-service-broker/blob/a85d2a365ac1fb293dae0951ee61320038649523/apb/tasks/main.yml#L54) in order for the broker-user-auth ClusterRole to be created.

@shawn-hurley
Copy link
Contributor Author

shawn-hurley commented Jun 26, 2018 via email

@shawn-hurley shawn-hurley force-pushed the remove-subjectrulesreview-auth branch 2 times, most recently from 9cee5d8 to db56742 Compare June 27, 2018 23:07
@shawn-hurley shawn-hurley force-pushed the remove-subjectrulesreview-auth branch from ae7ca30 to 8eec92c Compare July 11, 2018 17:32
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants