Skip to content

Conversation

@mikhail-aws
Copy link
Contributor

@mikhail-aws mikhail-aws commented Oct 27, 2023

Note:
add IAM auth policy conflicts handling

Tested manually by creating multiple policies for same target ref.
Log example:

2023-10-27T13:19:32.780-0700	ERROR	controller.iam-auth-policy	controllers/iamauthpolicy_controller.go:52	conflict with other policies for same TargetRef, policy: test-iam-auth-policy, conflicted with: [test-iam-auth-policy2]

Policy Status updated:

Status:
  Conditions:
    Last Transition Time:  2023-10-27T20:13:30Z
    Message:
    Observed Generation:   1
    Reason:                Conflicted
    Status:                False
    Type:                  Accepted

if err != nil {
if services.IsNotFoundError(err) {
c.log.Infof("reconcile error, retry in 30sec: %s", err)
if !isDelete && services.IsNotFoundError(err) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What this condition used for !isDelete?

Copy link
Contributor Author

@mikhail-aws mikhail-aws Oct 27, 2023

Choose a reason for hiding this comment

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

Thanks for pointing, logic is wrong in the code. Intention was:
If it's policy delete request and SN or Svc not found, dont throw error, just remove finalizer and we done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should start doing unit tests in controllers :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Why I think old code is correct... !isDelete && services.IsNotFoundError(err)

Copy link
Contributor Author

@mikhail-aws mikhail-aws Oct 27, 2023

Choose a reason for hiding this comment

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

if it's policy delete and svc not found ignore, otherwise throw
(isDelete && notFound) -> ignore
!(isDelete && notFound) -> throw

err = c.handleConflicts(ctx, k8sPolicy)
if err != nil {
c.log.Error(err)
return ctrl.Result{RequeueAfter: 30 * time.Second}, nil
Copy link
Contributor

@zijun726911 zijun726911 Oct 27, 2023

Choose a reason for hiding this comment

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

for the Conflicted policy, should it just be "reconciled" with PolicyReasonConflicted status, instead of keep looping? (i.e., only the first IAMAuthPolicy takes effect and the controller just ignore other Conflicted ones )

Copy link
Contributor Author

@mikhail-aws mikhail-aws Oct 27, 2023

Choose a reason for hiding this comment

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

with loop it should converge by itself when conflict is resolved, for example if I start controller with 2 policies, both will be in conflict state. When I remove one of them another one will succeed in ~30 sec

@coveralls
Copy link

coveralls commented Oct 27, 2023

Pull Request Test Coverage Report for Build 6672227526

  • 0 of 53 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 44.763%

Changes Missing Coverage Covered Lines Changed/Added Lines %
controllers/iamauthpolicy_controller.go 0 53 0.0%
Totals Coverage Status
Change from base Build 6660280457: -0.2%
Covered Lines: 4133
Relevant Lines: 9233

💛 - Coveralls

@mikhail-aws mikhail-aws merged commit 41b59bb into aws:main Oct 27, 2023
@mikhail-aws mikhail-aws deleted the iam-policy branch October 27, 2023 21:14
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.

3 participants