diff --git a/pkg/cmd/util/clientcmd/gating.go b/pkg/cmd/util/clientcmd/gating.go index 6149ed056e8a..1a349c70c6f5 100644 --- a/pkg/cmd/util/clientcmd/gating.go +++ b/pkg/cmd/util/clientcmd/gating.go @@ -8,7 +8,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/discovery" - "github.com/openshift/origin/pkg/authorization/apis/authorization" + authorization "github.com/openshift/api/authorization/v1" ) // LegacyPolicyResourceGate returns err if the server does not support the set of legacy policy objects (< 3.7) diff --git a/pkg/oc/admin/migrate/authorization/authorization.go b/pkg/oc/admin/migrate/authorization/authorization.go index 5434d048572f..c05048d41b38 100644 --- a/pkg/oc/admin/migrate/authorization/authorization.go +++ b/pkg/oc/admin/migrate/authorization/authorization.go @@ -5,9 +5,10 @@ import ( "fmt" "io" + "k8s.io/api/rbac/v1beta1" + kerrs "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - utilerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/client-go/util/flowcontrol" rbacinternalversion "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/rbac/internalversion" "k8s.io/kubernetes/pkg/kubectl/cmd/templates" kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util" @@ -39,7 +40,8 @@ var ( No resources are mutated.`) - errOutOfSync = errors.New("is not in sync with RBAC") + // errOutOfSync is retriable since it could be caused by the controller lagging behind + errOutOfSync = migrate.ErrRetriable{errors.New("is not in sync with RBAC")} ) type MigrateAuthorizationOptions struct { @@ -54,6 +56,7 @@ func NewCmdMigrateAuthorization(name, fullName string, f *clientcmd.Factory, in Out: out, ErrOut: errout, AllNamespaces: true, + Confirm: true, // force our save function to always run (it is read only) Include: []string{ "clusterroles.authorization.openshift.io", "roles.authorization.openshift.io", @@ -81,20 +84,40 @@ func (o *MigrateAuthorizationOptions) Complete(name string, f *clientcmd.Factory return fmt.Errorf("%s takes no positional arguments", name) } + o.ResourceOptions.SaveFn = o.checkParity if err := o.ResourceOptions.Complete(f, c); err != nil { return err } - kclient, err := f.ClientSet() + discovery, err := f.DiscoveryClient() if err != nil { return err } - if err := clientcmd.LegacyPolicyResourceGate(kclient.Discovery()); err != nil { + if err := clientcmd.LegacyPolicyResourceGate(discovery); err != nil { return err } - o.rbac = kclient.Rbac() + config, err := f.ClientConfig() + if err != nil { + return err + } + + // do not rate limit this client because it has to scan all RBAC data across the cluster + // this is safe because only a cluster admin will have the ability to read these objects + configShallowCopy := *config + configShallowCopy.RateLimiter = flowcontrol.NewFakeAlwaysRateLimiter() + + // This command is only compatible with a 3.6 server, which only supported RBAC v1beta1 + // Thus we must force that GV otherwise the client will default to v1 + gv := v1beta1.SchemeGroupVersion + configShallowCopy.GroupVersion = &gv + + rbac, err := rbacinternalversion.NewForConfig(&configShallowCopy) + if err != nil { + return err + } + o.rbac = rbac return nil } @@ -104,130 +127,159 @@ func (o MigrateAuthorizationOptions) Validate() error { } func (o MigrateAuthorizationOptions) Run() error { - return o.ResourceOptions.Visitor().Visit(func(info *resource.Info) (migrate.Reporter, error) { - return o.checkParity(info.Object) - }) + // we lie and say this object has changed so our save function will run + return o.ResourceOptions.Visitor().Visit(migrate.AlwaysRequiresMigration) } // checkParity confirms that Openshift authorization objects are in sync with Kubernetes RBAC // and returns an error if they are out of sync or if it encounters a conversion error -func (o *MigrateAuthorizationOptions) checkParity(obj runtime.Object) (migrate.Reporter, error) { - var errlist []error - switch t := obj.(type) { +func (o *MigrateAuthorizationOptions) checkParity(info *resource.Info, _ migrate.Reporter) error { + var err migrate.TemporaryError + + switch t := info.Object.(type) { case *authorizationapi.ClusterRole: - errlist = append(errlist, o.checkClusterRole(t)...) + err = o.checkClusterRole(t) case *authorizationapi.Role: - errlist = append(errlist, o.checkRole(t)...) + err = o.checkRole(t) case *authorizationapi.ClusterRoleBinding: - errlist = append(errlist, o.checkClusterRoleBinding(t)...) + err = o.checkClusterRoleBinding(t) case *authorizationapi.RoleBinding: - errlist = append(errlist, o.checkRoleBinding(t)...) + err = o.checkRoleBinding(t) default: - return nil, nil // indicate that we ignored the object + // this should never happen unless o.Include or the server is broken + return fmt.Errorf("impossible type %T for checkParity info=%#v object=%#v", t, info, t) + } + + // We encountered no error, so this object is in sync. + if err == nil { + // we only perform read operations so we return this error to signal that we did not change anything + return migrate.ErrUnchanged + } + + // At this point we know that we have some non-nil TemporaryError. + // If it has the possibility of being transient, we need to sync ourselves with the current state of the object. + if err.Temporary() { + // The most likely cause is that an authorization object was deleted after we initially fetched it, + // and the controller deleted the associated RBAC object, which caused our RBAC GET to fail. + // We can confirm this by refetching the authorization object. + refreshErr := info.Get() + if refreshErr != nil { + // Our refresh GET for this authorization object failed. + // The default logic for migration errors is appropriate in this case (refreshErr is most likely a NotFound). + return migrate.DefaultRetriable(info, refreshErr) + } + // We had no refreshErr, but encountered some other possibly transient error. + // No special handling is required in this case, we just pass it through below. } - return migrate.NotChanged, utilerrors.NewAggregate(errlist) // we only perform read operations + + // All of the check* funcs return an appropriate TemporaryError based on the failure, + // so we can pass that through to the default migration logic which will retry as needed. + return err } -func (o *MigrateAuthorizationOptions) checkClusterRole(originClusterRole *authorizationapi.ClusterRole) []error { - var errlist []error +// handleRBACGetError signals for a retry on NotFound (handles deletion and sync lag) +// and ServerTimeout (handles heavy load against the server). +func handleRBACGetError(err error) migrate.TemporaryError { + switch { + case kerrs.IsNotFound(err), kerrs.IsServerTimeout(err): + return migrate.ErrRetriable{err} + default: + return migrate.ErrNotRetriable{err} + } +} +func (o *MigrateAuthorizationOptions) checkClusterRole(originClusterRole *authorizationapi.ClusterRole) migrate.TemporaryError { // convert the origin role to a rbac role convertedClusterRole, err := util.ConvertToRBACClusterRole(originClusterRole) if err != nil { - errlist = append(errlist, err) + // conversion errors should basically never happen, so we do not attempt to retry on those + return migrate.ErrNotRetriable{err} } // try to get the equivalent rbac role from the api rbacClusterRole, err := o.rbac.ClusterRoles().Get(originClusterRole.Name, v1.GetOptions{}) if err != nil { - errlist = append(errlist, err) + // it is possible that the controller has not synced this yet + return handleRBACGetError(err) } - // compare the results if there have been no errors so far - if len(errlist) == 0 { - // if they are not equal, something has gone wrong and the two objects are not in sync - if util.PrepareForUpdateClusterRole(convertedClusterRole, rbacClusterRole) { - errlist = append(errlist, errOutOfSync) - } + // if they are not equal, something has gone wrong and the two objects are not in sync + if util.PrepareForUpdateClusterRole(convertedClusterRole, rbacClusterRole) { + // we retry on this since it could be caused by the controller lagging behind + return errOutOfSync } - return errlist + return nil } -func (o *MigrateAuthorizationOptions) checkRole(originRole *authorizationapi.Role) []error { - var errlist []error - +func (o *MigrateAuthorizationOptions) checkRole(originRole *authorizationapi.Role) migrate.TemporaryError { // convert the origin role to a rbac role convertedRole, err := util.ConvertToRBACRole(originRole) if err != nil { - errlist = append(errlist, err) + // conversion errors should basically never happen, so we do not attempt to retry on those + return migrate.ErrNotRetriable{err} } // try to get the equivalent rbac role from the api rbacRole, err := o.rbac.Roles(originRole.Namespace).Get(originRole.Name, v1.GetOptions{}) if err != nil { - errlist = append(errlist, err) + // it is possible that the controller has not synced this yet + return handleRBACGetError(err) } - // compare the results if there have been no errors so far - if len(errlist) == 0 { - // if they are not equal, something has gone wrong and the two objects are not in sync - if util.PrepareForUpdateRole(convertedRole, rbacRole) { - errlist = append(errlist, errOutOfSync) - } + // if they are not equal, something has gone wrong and the two objects are not in sync + if util.PrepareForUpdateRole(convertedRole, rbacRole) { + // we retry on this since it could be caused by the controller lagging behind + return errOutOfSync } - return errlist + return nil } -func (o *MigrateAuthorizationOptions) checkClusterRoleBinding(originRoleBinding *authorizationapi.ClusterRoleBinding) []error { - var errlist []error - +func (o *MigrateAuthorizationOptions) checkClusterRoleBinding(originRoleBinding *authorizationapi.ClusterRoleBinding) migrate.TemporaryError { // convert the origin role binding to a rbac role binding convertedRoleBinding, err := util.ConvertToRBACClusterRoleBinding(originRoleBinding) if err != nil { - errlist = append(errlist, err) + // conversion errors should basically never happen, so we do not attempt to retry on those + return migrate.ErrNotRetriable{err} } // try to get the equivalent rbac role binding from the api rbacRoleBinding, err := o.rbac.ClusterRoleBindings().Get(originRoleBinding.Name, v1.GetOptions{}) if err != nil { - errlist = append(errlist, err) + // it is possible that the controller has not synced this yet + return handleRBACGetError(err) } - // compare the results if there have been no errors so far - if len(errlist) == 0 { - // if they are not equal, something has gone wrong and the two objects are not in sync - if util.PrepareForUpdateClusterRoleBinding(convertedRoleBinding, rbacRoleBinding) { - errlist = append(errlist, errOutOfSync) - } + // if they are not equal, something has gone wrong and the two objects are not in sync + if util.PrepareForUpdateClusterRoleBinding(convertedRoleBinding, rbacRoleBinding) { + // we retry on this since it could be caused by the controller lagging behind + return errOutOfSync } - return errlist + return nil } -func (o *MigrateAuthorizationOptions) checkRoleBinding(originRoleBinding *authorizationapi.RoleBinding) []error { - var errlist []error - +func (o *MigrateAuthorizationOptions) checkRoleBinding(originRoleBinding *authorizationapi.RoleBinding) migrate.TemporaryError { // convert the origin role binding to a rbac role binding convertedRoleBinding, err := util.ConvertToRBACRoleBinding(originRoleBinding) if err != nil { - errlist = append(errlist, err) + // conversion errors should basically never happen, so we do not attempt to retry on those + return migrate.ErrNotRetriable{err} } // try to get the equivalent rbac role binding from the api rbacRoleBinding, err := o.rbac.RoleBindings(originRoleBinding.Namespace).Get(originRoleBinding.Name, v1.GetOptions{}) if err != nil { - errlist = append(errlist, err) + // it is possible that the controller has not synced this yet + return handleRBACGetError(err) } - // compare the results if there have been no errors so far - if len(errlist) == 0 { - // if they are not equal, something has gone wrong and the two objects are not in sync - if util.PrepareForUpdateRoleBinding(convertedRoleBinding, rbacRoleBinding) { - errlist = append(errlist, errOutOfSync) - } + // if they are not equal, something has gone wrong and the two objects are not in sync + if util.PrepareForUpdateRoleBinding(convertedRoleBinding, rbacRoleBinding) { + // we retry on this since it could be caused by the controller lagging behind + return errOutOfSync } - return errlist + return nil } diff --git a/pkg/oc/admin/migrate/migrator.go b/pkg/oc/admin/migrate/migrator.go index 9c03567086ce..20f24d10215a 100644 --- a/pkg/oc/admin/migrate/migrator.go +++ b/pkg/oc/admin/migrate/migrator.go @@ -55,9 +55,6 @@ func timeStampNow() string { return time.Now().Format("0102 15:04:05.000000") } -// NotChanged is a Reporter returned by operations that are guaranteed to be read-only -var NotChanged = ReporterBool(false) - // ResourceOptions assists in performing migrations on any object that // can be retrieved via the API. type ResourceOptions struct { @@ -351,10 +348,13 @@ var ErrUnchanged = fmt.Errorf("migration was not necessary") // both status and spec must be changed). var ErrRecalculate = fmt.Errorf("recalculate migration") +// MigrateError is an exported alias to error to allow external packages to use ErrRetriable and ErrNotRetriable +type MigrateError error + // ErrRetriable is a wrapper for an error that a migrator may use to indicate the // specific error can be retried. type ErrRetriable struct { - error + MigrateError } func (ErrRetriable) Temporary() bool { return true } @@ -362,12 +362,14 @@ func (ErrRetriable) Temporary() bool { return true } // ErrNotRetriable is a wrapper for an error that a migrator may use to indicate the // specific error cannot be retried. type ErrNotRetriable struct { - error + MigrateError } func (ErrNotRetriable) Temporary() bool { return false } -type temporary interface { +// TemporaryError is a wrapper interface that is used to determine if an error can be retried. +type TemporaryError interface { + error // Temporary should return true if this is a temporary error Temporary() bool } @@ -479,7 +481,7 @@ func (t *migrateTracker) try(info *resource.Info, retries int) (attemptResult, e // canRetry returns true if the provided error indicates a retry is possible. func canRetry(err error) bool { - if temp, ok := err.(temporary); ok && temp.Temporary() { + if temp, ok := err.(TemporaryError); ok && temp.Temporary() { return true } return err == ErrRecalculate diff --git a/pkg/oc/cli/cmd/create/policy_binding.go b/pkg/oc/cli/cmd/create/policy_binding.go index 95bfed228b47..163e4d4b10ab 100644 --- a/pkg/oc/cli/cmd/create/policy_binding.go +++ b/pkg/oc/cli/cmd/create/policy_binding.go @@ -72,12 +72,12 @@ func (o *CreatePolicyBindingOptions) Complete(cmd *cobra.Command, f *clientcmd.F } o.BindingNamespace = namespace - kclient, err := f.ClientSet() + discovery, err := f.DiscoveryClient() if err != nil { return err } - if err := clientcmd.LegacyPolicyResourceGate(kclient.Discovery()); err != nil { + if err := clientcmd.LegacyPolicyResourceGate(discovery); err != nil { return err } client, err := f.OpenshiftInternalAuthorizationClient()