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.

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

Make bootstrap policy setup more robust #10099

Merged
merged 5 commits into from
Aug 2, 2016
Merged

Make bootstrap policy setup more robust #10099

merged 5 commits into from
Aug 2, 2016

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Jul 29, 2016 •
edited
Loading

Makes the virtual storage for role/rolebinding act more like real objects:

  • Tolerate AlreadyExists errors when lazily creating empty Policy/PolicyBinding objects
  • Allow unconditional updates of roles/rolebindings (empty ResourceVersion)
  • Detect resourceVersion conflicts on updates on roles/rolebindings
  • Retry on underlying policy/policyBinding conflicts
  • Avoid mutating the original object passed into Create calls

Makes the reconcile and bootstrap operations aggregate errors and continue

Makes the AddRole() role-modification operation tolerate a race where the computed rolebinding name already exists when the operation tries to create it

Makes the bootstrap policy overwrite avoid unnecessary deletes/recreates:

  1. Attempt create
  2. If an AlreadyExists error is returned, attempt an unconditional update
  3. If any error is returned from the update, fall back to delete/recreate (current behavior)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1359900
Fixes #8781

@liggitt liggitt mentioned this pull request Jul 29, 2016
Merged
@liggitt
Copy link
Contributor Author

liggitt commented Jul 29, 2016

@deads2k PTAL

@liggitt
Copy link
Contributor Author

liggitt commented Jul 29, 2016

[test]

@liggitt
Copy link
Contributor Author

liggitt commented Jul 29, 2016 •
edited
Loading

yum failure on integration (#8571), re[test]

deads2k
deads2k reviewed Jul 29, 2016
View reviewed changes
pkg/cmd/server/admin/overwrite_bootstrappolicy.go
for _, item := range template.Objects {
switch t := item.(type) {
case *authorizationapi.Role:
ctx := kapi.WithNamespace(kapi.NewContext(), t.Namespace)
if change {
roleStorage.Delete(ctx, t.Name, nil)
if _, err := roleStorage.CreateRoleWithEscalation(ctx, t); err != nil {
if err := kclient.RetryOnConflict(tryTryAgainBackoff, func() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking on it, is there a reason we don't retry server-side? Seems like it would better meet expectations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in create cases, you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is weird to retryonconflict from a create call

Copy link
Contributor

Choose a reason for hiding this comment

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

it is weird to retryonconflict from a create call

No, I mean in pkg/authorization/registry/role/policybased/virtual_storage.go, detecting a conflict during the policy update and retrying the role update/create/delete in that code.

@liggitt
Copy link
Contributor Author

liggitt commented Jul 31, 2016

as a test, ran 10 copies of all the ensure functions on an empty etcd simultaneously. no errors reported, all roles, rolebindings, infra sa roles and bindings created successfully.

deads2k
deads2k reviewed Aug 1, 2016
View reviewed changes
pkg/authorization/registry/role/policybased/virtual_storage.go
}
if err := kclient.RetryOnConflict(kclient.DefaultRetry, func() error {
policy, err := m.PolicyStorage.GetPolicy(ctx, authorizationapi.PolicyName)
if err != nil && kapierrors.IsNotFound(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't need the nil check.

@liggitt
Copy link
Contributor Author

liggitt commented Aug 1, 2016

comments addressed, will squash down after review

@deads2k
Copy link
Contributor

deads2k commented Aug 2, 2016

I was expecting a validation update to ensure that role refs didn't change

@deads2k
Copy link
Contributor

deads2k commented Aug 2, 2016

lgtm other than validation.

@liggitt
Copy link
Contributor Author

liggitt commented Aug 2, 2016

Already existed:

func ValidateRoleBindingUpdate(roleBinding *authorizationapi.RoleBinding, oldRoleBinding *authorizationapi.RoleBinding, isNamespaced bool) field.ErrorList {
    allErrs := ValidateRoleBinding(roleBinding, isNamespaced)
    allErrs = append(allErrs, validation.ValidateObjectMetaUpdate(&roleBinding.ObjectMeta, &oldRoleBinding.ObjectMeta, field.NewPath("metadata"))...)

    if oldRoleBinding.RoleRef != roleBinding.RoleRef {
        allErrs = append(allErrs, field.Invalid(field.NewPath("roleRef"), roleBinding.RoleRef, "cannot change roleRef"))
    }

    return allErrs
}

liggitt added 5 commits August 2, 2016 10:48
@liggitt
Fix panic in rolebinding reconcile error message
872fc72
@liggitt
Improve role/rolebinding virtual storage
e5dd746 
Retry role/rolebinding creates/updates/deletes on policy/policybinding conflicts
Allow unconditional updates of roles/rolebindings
Detect resourceVersion conflicts of roles/rolebindings
Don't return AlreadyExists errors from lazy policy/policybinding creates
Remove unneeded nil checks
Run BeforeUpdate on actual object being updated
Remove unnecessary referential checks
Avoid mutating original object in Create calls
@liggitt
Make AddRole tolerate races on role additions
d4f3eee
@liggitt
Aggregate reconcile errors
842a8df
@liggitt
Improve bootstrap policy overwrite error handling
b5caf24 
Aggregate errors but continue to other objects
Avoid unnecessary delete/recreates
@liggitt
Copy link
Contributor Author

liggitt commented Aug 2, 2016

[merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 2, 2016 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7389/) (Image: devenv-rhel7_4730)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to b5caf24

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to b5caf24

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7389/)

@openshift-bot openshift-bot merged commit 3ebb112 into openshift:master Aug 2, 2016
@liggitt liggitt deleted the bootstrap-policy-fix branch August 3, 2016 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@liggitt @deads2k @openshift-bot @mfojtik