Add parity check for Openshift policy and Kube RBAC #14429

enj · 2017-05-31T20:17:24Z

This change adds the oadm migrate authorization command:

A controller is used to keep Openshift authorization objects and Kubernetes RBAC in sync. This command checks for parity between those objects across all namespaces and reports all objects that are out of sync. These objects require manual intervention to sync as the controller handles all cases where automatic sync is possible.

The following resource types are checked by this command:

clusterrole
role
clusterrolebinding
rolebinding

No resources are mutated.

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

Fixes #14139
Prerequisite #14365 #14475

TODO

Update conversions to split Origin rules that contain both resource and non-resource URLs: Split resource and non-resource rules during conversion #14454
Add tests for this?

[test]

deads2k · 2017-06-02T14:31:16Z

pkg/cmd/admin/admin.go

@@ -93,6 +94,7 @@ func NewCommandAdmin(name, fullName string, in io.Reader, out io.Writer, errout
 					// Migration commands
 					migrateimages.NewCmdMigrateImageReferences("image-references", fullName+" "+migrate.MigrateRecommendedName+" image-references", f, in, out, errout),
 					migratestorage.NewCmdMigrateAPIStorage("storage", fullName+" "+migrate.MigrateRecommendedName+" storage", f, in, out, errout),
+					migratepolicy.NewCmdMigratePolicy("policy", fullName+" "+migrate.MigrateRecommendedName+" policy", f, in, out, errout),


authorization

deads2k · 2017-06-02T14:31:58Z

pkg/cmd/admin/migrate/migrator.go

+// ReporterBool implements the Reporter interface for a boolean.
+type ReporterBool bool
+
+func (r ReporterBool) Changed() bool { return bool(r) }


separate lines please

Blah missed this one, will clean up later since #14365 is already 9 deep in the merge queue.

deads2k · 2017-06-02T14:33:49Z

pkg/cmd/admin/migrate/policy/policy.go

+)
+
+type MigratePolicyOptions struct {
+	migrate.ResourceOptions


anonymously including this for some reason? I dislike anonymous includes.

Just following the pattern for every other migrate command.

deads2k · 2017-06-02T14:34:55Z

pkg/cmd/admin/migrate/policy/policy.go

+	// compare the results if there have been no errors so far
+	if len(errlist) == 0 {
+		// there's one wrinkle.  If `openshift.io/reconcile-protect` is to true, then we must set rbac.authorization.kubernetes.io/autoupdate to false
+		if convertedClusterRole.Annotations["openshift.io/reconcile-protect"] == "true" {


this looks very familiar. Restructure enough to be able to re-use the implementation for this with the controllers.

They are just different enough to not warrant trying to refactor, especially since we will delete the controller soon.

enj · 2017-06-06T21:22:13Z

@deads2k addressed comments as possible. Let me know if you want anything else changed. This is waiting on #14365 and #14475

deads2k · 2017-06-07T12:43:30Z

pkg/cmd/admin/migrate/authorization/authorization.go

+			Out:           out,
+			ErrOut:        errout,
+			AllNamespaces: true,
+			Include:       []string{"clusterrole", "role", "clusterrolebinding", "rolebinding"},


clusterrole.authorization.openshift.io, right? This could never successfully be run on a prior version since the RBAC side would be missing.

fabianofranz · 2017-06-07T20:09:37Z

lgtm from a cli perspective.

enj · 2017-06-08T18:56:11Z

@deads2k PTAL @ 3e7035f

enj · 2017-06-08T22:03:31Z

@deads2k 057ec4d

deads2k · 2017-06-09T13:28:40Z

pkg/authorization/controller/authorizationsync/normalize.go

 )

-// NormalizePolicyRules mutates the given rules and lowercases verbs, resources and API groups.
+func PrepareForCreateClusterRole(originClusterRole *authorizationapi.ClusterRole) (*rbac.ClusterRole, error) {


ConvertToRBACClusterRole

deads2k · 2017-06-09T13:29:32Z

pkg/authorization/controller/authorizationsync/normalize.go

+	return equivalentClusterRole, nil
+}
+
+func PrepareForUpdateClusterRole(equivalentClusterRole, rbacClusterRole *rbac.ClusterRole) bool {


godoc, particularly the mutation bit.

(newClusterRole, existingClusterRole *rbac.ClusterRole)

Invert the bool. Return true if an update is needed and doc it.

deads2k · 2017-06-09T13:31:33Z

pkg/authorization/controller/authorizationsync/normalize.go

+	return apiequality.Semantic.DeepEqual(equivalentRoleBinding, rbacRoleBinding)
+}
+
+func normalizeObjectMeta(a *v1.ObjectMeta, b v1.ObjectMeta) {


godoc explaining why you're doing this to these particular fields. Also, this could all be non mutating given shallow copies. Take value objects and return a value object.

deads2k · 2017-06-09T13:31:51Z

pkg/authorization/controller/authorizationsync/normalize.go

+	return apiequality.Semantic.DeepEqual(equivalentRoleBinding, rbacRoleBinding)
+}
+
+func normalizeObjectMeta(a *v1.ObjectMeta, b v1.ObjectMeta) {


This isn't normalization. This is "prepareObjectMetaForUpdate"

deads2k · 2017-06-09T13:32:29Z

pkg/authorization/controller/authorizationsync/origin_to_rbac_role_controller.go

 	glog.V(1).Infof("writing RBAC role %v/%v", namespace, name)
 	_, err = c.rbacClient.Roles(namespace).Update(equivalentRole)
 	// if the update was invalid, we're probably changing an immutable field or something like that
 	// either way, the existing object is wrong.  Delete it and try again.
 	if apierrors.IsInvalid(err) {
-		c.rbacClient.Roles(namespace).Delete(name, nil)
+		c.rbacClient.Roles(namespace).Delete(name, nil) // ignore delete error


I'll trust you as a reader, but you really think it needs a comment?

enj · 2017-06-10T08:54:35Z

Comments addressed.

[merge]

This change adds the `oadm migrate authorization` command: A controller is used to keep Openshift authorization objects and Kubernetes RBAC in sync. This command checks for parity between those objects across all namespaces and reports all objects that are out of sync. These objects require manual intervention to sync as the controller handles all cases where automatic sync is possible. The following resource types are checked by this command: * clusterrole * role * clusterrolebinding * rolebinding No resources are mutated. Signed-off-by: Monis Khan <[email protected]>

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

This change adds functions that handle all normalization, conversion and comparison for the authorization objects. These are now shared between authorizationsync and `oadm migrate authorization` to prevent any logic drift. Signed-off-by: Monis Khan <[email protected]>

openshift-bot · 2017-06-10T15:45:33Z

Evaluated for origin test up to 46af0f1

openshift-bot · 2017-06-10T17:45:31Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2096/) (Base Commit: a659cf7)

enj · 2017-06-10T18:34:31Z

Flake #14496

openshift-bot · 2017-06-12T07:21:23Z

Evaluated for origin merge up to 46af0f1

openshift-bot · 2017-06-12T07:21:34Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/969/) (Base Commit: aed6393) (Image: devenv-rhel7_6338)

enj requested review from fabianofranz, liggitt, smarterclayton and deads2k May 31, 2017 20:17

enj force-pushed the enj/i/parity_rbac/14139 branch from 7a5caa0 to b25b87f Compare May 31, 2017 23:55

deads2k reviewed Jun 2, 2017

View reviewed changes

enj force-pushed the enj/i/parity_rbac/14139 branch from b25b87f to e7c982e Compare June 6, 2017 21:13

enj force-pushed the enj/i/parity_rbac/14139 branch from e7c982e to a2cd085 Compare June 6, 2017 21:29

deads2k reviewed Jun 7, 2017

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 8, 2017

enj force-pushed the enj/i/parity_rbac/14139 branch from a2cd085 to 3e7035f Compare June 8, 2017 18:55

enj removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 8, 2017

enj force-pushed the enj/i/parity_rbac/14139 branch from 3e7035f to 057ec4d Compare June 8, 2017 21:58

deads2k reviewed Jun 9, 2017

View reviewed changes

enj force-pushed the enj/i/parity_rbac/14139 branch from 057ec4d to 171b08c Compare June 10, 2017 08:53

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2017

enj added 2 commits June 10, 2017 11:37

Generate: oadm migrate authorization

684d4b4

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

enj force-pushed the enj/i/parity_rbac/14139 branch from 171b08c to 46af0f1 Compare June 10, 2017 15:38

enj removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2017

openshift-bot merged commit c09c601 into openshift:master Jun 12, 2017

enj mentioned this pull request Jun 12, 2017

need oadm migrate tool to check for parity between origin authorization and rbac #14139

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parity check for Openshift policy and Kube RBAC #14429

Add parity check for Openshift policy and Kube RBAC #14429

enj commented May 31, 2017 •

edited

Loading

deads2k Jun 2, 2017

enj Jun 6, 2017

deads2k Jun 2, 2017

enj Jun 6, 2017 •

edited

Loading

deads2k Jun 2, 2017

enj Jun 6, 2017

deads2k Jun 2, 2017

enj Jun 6, 2017

enj commented Jun 6, 2017

deads2k Jun 7, 2017

fabianofranz commented Jun 7, 2017

enj commented Jun 8, 2017

enj commented Jun 8, 2017

deads2k Jun 9, 2017

deads2k Jun 9, 2017

deads2k Jun 9, 2017

deads2k Jun 9, 2017

deads2k Jun 9, 2017

deads2k Jun 9, 2017

enj commented Jun 10, 2017

openshift-bot commented Jun 10, 2017

openshift-bot commented Jun 10, 2017

enj commented Jun 10, 2017

openshift-bot commented Jun 12, 2017

openshift-bot commented Jun 12, 2017 •

edited

Loading

Add parity check for Openshift policy and Kube RBAC #14429

Add parity check for Openshift policy and Kube RBAC #14429

Conversation

enj commented May 31, 2017 • edited Loading

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enj Jun 6, 2017 • edited Loading

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 Jun 6, 2017

Choose a reason for hiding this comment

fabianofranz commented Jun 7, 2017

enj commented Jun 8, 2017

enj commented Jun 8, 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

enj commented Jun 10, 2017

openshift-bot commented Jun 10, 2017

openshift-bot commented Jun 10, 2017

enj commented Jun 10, 2017

openshift-bot commented Jun 12, 2017

openshift-bot commented Jun 12, 2017 • edited Loading

enj commented May 31, 2017 •

edited

Loading

enj Jun 6, 2017 •

edited

Loading

openshift-bot commented Jun 12, 2017 •

edited

Loading