Skip to content

Commit

Permalink
added ManagementActionChecker to simplify management policy checks
Browse files Browse the repository at this point in the history 
Signed-off-by: lsviben <[email protected]>
  • Loading branch information
@lsviben
lsviben committed Jul 11, 2023
1 parent bb38615 commit efad1c1
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 164 deletions.
89 changes: 49 additions & 40 deletions pkg/reconciler/managed/reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,50 @@ func ControllerName(kind string) string {
return "managed/" + strings.ToLower(kind)
}

// ManagementActionChecker is used to perform checks on management actions
// to determine if they are allowed, or if they are the only allowed action.
type ManagementActionChecker interface {
// ActionAllowed returns true if the supplied action is allowed.
ActionAllowed(action xpv1.ManagementAction) bool
// OnlyAction returns true if the supplied action is the only allowed action.
OnlyAction(action xpv1.ManagementAction) bool
}

// ManagementPolicyActionChecker is used to perform checks on management actions
// based on the management policy and if the management policy feature is enabled.
type ManagementPolicyActionChecker struct {
managementPolicyEnabled bool
managementPolicy xpv1.ManagementPolicy
}

// NewManagementPolicyActionChecker returns an ActionChecker based on the
// management policy and if the management policy feature is enabled.
func NewManagementPolicyActionChecker(managementPolicyEnabled bool, managementPolicy xpv1.ManagementPolicy) *ManagementPolicyActionChecker {
return &ManagementPolicyActionChecker{
managementPolicyEnabled: managementPolicyEnabled,
managementPolicy: managementPolicy,
}
}

// ActionAllowed returns true if the supplied action is contained in the
// management policy. Returns true if the management policy feature is disabled.
func (m *ManagementPolicyActionChecker) ActionAllowed(action xpv1.ManagementAction) bool {
if !m.managementPolicyEnabled {
return true
}
return ContainsManagementAction(m.managementPolicy, action)
}

// OnlyAction returns true if the supplied action is the only action contained
// in the management policy. Returns false if the management policy feature is
// disabled.
func (m *ManagementPolicyActionChecker) OnlyAction(action xpv1.ManagementAction) bool {
if !m.managementPolicyEnabled {
return false
}
return ContainsOnlyManagementAction(m.managementPolicy, action)
}

// A CriticalAnnotationUpdater is used when it is critical that annotations must
// be updated before returning from the Reconcile loop.
type CriticalAnnotationUpdater interface {
Expand Down Expand Up @@ -724,6 +768,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
if managementPoliciesEnabled {
log.WithValues("managementPolicies", managed.GetManagementPolicy())
}
actionChecker := NewManagementPolicyActionChecker(managementPoliciesEnabled, managed.GetManagementPolicy())

// Check the pause annotation and return if it has the value "true"
// or if the management policies are enabled and the ManagementPolicy is empty.
Expand Down Expand Up @@ -876,7 +921,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco

// In the observe-only mode, !observation.ResourceExists will be an error
// case, and we will explicitly return this information to the user.
if !observation.ResourceExists && isManagementPolicyObserveOnly(managementPoliciesEnabled, managed.GetManagementPolicy()) {
if !observation.ResourceExists && actionChecker.OnlyAction(xpv1.ManagementActionObserve) {
record.Event(managed, event.Warning(reasonCannotObserve, errors.New(errExternalResourceNotExist)))
managed.SetConditions(xpv1.ReconcileError(errors.Wrap(errors.New(errExternalResourceNotExist), errReconcileObserve)))
return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
Expand Down Expand Up @@ -969,7 +1014,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
}

if !observation.ResourceExists && shouldCreate(managementPoliciesEnabled, managed.GetManagementPolicy()) {
if !observation.ResourceExists && actionChecker.ActionAllowed(xpv1.ManagementActionCreate) {
// We write this annotation for two reasons. Firstly, it helps
// us to detect the case in which we fail to persist critical
// information (like the external name) that may be set by the
Expand Down Expand Up @@ -1059,7 +1104,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
return reconcile.Result{Requeue: true}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
}

if observation.ResourceLateInitialized && shouldLateInit(managementPoliciesEnabled, managed.GetManagementPolicy()) {
if observation.ResourceLateInitialized && actionChecker.ActionAllowed(xpv1.ManagementActionLateInitialize) {
// Note that this update may reset any pending updates to the status of
// the managed resource from when it was observed above. This is because
// the API server replies to the update with its unchanged view of the
Expand Down Expand Up @@ -1092,7 +1137,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
}

// skip the update if the management policy is set to ignore updates
if !shouldUpdate(managementPoliciesEnabled, managed.GetManagementPolicy()) {
if !actionChecker.ActionAllowed(xpv1.ManagementActionUpdate) {
log.Debug("Reconciliation succeeded", "requeue-after", time.Now().Add(r.pollInterval))
managed.SetConditions(xpv1.ReconcileSuccess())
return reconcile.Result{RequeueAfter: r.pollInterval}, errors.Wrap(r.client.Status().Update(ctx, managed), errUpdateManagedStatus)
Expand Down Expand Up @@ -1165,42 +1210,6 @@ func ContainsOnlyManagementAction(managementPolicy xpv1.ManagementPolicy, action
return len(managementPolicy) == 1 && managementPolicy[0] == action
}

// shouldUpdate returns true if the management policy is enabled, and
// contains the Update action.
func shouldUpdate(managementPolicyEnabled bool, policy xpv1.ManagementPolicy) bool {
return shouldDoAction(managementPolicyEnabled, policy, xpv1.ManagementActionUpdate)
}

// shouldLateInit returns true if the management policy is enabled, and
// contains the LateInitialize action.
func shouldLateInit(managementPolicyEnabled bool, policy xpv1.ManagementPolicy) bool {
return shouldDoAction(managementPolicyEnabled, policy, xpv1.ManagementActionLateInitialize)
}

// shouldCreate returns true if the management policy is enabled, and
// contains the Create action.
func shouldCreate(managementPolicyEnabled bool, policy xpv1.ManagementPolicy) bool {
return shouldDoAction(managementPolicyEnabled, policy, xpv1.ManagementActionCreate)
}

// shouldDoAction returns true if the management policy is enabled, and
// contains the supplied action.
func shouldDoAction(managementPolicyEnabled bool, policy xpv1.ManagementPolicy, action xpv1.ManagementAction) bool {
if !managementPolicyEnabled {
return true
}
return ContainsManagementAction(policy, action)
}

// isManagementPolicyObserveOnly returns true if the management policy is
// enabled, and contains only the Observe action.
func isManagementPolicyObserveOnly(managementPolicyEnabled bool, policy xpv1.ManagementPolicy) bool {
if !managementPolicyEnabled {
return false
}
return ContainsOnlyManagementAction(policy, xpv1.ManagementActionObserve)
}

// isNonDefaultManagementPolicy returns true if the management policy is
// not enabled, and set to a non-default value.
func isNonDefaultManagementPolicy(managementPolicyEnabled bool, policy xpv1.ManagementPolicy) bool {
Expand Down
143 changes: 19 additions & 124 deletions pkg/reconciler/managed/reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1917,10 +1917,11 @@ func TestIsPaused(t *testing.T) {
}
}

func TestShouldLateInit(t *testing.T) {
func TestPolicyCheckerActionAllowed(t *testing.T) {
type args struct {
managementPoliciesEnabled bool
policy xpv1.ManagementPolicy
action xpv1.ManagementAction
}
cases := map[string]struct {
args args
Expand All @@ -1930,135 +1931,46 @@ func TestShouldLateInit(t *testing.T) {
args: args{
managementPoliciesEnabled: false,
policy: xpv1.ManagementPolicy{xpv1.ManagementActionLateInitialize},
action: xpv1.ManagementActionUpdate,
},
want: true,
},
"ManagementPoliciesEnabledActionLateInitialize": {
"ManagementPoliciesEnabledActionAllowed": {
args: args{
managementPoliciesEnabled: true,
policy: xpv1.ManagementPolicy{xpv1.ManagementActionLateInitialize},
action: xpv1.ManagementActionLateInitialize,
},
want: true,
},
"ManagementPoliciesEnabledActionAll": {
args: args{
managementPoliciesEnabled: true,
policy: xpv1.ManagementPolicy{xpv1.ManagementActionAll},
action: xpv1.ManagementActionLateInitialize,
},
want: true,
},
"ManagementPoliciesEnabledNoUpdate": {
args: args{
managementPoliciesEnabled: true,
policy: xpv1.ManagementPolicy{xpv1.ManagementActionObserve},
},
want: false,
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
if diff := cmp.Diff(tc.want, shouldLateInit(tc.args.managementPoliciesEnabled, tc.args.policy)); diff != "" {
t.Errorf("shouldLateInit(...): -want, +got:\n%s", diff)
}
})
}
}

func TestShouldUpdate(t *testing.T) {
type args struct {
managementPoliciesEnabled bool
policy xpv1.ManagementPolicy
}
cases := map[string]struct {
args args
want bool
}{
"ManagementPoliciesDisabled": {
args: args{
managementPoliciesEnabled: false,
policy: xpv1.ManagementPolicy{xpv1.ManagementActionUpdate},
},
want: true,
},
"ManagementPoliciesEnabledActionUpdate": {
args: args{
managementPoliciesEnabled: true,
policy: xpv1.ManagementPolicy{xpv1.ManagementActionUpdate},
},
want: true,
},
"ManagementPoliciesEnabledActionAll": {
args: args{
managementPoliciesEnabled: true,
policy: xpv1.ManagementPolicy{xpv1.ManagementActionAll},
},
want: true,
},
"ManagementPoliciesEnabledNoUpdate": {
args: args{
managementPoliciesEnabled: true,
policy: xpv1.ManagementPolicy{xpv1.ManagementActionObserve},
},
want: false,
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
if diff := cmp.Diff(tc.want, shouldUpdate(tc.args.managementPoliciesEnabled, tc.args.policy)); diff != "" {
t.Errorf("shouldUpdate(...): -want, +got:\n%s", diff)
}
})
}
}

func TestShouldCreate(t *testing.T) {
type args struct {
managementPoliciesEnabled bool
policy xpv1.ManagementPolicy
}
cases := map[string]struct {
args args
want bool
}{
"ManagementPoliciesDisabled": {
args: args{
managementPoliciesEnabled: false,
policy: xpv1.ManagementPolicy{xpv1.ManagementActionCreate},
},
want: true,
},
"ManagementPoliciesEnabledActionCreate": {
args: args{
managementPoliciesEnabled: true,
policy: xpv1.ManagementPolicy{xpv1.ManagementActionCreate},
},
want: true,
},
"ManagementPoliciesEnabledActionAll": {
args: args{
managementPoliciesEnabled: true,
policy: xpv1.ManagementPolicy{xpv1.ManagementActionAll},
},
want: true,
},
"ManagementPoliciesEnabledNoCreate": {
"ManagementPoliciesEnabledNotAllowed": {
args: args{
managementPoliciesEnabled: true,
policy: xpv1.ManagementPolicy{xpv1.ManagementActionObserve},
action: xpv1.ManagementActionLateInitialize,
},
want: false,
},
}
for name, tc := range cases {
t.Run(name, func(t *testing.T) {
if diff := cmp.Diff(tc.want, shouldCreate(tc.args.managementPoliciesEnabled, tc.args.policy)); diff != "" {
t.Errorf("shouldCreate(...): -want, +got:\n%s", diff)
actionChecker := NewManagementPolicyActionChecker(tc.args.managementPoliciesEnabled, tc.args.policy)
if diff := cmp.Diff(tc.want, actionChecker.ActionAllowed(tc.args.action)); diff != "" {
t.Errorf("ActionAllowed(...): -want, +got:\n%s", diff)
}
})
}
}

func TestShouldDoAction(t *testing.T) {
func TestPolicyCheckerOnlyAction(t *testing.T) {
type args struct {
managementPoliciesEnabled bool
policy xpv1.ManagementPolicy
Expand All @@ -2074,47 +1986,30 @@ func TestShouldDoAction(t *testing.T) {
policy: xpv1.ManagementPolicy{xpv1.ManagementActionObserve},
action: xpv1.ManagementActionObserve,
},
want: true,
want: false,
},
"ManagementPoliciesEnabled": {
"ManagementPoliciesEnabledOnlyAction": {
args: args{
managementPoliciesEnabled: true,
policy: xpv1.ManagementPolicy{xpv1.ManagementActionObserve},
action: xpv1.ManagementActionObserve,
},
want: true,
},
"ManagementPoliciesEnabledActionAll": {
"ManagementPoliciesEnabledNotOnlyAction": {
args: args{
managementPoliciesEnabled: true,
policy: xpv1.ManagementPolicy{xpv1.ManagementActionAll},
policy: xpv1.ManagementPolicy{xpv1.ManagementActionObserve, xpv1.ManagementActionUpdate},
action: xpv1.ManagementActionObserve,
},
want: true,
},
"ManagementPoliciesEnabledNoAction": {
args: args{
managementPoliciesEnabled: true,
policy: xpv1.ManagementPolicy{xpv1.ManagementActionObserve},
action: xpv1.ManagementActionCreate,
},
want: false,
},
"ManagementPoliciesEnabledNoManagementPolicy": {
args: args{
managementPoliciesEnabled: true,
policy: xpv1.ManagementPolicy{},
action: xpv1.ManagementActionCreate,
},
want: false,
},
}

for name, tc := range cases {
t.Run(name, func(t *testing.T) {
got := shouldDoAction(tc.args.managementPoliciesEnabled, tc.args.policy, tc.args.action)
if diff := cmp.Diff(tc.want, got); diff != "" {
t.Errorf("shouldDoAction(...): -want, +got:\n%s", diff)
actionChecker := NewManagementPolicyActionChecker(tc.args.managementPoliciesEnabled, tc.args.policy)
if diff := cmp.Diff(tc.want, actionChecker.OnlyAction(tc.args.action)); diff != "" {
t.Errorf("OnlyAction(...): -want, +got:\n%s", diff)
}
})
}
Expand Down

0 comments on commit efad1c1

Please sign in to comment.