Bootstrap Kube namespaced roles and bindings #16517

enj · 2017-09-22T18:55:13Z

This change brings in the upstream namespaced roles that are used by controllers. These will never conflict with the openshift ones since they are required to be in namespaces prefixed with "kube-".

Signed-off-by: Monis Khan [email protected]

deads2k · 2017-09-22T18:59:58Z

pkg/cmd/server/admin/create_bootstrappolicy_file.go

@@ -53,6 +55,7 @@ func NewCommandCreateBootstrapPolicyFile(commandName string, fullName string, ou

 	flags.StringVar(&options.File, "filename", DefaultPolicyFile, "The policy template file that will be written with roles and bindings.")
 	flags.StringVar(&options.OpenShiftSharedResourcesNamespace, "openshift-namespace", "openshift", "Namespace for shared resources.")
+	flags.MarkDeprecated("openshift-namespace", "this field is no longer supported and using it can lead to undefined behavior")


this entire command should just die.

deads2k · 2017-09-22T19:02:20Z

pkg/cmd/server/admin/create_bootstrappolicy_file.go

@@ -53,6 +55,7 @@ func NewCommandCreateBootstrapPolicyFile(commandName string, fullName string, ou

 	flags.StringVar(&options.File, "filename", DefaultPolicyFile, "The policy template file that will be written with roles and bindings.")
 	flags.StringVar(&options.OpenShiftSharedResourcesNamespace, "openshift-namespace", "openshift", "Namespace for shared resources.")
+	flags.MarkDeprecated("openshift-namespace", "this field is no longer supported and using it can lead to undefined behavior")


We got rid of openshift-infra choice. I don't remember removing the shared namespace choice. If openshift-infra was changed, we simply created what needed to be. If they used a custom shared namespace, we cannot recreate that for them.

@smarterclayton its easier if we remove it, but I'm not sure about breaks. Opinion?

We got rid of openshift-infra choice. I don't remember removing the shared namespace choice. If openshift-infra was changed, we simply created what needed to be. If they used a custom shared namespace, we cannot recreate that for them.

@smarterclayton its easier if we remove it, but I'm not sure about breaks. Opinion?

Ansible doesn't allow it to be set. If a user changes it, I think its up to him to make sure all his roles are ready

I think we should remove it.

Deprecate in 3.7 and remove in 3.8 seems reasonable to me. Removing it completely in 3.7 seems a bit sudden, but I have a feeling that no sane person ever messed with this so I can stomp on it if you really want.

Deprecate in 3.7 and remove in 3.8 seems reasonable to me. Removing it completely in 3.7 seems a bit sudden, but I have a feeling that no sane person ever messed with this so I can stomp on it if you really want.

@enj You already have removed it in this pull. You just don't realize that you have. It no longer functions as it used to.

Hm yeah I killed the bootstrapping of it which is what matters.

deads2k · 2017-09-22T19:04:12Z

pkg/cmd/server/bootstrappolicy/namespace_policy.go

+	namespaceRoleBindings := map[string][]rbac.RoleBinding{}
+
+	addNamespaceRole(namespaceRoles,
+		DefaultOpenShiftSharedResourcesNamespace,


It ripples down to here.

deads2k · 2017-09-22T19:04:23Z

pkg/cmd/server/bootstrappolicy/policy.go

@@ -87,32 +89,6 @@ var (
 	legacyNetworkGroup  = networkapi.LegacyGroupName
 )

-func GetBootstrapOpenshiftRoles(openshiftNamespace string) []rbac.Role {


https://github.com/openshift/origin/pull/16512/files keeps this

deads2k · 2017-09-25T18:56:48Z

pkg/cmd/server/bootstrappolicy/namespace_policy.go

+
+func buildNamespaceRolesAndBindings() (map[string][]rbac.Role, map[string][]rbac.RoleBinding) {
+	// namespaceRoles is a map of namespace to slice of roles to create
+	namespaceRoles := map[string][]rbac.Role{}


I prefer the private, global map upstream has.

I prefer the approach that @liggitt started upstream with plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go, especially since it allows us to mutate the output easily.

I prefer the approach that @liggitt started upstream with plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go, especially since it allows us to mutate the output easily.

Letting you do something you shouldn't do isn't a point in its favor. Unbork the output and you can have it though.

deads2k · 2017-09-25T18:58:08Z

pkg/cmd/server/bootstrappolicy/policy.go

+	for _, roles := range ret {
+		for i := range roles {
+			role := &roles[i]
+			addDefaultMetadata(role)


upstream adds it as they go and I think has a test to ensure everyone gets it.

deads2k · 2017-09-25T18:58:30Z

test/testdata/bootstrappolicy/bootstrap_cluster_roles.yaml

    creationTimestamp: null
+    labels:
+      kubernetes.io/bootstrapping: rbac-defaults


don't add this

enj · 2017-09-26T16:05:59Z

@deads2k label removed. #16512 (review) ?

deads2k · 2017-09-26T16:56:34Z

pkg/cmd/server/bootstrappolicy/namespace_policy.go

+	"github.com/golang/glog"
+)
+
+func addNamespaceRole(namespaceRoles map[string][]rbac.Role, namespace string, role rbac.Role) {


Not seeing a use-case where these shouldn't be auto-reconciled. Add the annotation here.

deads2k · 2017-09-26T16:57:48Z

pkg/cmd/server/bootstrappolicy/policy.go

+	for _, roles := range ret {
+		for i := range roles {
+			role := &roles[i]
+			addDefaultMetadata(role)


don't mutate the upstream roles.

deads2k · 2017-09-26T16:57:58Z

pkg/cmd/server/bootstrappolicy/policy.go

@@ -1047,6 +1016,11 @@ func GetBootstrapClusterRoleBindings() []rbac.ClusterRoleBinding {
 		}
 	}

+	for i := range finalClusterRoleBindings {
+		roleBinding := &finalClusterRoleBindings[i]
+		addDefaultMetadata(roleBinding)


Why are we mutating upstream roles.

deads2k · 2017-09-26T19:43:06Z

pkg/cmd/server/bootstrappolicy/policy.go

+	// add default metadata only to openshift cluster roles
+	for i := range finalClusterRoles {
+		role := &finalClusterRoles[i]
+		addDefaultMetadata(role)


Seems like this should be done when you're building the roles. What is the use-case for having GetOpenshiftBootstrapClusterRoles to be missing annotations?

deads2k · 2017-09-26T19:43:58Z

pkg/cmd/server/bootstrappolicy/policy.go

+	// add default metadata only to openshift cluster role bindings
+	for i := range finalClusterRoleBindings {
+		roleBinding := &finalClusterRoleBindings[i]
+		addDefaultMetadata(roleBinding)


What is the use-case for GetOpenshiftBootstrapClusterRoleBindings to be missing annotations?

deads2k · 2017-09-26T19:44:28Z

pkg/cmd/server/bootstrappolicy/policy.go

+func GetBootstrapNamespaceRoles() map[string][]rbac.Role {
+	// openshift and kube are guaranteed never to conflict on these
+	// the openshift map is safe to mutate unlike the kube one
+	ret := NamespaceRoles()


What is the use-case for this to return roles missing annotations?

deads2k · 2017-09-26T19:44:42Z

pkg/cmd/server/bootstrappolicy/policy.go

+func GetBootstrapNamespaceRoleBindings() map[string][]rbac.RoleBinding {
+	// openshift and kube are guaranteed never to conflict on these
+	// the openshift map is safe to mutate unlike the kube one
+	ret := NamespaceRoleBindings()


What is the use-case for this to be missing annotations?

This change brings in the upstream namespaced roles that are used by controllers. These will never conflict with the openshift ones since they are required to be in namespaces prefixed with "kube-". Signed-off-by: Monis Khan <[email protected]>

Signed-off-by: Monis Khan <[email protected]>

enj · 2017-09-27T03:20:42Z

@deads2k

deads2k · 2017-09-27T12:19:54Z

/retest
/lgtm

openshift-merge-robot · 2017-09-27T12:20:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, enj

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~contrib/completions/OWNERS~~ [deads2k]
~~pkg/authorization/OWNERS~~ [deads2k,enj]
~~pkg/cmd/OWNERS~~ [deads2k,enj]
~~test/testdata/OWNERS~~ [deads2k,enj]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

deads2k · 2017-09-27T14:09:17Z

test integration flaked. killing the rest to free our queue

deads2k · 2017-09-27T14:10:46Z

/retest

openshift-merge-robot · 2017-09-27T17:21:45Z

Automatic merge from submit-queue

…e catalog example template. Upstream role was added in openshift#16517.

openshift-merge-robot assigned abstractj and jim-minter Sep 22, 2017

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 22, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2017

deads2k reviewed Sep 22, 2017

View reviewed changes

deads2k reviewed Sep 25, 2017

View reviewed changes

enj force-pushed the enj/i/kube_bootstrap_roles branch from eac5ffd to 8369288 Compare September 26, 2017 15:52

enj mentioned this pull request Sep 26, 2017

use the upstream namespaced roles #16512

Closed

enj force-pushed the enj/i/kube_bootstrap_roles branch from 8369288 to f368abf Compare September 26, 2017 16:24

openshift-merge-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2017

deads2k reviewed Sep 26, 2017

View reviewed changes

openshift deleted a comment from enj Sep 26, 2017

enj force-pushed the enj/i/kube_bootstrap_roles branch from f368abf to 6fa5344 Compare September 26, 2017 18:41

deads2k reviewed Sep 26, 2017

View reviewed changes

enj added 2 commits September 26, 2017 23:15

Generated changes to bootstrappolicy test data

f42b2d7

Signed-off-by: Monis Khan <[email protected]>

enj force-pushed the enj/i/kube_bootstrap_roles branch from 6fa5344 to f42b2d7 Compare September 27, 2017 03:20

enj unassigned abstractj Sep 27, 2017

openshift-ci-robot assigned deads2k Sep 27, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2017

openshift-merge-robot merged commit 56bc677 into openshift:master Sep 27, 2017

deads2k mentioned this pull request Sep 28, 2017

Allow bootstrap configuration to be configured and reentrant #16571

Merged

jim-minter pushed a commit to jim-minter/origin that referenced this pull request Sep 28, 2017

Remove unneeded extension-apiserver-authentication-reader from servic…

24f8e57

…e catalog example template. Upstream role was added in openshift#16517.

jim-minter mentioned this pull request Sep 28, 2017

Remove unneeded extension-apiserver-authentication-reader from service catalog example template. #16600

Merged

jim-minter pushed a commit to jim-minter/origin that referenced this pull request Sep 28, 2017

Remove unneeded extension-apiserver-authentication-reader from servic…

036808f

…e catalog example template. Upstream role was added in openshift#16517.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bootstrap Kube namespaced roles and bindings #16517

Bootstrap Kube namespaced roles and bindings #16517

enj commented Sep 22, 2017

deads2k Sep 22, 2017

enj Sep 22, 2017

deads2k Sep 22, 2017

deads2k Sep 25, 2017

smarterclayton Sep 26, 2017

enj Sep 26, 2017

deads2k Sep 26, 2017

enj Sep 26, 2017

deads2k Sep 22, 2017

deads2k Sep 22, 2017

deads2k Sep 25, 2017

enj Sep 26, 2017

deads2k Sep 26, 2017

deads2k Sep 25, 2017

deads2k Sep 25, 2017

enj commented Sep 26, 2017

deads2k Sep 26, 2017

deads2k Sep 26, 2017

deads2k Sep 26, 2017

deads2k Sep 26, 2017

deads2k Sep 26, 2017

deads2k Sep 26, 2017

deads2k Sep 26, 2017

enj commented Sep 27, 2017

deads2k commented Sep 27, 2017

openshift-merge-robot commented Sep 27, 2017

deads2k commented Sep 27, 2017

deads2k commented Sep 27, 2017

openshift-merge-robot commented Sep 27, 2017

Bootstrap Kube namespaced roles and bindings #16517

Bootstrap Kube namespaced roles and bindings #16517

Conversation

enj commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enj commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enj commented Sep 27, 2017

deads2k commented Sep 27, 2017

openshift-merge-robot commented Sep 27, 2017

deads2k commented Sep 27, 2017

deads2k commented Sep 27, 2017

openshift-merge-robot commented Sep 27, 2017