Skip to content

Conversation

@mikhail-aws
Copy link
Contributor

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

Note:
Add model and manager for IAM Auth Policy. Currently handles only happy path when SN or Svc exists. If Lattice targetRef does not exists will print NotFound exceptions and retry later.

Always change AuthPolicy for Sn and Svc to AWS_IAM on upsert.

Tests:
Created SN and Svc in Lattice console. Applied and deleted examples/iam-auth-policy-example.yaml for SN and Svc.

@coveralls
Copy link

coveralls commented Oct 24, 2023

Pull Request Test Coverage Report for Build 6657681222

  • 33 of 210 (15.71%) changed or added relevant lines in 4 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.6%) to 45.066%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/aws/services/vpclattice.go 0 6 0.0%
pkg/deploy/lattice/iamauthpolicy_manager.go 33 50 66.0%
pkg/aws/services/vpclattice_mocks.go 0 22 0.0%
controllers/iamauthpolicy_controller.go 0 132 0.0%
Files with Coverage Reduction New Missed Lines %
controllers/iamauthpolicy_controller.go 3 0.0%
Totals Coverage Status
Change from base Build 6634175812: -0.6%
Covered Lines: 4133
Relevant Lines: 9171

💛 - Coveralls

xWink
xWink previously requested changes Oct 24, 2023
Copy link
Member

@xWink xWink left a comment

Choose a reason for hiding this comment

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

Looks good for happy path, just a few small requests

Copy link
Contributor

@zijun726911 zijun726911 left a comment

Choose a reason for hiding this comment

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

Could you describe some manually tests you did for the PR?

Overall LGTM, This code got rid of our old cumbersome style: ModelBuilder, StackDeployer... Hopfully all our later controller code(that handle new kind of k8s resource) could do that. I really love this new version.

}
} else {
if controllerutil.ContainsFinalizer(k8sPolicy, authPolicyFinalizer) {
controllerutil.RemoveFinalizer(k8sPolicy, authPolicyFinalizer)
Copy link
Contributor

Choose a reason for hiding this comment

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

When handling resouce deletion, we should do RemoveFinalizer() at very last, to avoid actual reconcile logic return err and k8s api server lose track of that IAMAuthPolicy resource?

For example, in our other controller:

if err := r.cleanupRouteResources(ctx, route); err != nil {
return fmt.Errorf("failed to cleanup GRPCRoute %s, %s: %w", route.Name(), route.Namespace(), err)
}
if err := updateRouteListenerStatus(ctx, r.client, route); err != nil {
return err
}
r.log.Infow("reconciled", "name", req.Name)
return r.finalizerManager.RemoveFinalizers(ctx, route.K8sObject(), routeTypeToFinalizer[r.routeType])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RemoveFinalizer() only mutates object, but does not call kube-api. Need to do client.Update/Patch to apply changes.
In my case I do update at the very last step in Reconcile function, so until then finalizer is not added or removed

@mikhail-aws
Copy link
Contributor Author

Could you describe some manually tests you did for the PR?

@zijun726911 updated description with manual tests

Copy link
Contributor

@zijun726911 zijun726911 left a comment

Choose a reason for hiding this comment

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

LGTM

@mikhail-aws mikhail-aws dismissed xWink’s stale review October 26, 2023 18:20

approved by zijun726911

@mikhail-aws mikhail-aws merged commit 21eb9ed into aws:main Oct 26, 2023
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.

4 participants