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

Changing the default for auto escalate to false #503

Merged
merged 2 commits into from
Nov 1, 2017

Conversation

shawn-hurley
Copy link
Contributor

  • Also since we are now using 3.7 moves to using kubernetes rbac.

Describe what this PR does and why we need it:
Changing the default for auto escalate to false. Currently, we are by default testing no authorization code paths, this change will turn on the authorization code paths by default.

Changes proposed in this pull request

  • update apiGroup and kind for rbac resources to use kubernetes and not openshift in the deployment template
  • update default value for deployment template parameter AUTO_ESCALATE to false
  • update default for prep_local_devel_env.sh to false when generating the configuration.

Does this PR depend on another PR (Use this to track when PRs should be merged)
depends-on
Should be merged with fusor/catasb#168

Which issue this PR fixes (This will close that issue when PR gets merged)
fixes #429

* Also since we are now using 3.7 move to using kubernetes rbac.
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 18, 2017
@shawn-hurley shawn-hurley added enhancement needs-review and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 18, 2017
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.

VISIACK

@shawn-hurley
Copy link
Contributor Author

This can be merged after #522 is merged.

@rthallisey
Copy link
Contributor

Currently, we are by default testing no authorization code paths, this change will turn on the
authorization code paths by default.

@shawn-hurley I'm all for us testing the auth code paths, but is disabling auto_escalate something that could break broker developers and apb developers when running off of master? Also when we move to Kubernetes, I think we'll need this defaulted to true for anything to work. What I'm getting at is should we merge disabling auto_escalate into master? Or what are your thoughts on disabling auto_escalate only in our release branches?

@shawn-hurley
Copy link
Contributor Author

@rthallisey What do you think about making the deployment template and the generated script to default to true for developers, and moving the default to false for catasb (which has been merged) and then we can set it to true when using the kubernetes playbooks?

@rthallisey
Copy link
Contributor

My opinion is that I would like for us to be using the broker like it would be used in production, but if you're using the master branch then I would expect the developer experience to be priority so some of the security is going to be off.

@rthallisey What do you think about making the deployment template and the generated script to
default to true for developers, and moving the default to false for catasb (which has been merged)
and then we can set it to true when using the kubernetes playbooks

@shawn-hurley I'm good with that. I think that covers it.

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 31, 2017
@rthallisey rthallisey merged commit ddae7be into openshift:master Nov 1, 2017
@enj
Copy link

enj commented Nov 1, 2017

Seems like you are still auto escalating? To me it makes more sense to always have auto escalating be false and just provision a powerful user in test environments.

@shawn-hurley
Copy link
Contributor Author

@enj all of the downstream environments will be using the correct setting, and in fact, the default for most of the developer's dev environment will also be set false (fusor/catasb#168). The only things that are set to true, are our prep_local_devel_env.shand the run_latest_build.sh which is meant for only a testing environment to be spun up quickly.

If the above still makes you uncomfortable, what if we change the default for the template to false and override the parameter for the two cases below?

@enj
Copy link

enj commented Nov 1, 2017

Generally I feel a lot more comfortable now that you pointed out fusor/catasb#168, but some thoughts:

The only things that are set to true, are our prep_local_devel_env.shand the run_latest_build.sh which is meant for only a testing environment to be spun up quickly

But why do those need to be different than everyone else's environments?

If the above still makes you uncomfortable, what if we change the default for the template to false and override the parameter for the two cases below?

That seems OK too, but since you are creating a custom environment, why not just grant extra rights to your custom dev user (I am assuming you have something like that)? Then its the user that is doing the action that is different in dev (instead of the entire auth code path being skipped).

@shawn-hurley
Copy link
Contributor Author

@enj the user's that we create will have full access to their namespaces, I think you are correct that it will not affect development in any way. @rthallisey can you please comment if you feel otherwise?

I will make another PR to move this back to false as the default, and will not override in the two scripts.

@rthallisey
Copy link
Contributor

The only things that are set to true, are our prep_local_devel_env.shand the run_latest_build.sh
which is meant for only a testing environment to be spun up quickly

But why do those need to be different than everyone else's environments?

If you're running locally, then you're doing development primarily on the broker. So the expectation is you are going to be in 'privileged mode'. Like you said @enj, I think while using a local broker we can bump the user's permissions way up so we still use the auth code path. I think that's a better solution.

@shawn-hurley lets turn auto-escalate to false and change the service account permissions in the local script.

Let me know if folks disagree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-review size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move to RBAC authorization when moving broker to 3.7
5 participants