Skip to content

Fix issues with oc adm migrate authorization #18005

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 20, 2018
Merged

Fix issues with oc adm migrate authorization #18005

merged 1 commit into from
Jan 20, 2018

Conversation

enj
Copy link
Contributor

@enj enj commented Jan 5, 2018

This change handles a number of bugs with the migrate authorization command:

  1. Rework MigrateAuthorizationOptions.checkParity to be a MigrateActionFunc instead of a MigrateVisitFunc. This allows us to take advantage of migrate's default retry handling.
  2. Add proper retry logic to checkParity, and have all check* funcs return the appropriate TemporaryError based on the situation. This coupled with migrate's existing retry logic makes the command resilient against common errors such as the deletion of resources.
  3. Remove the binding of the standard migrate flags from migrate authorization. This command supports no parameters, and exposing the standard migrate parameters allows the user to accidentally break how the command runs.
  4. Fix GroupVersion constants used for discovery based gating. They were incorrectly set to the internal version instead of v1. This would cause the policy based gating to always think that the server did not support policy objects.
  5. Force RBAC client to use v1beta1 since that is the only version supported by a 3.6 server. This allows you to use a 3.9 client against a 3.6 server.
  6. Remove rate limiting from the RBAC client to fix BZ 1513139. Only a cluster admin can interact with RBAC resources on a 3.6 server, so this will quickly error out if run by a non-privileged user.

Bug 1513139

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

/kind bug
/assign @simo5 @smarterclayton @deads2k @mrogers950

cc @sdodson @jupierce

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 5, 2018
@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 5, 2018
@enj
Copy link
Contributor Author

enj commented Jan 5, 2018

/cherrypick release-3.8

@openshift-cherrypick-robot
Copy link

@enj: once the present PR merges, I will cherry-pick it on top of release-3.8 in a new PR and assign it to you.

In response to this:

/cherrypick release-3.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@enj
Copy link
Contributor Author

enj commented Jan 5, 2018

/cherrypick release-3.7

@openshift-cherrypick-robot
Copy link

@enj: once the present PR merges, I will cherry-pick it on top of release-3.7 in a new PR and assign it to you.

In response to this:

/cherrypick release-3.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@enj
Copy link
Contributor Author

enj commented Jan 5, 2018

/cherrypick release-3.6

@openshift-cherrypick-robot
Copy link

@enj: once the present PR merges, I will cherry-pick it on top of release-3.6 in a new PR and assign it to you.

In response to this:

/cherrypick release-3.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@simo5
Copy link
Contributor

simo5 commented Jan 5, 2018

/retest

@enj
Copy link
Contributor Author

enj commented Jan 17, 2018

@smarterclayton @deads2k this needs review.

smarterclayton
smarterclayton reviewed Jan 19, 2018
View reviewed changes
pkg/oc/admin/migrate/migrator.go Outdated
}

func (ErrNotRetriable) Temporary() bool { return false }

type temporary interface {
type TemporaryError interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add godoc

@smarterclayton
Copy link
Contributor

LGTM other than the one minor thing, feel free to self tag

@enj
Fix issues with oc adm migrate authorization
679add3 
This change handles a number of bugs with the migrate authorization
command:

1. Rework MigrateAuthorizationOptions.checkParity to be a
MigrateActionFunc instead of a MigrateVisitFunc.  This allows us to
take advantage of migrate's default retry handling.
2. Add proper retry logic to checkParity, and have all check* funcs
return the appropriate TemporaryError based on the situation.  This
coupled with migrate's existing retry logic makes the command
resilient against common errors such as the deletion of resources.
3. Remove the binding of the standard migrate flags from migrate
authorization.  This command supports no parameters, and exposing
the standard migrate parameters allows the user to accidentally
break how the command runs.
4. Fix GroupVersion constants used for discovery based gating.  They
were incorrectly set to the internal version instead of v1.  This
would cause the policy based gating to always think that the server
did not support policy objects.
5. Force RBAC client to use v1beta1 since that is the only version
supported by a 3.6 server.  This allows you to use a 3.9 client
against a 3.6 server.
6. Remove rate limiting from the RBAC client to fix BZ 1513139.
Only a cluster admin can interact with RBAC resources on a 3.6
server, so this will quickly error out if run by a non-privileged
user.

Bug 1513139

Signed-off-by: Monis Khan <[email protected]>
@enj enj force-pushed the enj/i/migrate_authz_no_rate_limiting/1513139 branch from 496c518 to 679add3 Compare January 19, 2018 20:39
@enj enj added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Jan 19, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: 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:

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

@enj
Copy link
Contributor Author

enj commented Jan 19, 2018

/retest

@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@enj
Copy link
Contributor Author

enj commented Jan 20, 2018

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 18005, 18174).

@openshift-merge-robot openshift-merge-robot merged commit 61d82de into openshift:master Jan 20, 2018
@openshift-cherrypick-robot
Copy link

@enj: #18005 failed to apply on top of branch "release-3.8":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pkg/oc/admin/migrate/authorization/authorization.go
M	pkg/oc/admin/migrate/migrator.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/oc/admin/migrate/migrator.go
CONFLICT (content): Merge conflict in pkg/oc/admin/migrate/migrator.go
Auto-merging pkg/oc/admin/migrate/authorization/authorization.go
Patch failed at 0001 Fix issues with oc adm migrate authorization

In response to this:

/cherrypick release-3.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link

@enj: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_conformance_install 679add3 link /test extended_conformance_install

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@enj enj mentioned this pull request Jan 22, 2018
Merged
openshift-merge-robot added a commit that referenced this pull request Jan 24, 2018
@openshift-merge-robot
Merge pull request #18220 from enj/automated-cherry-pick-of-#18005-up…
d9ebdd2 
…stream-release-3.8

Automatic merge from submit-queue.

Automated cherry pick of #18005 on release-3.8

Cherry pick of #18005 on release-3.8.

#18005: Fix issues with oc adm migrate authorization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarterclayton smarterclayton smarterclayton left review comments

@juanvallejo juanvallejo Awaiting requested review from juanvallejo

@soltysh soltysh Awaiting requested review from soltysh

Assignees

@smarterclayton smarterclayton

@mrogers950 mrogers950

@deads2k deads2k

@simo5 simo5

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@enj @openshift-cherrypick-robot @simo5 @smarterclayton @openshift-ci-robot @openshift-merge-robot @mrogers950 @deads2k