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.

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

Policy version handling #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jorgeperis
Copy link
Contributor

@jorgeperis jorgeperis commented Sep 28, 2023

Old PR => #34

Current IAM operator version does not support multiple history versions of the policies. This produces an error in the operator not allowing policies to be updated.

With this PR we are changing two behaviors:

  • When trying to update a AWS policy if the current policy version count is equal to AWS limit of five, we remove the oldest version and then continue.
  • Update the observerGeneration of the resource in the successStatusUpdater itself

// +kubebuilder:rbac:groups=aws-iam.redradrat.xyz,resources=policies,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=aws-iam.redradrat.xyz,resources=policies/status,verbs=get;update;patch
// +kubebuilder:rbac:groups=aws-iam.redradrat.xyz,resources=policies/finalizers,verbs=get;update
// +kubebuilder:rbac:groups=aws-iam.redradrat.xyz,resources=policyattachments,verbs=get;list;watch
Copy link
Owner

Choose a reason for hiding this comment

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

I think this shouldn't change. policies/finalizer should still be good?

@@ -125,35 +132,46 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr

// RECONCILE THE RESOURCE

// if there is already an ARN in our status, then we update the object
// We try to create the resource
statusWriter, err := CreateAWSObject(iamsvc, ins, DoNothingPreFunc)
statusWriter(ctx, ins, &policy, r.Status(), log)
if err != nil {
Copy link
Owner

Choose a reason for hiding this comment

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

We can simplify this, by turning the condition around. If err == nil, then we log creation success and return. Then we don't have to have the whole error handling inside another if. Can be cleaner?

Copy link
Owner

Choose a reason for hiding this comment

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

this refers to the error handler from 131 onwards... not sure why my comment here ended up being weird

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants