enable service catalog in oc cluster up #14630

bparees · 2017-06-14T05:19:13Z

No description provided.

bparees · 2017-06-14T05:26:59Z

@csrwng @pmorie @deads2k @smarterclayton @jwforres @jim-minter i'm moving my service catalog enabling PR over here because my other branch got killed by rebasing on top of aggregator behavior that seems to have broken things (i'm not saying the aggregator is broken, but i wanted to go back to something that worked w/ my existing code since i don't know what changes i may need to make to adapt to whatever changed recently). (yes i could have stomped the other branch instead of doing this).

Anyway, i think this represents a mostly final state of the roles/permissions/etc. open items:

this is currently granting anonymous access to the TSB. that's being discussed elsewhere.
i added the default project role but it's not conditioned on SC enablement, i didn't see how the default project template had any context for that, and it doesn't really seem problematic to grant the role to users regardless of whether the SC exists.
still needs oadm migrate logic to add the service-catalog-user-role to existing users/projects.

if you want to run this:

grab the branch
build it
build a new openshift/origin image w/ the new openshift binary in your preferred fashion
run oc cluster up --version=latest --service-catalog=true

bparees · 2017-06-14T05:28:34Z

examples/service-catalog/service-catalog.yaml

+  metadata:
+    name: service-catalog-controller
+
+- kind: ClusterRoleBinding


the controversial one.

bparees · 2017-06-14T05:30:15Z

examples/service-catalog/service-catalog.yaml

+  userNames:
+    - system:serviceaccount:service-catalog:service-catalog-controller
+
+- kind: ClusterRole


allow controller to manage podpresets in user's namespaces.

bparees · 2017-06-14T05:30:36Z

examples/service-catalog/service-catalog.yaml

+  userNames:
+    - system:serviceaccount:service-catalog:service-catalog-controller
+
+- kind: ClusterRole


allow controller to update sc object statuses

bparees · 2017-06-14T05:31:13Z

examples/service-catalog/service-catalog.yaml

+    - system:serviceaccount:service-catalog:service-catalog-controller
+
+
+- kind: PolicyBinding


allow controller to access endpoints for etcd logic.

bparees · 2017-06-14T05:31:56Z

examples/service-catalog/service-catalog.yaml

+  userNames:
+    - system:serviceaccount:service-catalog:service-catalog-controller
+
+- kind: PolicyBinding


allow api server to read configmaps in kube-system namespace for delegated auth support

bparees · 2017-06-14T05:32:13Z

examples/service-catalog/service-catalog.yaml

+  userNames:
+    - system:serviceaccount:service-catalog:default
+
+- kind: ClusterRoleBinding


allow apiserver to do delegated auth

bparees · 2017-06-14T05:32:42Z

examples/service-catalog/service-catalog.yaml

+    - system:serviceaccount:service-catalog:default
+
+
+- kind: ClusterRole


allow any user to view available serviceclasses (cluster resource)

bparees · 2017-06-14T05:43:41Z

@jwforres

--- FAIL: TestSystemOnlyRoles (0.01s)
	web_console_role_test.go:77: The list of expected end user roles has been changed.  Please discuss with the web console team to update role annotations.
	web_console_role_test.go:79: These roles are visible but not in rolesToShow: [service-catalog-user]

let's discuss as requested :)

jim-minter · 2017-06-14T09:40:18Z

this is currently granting anonymous access to the TSB. that's being discussed elsewhere.

I'm not actually sure where that's being discussed, please do you have a link?

jim-minter · 2017-06-14T09:41:43Z

still needs oadm migrate logic to add the service-catalog-user-role to existing users/projects.

Yikes - as part of an oc cluster up card?

jim-minter · 2017-06-14T09:46:39Z

let's discuss as requested :)

Is this just indicating that the service-catalog-user-role should be annotated with "authorization.openshift.io/system-only" ?

Also is it ruled out just to add the appropriate rules into the existing view and edit roles and save the migration work?

(We did that recently on the template service broker side to save the additional complexity, btw)

Expanding on this - it seems hard work that this PR is (a) doing some RBAC changes in bootstrappolicy and some in a template (it's half in, half out), and (b) has an implication on migration. Is it possible to either move the ServiceCatalogUserRoleName bits into view and edit within bootstrap policy, or maybe to live oc patch these into view and edit from a separate yaml file sitting in examples/service-catalog?

jim-minter · 2017-06-14T10:08:24Z

examples/service-catalog/service-catalog.yaml

+  metadata:
+    name: namespace-viewer
+  rules:
+  - apiGroups:


in several places in this file, I wonder whether this must be

- apiGroups: - ""

(note additional second line). I wonder whether as it stands what you have means "all API groups" rather than "just kubernetes core"?

not sure but i'll add it.

jim-minter · 2017-06-14T10:16:37Z

examples/service-catalog/service-catalog.yaml

+- kind: ClusterRole
+  apiVersion: v1
+  metadata:
+    name: secret-admin


ClusterRoles can contain multiple rules - is it possible to squash all these clusterroles down to approximately 'this cluster role A has everything for a sc controller' and 'this cluster role B has everything for a sc apiserver', then 2 clusterrolebindings against these clusterroles + any other clusterrolebindings required (e.g. template-broker-caller-binding)

Also a rule can contain multiple resources and I think a cartesian product is used internally. Thus 2 rules can be condensed when they are equal in all respects except the resources (and don't need to be separated for different bindings). I think secret-admin and podpreset-admin are cases in point - they don't need to be separate rules on a clusterrole, but can be 2 resources in 1 rule in 1 clusterrole.

i'll take a crack at it.

jwforres · 2017-06-14T10:23:19Z

On Jun 14, 2017 5:46 AM, "Jim Minter" <[email protected]> wrote: let's discuss as requested :) Is this just indicating that the service-catalog-user-role should be annotated with "authorization.openshift.io/system-only" ? Yes exactly. If the role should not appear to end users in the Project membership page role dropdown then it needs to be added to the list that gets the annotation. For most infrastructure things the answer is that it should not. Also is it ruled out just to add the appropriate rules into the existing view and edit roles and save the migration work? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14630 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7Qvj7itQzPk39_3R6J-g7CIdMkFUks5sD6wIgaJpZM4N5ZOB> .

jim-minter · 2017-06-14T10:29:00Z

examples/service-catalog/service-catalog.yaml

+    - system:serviceaccount:service-catalog:service-catalog-controller
+
+
+- kind: PolicyBinding


I think all the PolicyBinding objects can go away when #14547 merges?

that's correct

jim-minter · 2017-06-14T10:32:43Z

examples/service-catalog/service-catalog.yaml

+  name: service-catalog
+objects:
+
+- kind: ServiceAccount


(nice to have) would it be possible to have a named SA for the api server as well?

i figured we might as well use the default SA for something but yeah, i can rework it.

jim-minter · 2017-06-14T10:39:40Z

pkg/bootstrap/docker/openshift/servicecatalog.go

+
+import (
+	"bytes"
+	//	"crypto/tls"


remove the // imports ?

and maybe some of the newlines?

thanks for the catch. been a lot of iterating on this one.

jim-minter · 2017-06-14T10:40:50Z

don't know if there are any knock-on effects of this PR on oc cluster status - perhaps a follow-up issue.

jim-minter · 2017-06-14T10:44:55Z

pkg/bootstrap/docker/openshift/helper.go

@@ -709,6 +722,92 @@ func (h *Helper) updateConfig(configDir string, opt *StartOptions) error {
 		cfg.AdmissionConfig.PluginConfig[defaultsapi.BuildDefaultsPlugin] = buildDefaultsConfig
 	}

+	if opt.ServiceCatalog {
+		cfg.TemplateServiceBrokerConfig = &configapi.TemplateServiceBrokerConfig{
+			TemplateNamespaces: []string{"openshift"},


there's a const openshiftNamespace but it might be located in the wrong package (pkg/bootstrap/docker) - maybe not this PR

no time like the present. will update.

deads2k · 2017-06-14T11:09:19Z

(i'm not saying the aggregator is broken

it's broken until #14594 makes it through the queue.

bparees · 2017-06-14T12:56:03Z

I'm not actually sure where that's being discussed, please do you have a link?

it's being discussed here: #14623

Yikes - as part of an oc cluster up card?

since i'm adding the default project role as part of this work (because it is needed for the oc cluster up feature to work), my understanding is i don't really have a choice but to also add the migration for it at this point.

Is this just indicating that the service-catalog-user-role should be annotated with "authorization.openshift.io/system-only" ?

well it's indicating the discussion should happen. i haven't decided myself if that's the right fix or not, that's why i wanted to talk to @jwforres first.

Also is it ruled out just to add the appropriate rules into the existing view and edit roles and save the migration work?
(We did that recently on the template service broker side to save the additional complexity, btw)

that's a great question. @deads2k can we do that?

Expanding on this - it seems hard work that this PR is (a) doing some RBAC changes in bootstrappolicy and some in a template (it's half in, half out), and (b) has an implication on migration. Is it possible to either move the ServiceCatalogUserRoleName bits into view and edit within bootstrap policy, or maybe to live oc patch these into view and edit from a separate yaml file sitting in examples/service-catalog?

moving the SCuserRole to be part of view/edit seems plausible if @deads2k is ok with it.

as for live patching into view and edit, specifically for oc cluster up, i'd rather just do the right thing now than do that for oc cluster up for now and have to revisit this when someone wants to enable TSB on a real cluster.

bparees · 2017-06-14T12:57:44Z

Yes exactly. If the role should not appear to end users in the Project
membership page role dropdown then it needs to be added to the list that
gets the annotation. For most infrastructure things the answer is that it
should not.

right, well i wasn't sure if i'd qualify it as an infrastructure thing or not. SC instances+bindings (what this lets you create/view) are pretty user facing. This is effectively "user can use the service catalog" permission.

I'm happy to mark it system if that's what we agree on though. (and even happier to just roll into into edit and forget about it)

deads2k · 2017-06-14T13:21:57Z

that's a great question. @deads2k can we do that?

I really dislike the idea of granting permissions to alpha/unsupported features in stock roles because they will never be removed in a normal upgrade flow. If something changes and types become restricted or some such, there is no "normal" way to fix up an existing, non-alpha installation.

bparees · 2017-06-14T13:59:24Z

I really dislike the idea of granting permissions to alpha/unsupported features in stock roles because they will never be removed in a normal upgrade flow. If something changes and types become restricted or some such, there is no "normal" way to fix up an existing, non-alpha installation.

please point me to the migration logic i need to edit.

deads2k · 2017-06-14T14:04:49Z

please point me to the migration logic i need to edit.

It doesn't exist. You'd have to write something from scratch. The reconcile commands never run with tightening on and it isn't selective because that carries a high risk of breaking existing clusters.

I don't think you should modify admin, edit, or view roles to include unsupported resources.

deads2k · 2017-06-14T14:05:17Z

Oh, and we don't directly own reconcile command after 3.6 since we'll be using the RBAC technique.

bparees · 2017-06-14T14:18:02Z

I talked to @smarterclayton and i'm going to go w/ the patch approach @jim-minter suggested.

bparees · 2017-06-14T17:04:16Z

@jim-minter i think i've addressed all your comments, ptal?

@smarterclayton @deads2k pending review from @jim-minter and @csrwng this is ready to go except for removing the "grant anonymous access to the TSB" binding (and the associated caveat text on the enablement flag help).

bparees · 2017-06-14T19:03:57Z

[test]

bparees · 2017-06-14T23:04:38Z

@smarterclayton message added:

$ oc cluster up --version=latest --service-catalog=true
Starting OpenShift using openshift/origin:latest ...
OpenShift server started.

The server is accessible via web console at:
    https://127.0.0.1:8443

You are logged in as:
    User:     developer

To login as administrator:
    oc login -u system:admin

In order to enable access to the Template Service Broker for use with the Service Catalog, you must first grant unauthenticated access to the template service broker api.

WARNING: Enabling this access allows anyone who can see your cluster api server to provision templates within your cluster, impersonating any user in the cluster (including administrators).  This can be used to gain full administrative access to your cluster.  Do not allow this access unless you fully understand the implications.  To enable unauthenticated access to the template service broker api, run the following command as cluster admin:

############################

echo "\
---
apiVersion: v1
groupNames:
  - "system:unauthenticated"
  - "system:authenticated"
kind: ClusterRoleBinding
metadata:
  name: template-broker-caller-binding
roleRef:
  name: "system:openshift:templateservicebroker-client"
" | oc create -f -

############################

WARNING: Running the above command allows unauthenticated users to access and potentially exploit your cluster.

smarterclayton · 2017-06-15T04:38:04Z

Why not `oc adm policy add-cluster-role-to-group system:openshift:templateservicebroker-client system:unauthenticated system:authenticated`?

…

On Thu, Jun 15, 2017 at 12:06 AM, OpenShift Bot ***@***.***> wrote: continuous-integration/openshift-jenkins/test Running ( https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2253/) (Base Commit: 811a267 <811a267> ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14630 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pzNoLeuGxUnb3eCwqsVHPdix-WCDks5sEK2pgaJpZM4N5ZOB> .

bparees · 2017-06-15T04:39:39Z

because that would be way too easy for our users....

i'll update it.

bparees · 2017-06-15T05:02:33Z

@smarterclayton updated.

bparees · 2017-06-15T18:35:41Z

added changes for #14676 as a new commit. May need to drop the cherry-picked commits before merging depending on whether @deads2k's PR changes before it merges.

@smarterclayton final sign off on the TSB warning message/instructions?

smarterclayton · 2017-06-16T00:59:19Z

Messages are fine

bparees · 2017-06-16T15:01:58Z

[merge][severity:blocker]

openshift-bot · 2017-06-16T15:08:21Z

continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 6

openshift-bot · 2017-06-16T15:08:21Z

Evaluated for origin merge up to 4cd9479

bparees · 2017-06-16T17:11:46Z

flake #14708
[test]

openshift-bot · 2017-06-17T00:07:45Z

Evaluated for origin test up to 0a2cf7c

openshift-bot · 2017-06-17T02:09:24Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2348/) (Base Commit: cc06db0) (PR Branch Commit: 0a2cf7c)

bparees force-pushed the cluster2 branch 2 times, most recently from c866c7a to e4a3875 Compare June 14, 2017 05:26

bparees commented Jun 14, 2017

View reviewed changes

bparees force-pushed the cluster2 branch from e4a3875 to 7c6d57c Compare June 14, 2017 05:46

jim-minter reviewed Jun 14, 2017

View reviewed changes

bparees force-pushed the cluster2 branch from 7c6d57c to 9aa6958 Compare June 14, 2017 17:02

bparees force-pushed the cluster2 branch 2 times, most recently from a5ccae9 to 16e447a Compare June 14, 2017 17:08

bparees mentioned this pull request Jun 14, 2017

[WIP] add service catalog support to oc cluster up #14528

Closed

bparees force-pushed the cluster2 branch 2 times, most recently from 67c65c4 to d5f4a2f Compare June 14, 2017 23:03

bparees force-pushed the cluster2 branch 3 times, most recently from 5284fe0 to 1caf0fb Compare June 15, 2017 03:54

bparees force-pushed the cluster2 branch from 1caf0fb to 5244002 Compare June 15, 2017 04:47

0xmichalis mentioned this pull request Jun 15, 2017

Add a submit queue pipeline openshift/release#35

Merged

bparees force-pushed the cluster2 branch from 493c526 to 7e9d989 Compare June 15, 2017 18:34

bparees force-pushed the cluster2 branch from 52c42dc to 4cd9479 Compare June 16, 2017 15:01

bparees force-pushed the cluster2 branch from 4cd9479 to 965bddc Compare June 16, 2017 23:53

install service catalog w/ oc cluster up

0a2cf7c

bparees force-pushed the cluster2 branch from 965bddc to 0a2cf7c Compare June 17, 2017 00:01

bparees mentioned this pull request Jun 17, 2017

Creation of service_catalog and broker roles openshift/openshift-ansible#4195

Merged

smarterclayton merged commit ddeb088 into openshift:master Jun 17, 2017

bparees deleted the cluster2 branch June 19, 2017 14:32

jorgemoralespou mentioned this pull request Jul 16, 2017

Expose service catalog option for OpenShift 3.6 minishift/minishift#1137

Closed

warmchang mentioned this pull request Aug 2, 2017

OCP 3.6 Release Notes openshift/openshift-docs#4714

Merged

		- system:serviceaccount:service-catalog:service-catalog-controller


		- kind: PolicyBinding

		- system:serviceaccount:service-catalog:default


		- kind: ClusterRole

enable service catalog in oc cluster up #14630

enable service catalog in oc cluster up #14630

Uh oh!

Conversation

bparees commented Jun 14, 2017

Uh oh!

bparees commented Jun 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparees commented Jun 14, 2017

Uh oh!

jim-minter commented Jun 14, 2017

Uh oh!

jim-minter commented Jun 14, 2017

Uh oh!

jim-minter commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwforres commented Jun 14, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jim-minter commented Jun 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Jun 14, 2017

Uh oh!

bparees commented Jun 14, 2017

Uh oh!

bparees commented Jun 14, 2017

Uh oh!

deads2k commented Jun 14, 2017

Uh oh!

bparees commented Jun 14, 2017

Uh oh!

deads2k commented Jun 14, 2017

Uh oh!

deads2k commented Jun 14, 2017

Uh oh!

bparees commented Jun 14, 2017

Uh oh!

bparees commented Jun 14, 2017

jim-minter commented Jun 14, 2017 •

edited

Loading

openshift-bot commented Jun 16, 2017 •

edited

Loading

bparees commented Jun 16, 2017 •

edited

Loading