EventHandling: Gateway/HTTPRoute/GRPCRoute resource changes enqueue events for attached IAMAuthPolicy/AccessLogPolicy resource #438

zijun726911 · 2023-10-16T17:02:04Z

Add resourceMapper.XXXXToIAMAuthPolicy() and XXXXToAccessLogPolicy() methods for event handling
Extend GetAttachedPolicy method to make it be able to get IAMAuthPolicy and AccessLogPolicy attached policy
Add IAMAuthPolicyController stub code

What type of PR is this? prepare for feature #18

Which issue does this PR fix: #18 #420

What does this PR do / Why do we need it:

If an issue # is not available please add repro steps and logs from aws-gateway-controller showing the issue:

Testing done on this change:

Manual test done for xxxEventHandler.MapToIAMAuthPolicy/MapToAccessLogPolicy():
1. Do kubectl apply -f examples/iam-auth-policy-example.yaml
2. Set debugger break points in gatewayEventHandler.MapToIAMAuthPolicy() and (r *authPolicyReconciler) Reconcile()
3. Do kubectl apply -f examples/my-hotel-gateway.yaml
4. Confirmed it could enter the (r *authPolicyReconciler) Reconcile()function for the "targetRef gw obj my-hotel", i.e., Gateway/HTTPRoute/GRPCRoute resource changes could enqueue attached IAMAuthPolicy/AccessLogPolicy

(Did the same thing for httpRoute, grpcRoute and MapToAccessLogPolicy() )

E2E Test Pass, except broken ones [FAIL] Creating Access Log Policy [It] sets Access Log Policy status to Conflicted when creating a new policy for the same targetRef and destination type

Ran 40 of 43 Specs in 2987.036 seconds
FAIL! -- 39 Passed | 1 Failed | 0 Pending | 3 Skipped

Will this PR introduce any new dependencies?:

No

Will this break upgrades or downgrades. Has updating a running cluster been tested?: No

Does this PR introduce any user-facing change?: No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

coveralls · 2023-10-16T17:11:57Z

Pull Request Test Coverage Report for Build 6541077103

42 of 205 (20.49%) changed or added relevant lines in 9 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.6%) to 44.721%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/gateway/utils.go	28	32	87.5%
controllers/accesslogpolicy_controller.go	0	8	0.0%
pkg/gateway/model_build_targetgroup.go	5	15	33.33%
controllers/eventhandlers/gateway.go	0	24	0.0%
controllers/eventhandlers/grpcroute.go	0	24	0.0%
controllers/eventhandlers/httproute.go	0	24	0.0%
controllers/eventhandlers/mapper.go	0	24	0.0%
controllers/iamauthpolicy_controller.go	0	45	0.0%

Totals
Change from base Build 6536982609:	-0.6%
Covered Lines:	3977
Relevant Lines:	8893

💛 - Coveralls

zijun726911 · 2023-10-16T23:09:02Z

pkg/apis/applicationnetworking/v1alpha1/accesslogpolicy_types.go

-	for i, item := range pl.Items {
-		items[i] = &item
+	for i := range pl.Items {
+		items[i] = &pl.Items[i]
 	}


Previous code items[i] = &item has a bug that mess up the item pointers. it must use items[i] = &pl.Items[i]

Thanks for catching this. What was the bug doing?

Previous way &item could cause this scenario:

if pl.Items actually have {httproute-1, httproute-2}
the GetItems() could return {&httproute-2, &httproute-2}

zijun726911 · 2023-10-16T23:16:19Z

pkg/gateway/utils_test.go

+				policy: &anv1alpha1.AccessLogPolicy{},
+			},
+			expectedK8sClientReturnedPolicies: []core.Policy{accessLogPolicy1HappyPath, accessLogPolicy2HappyPath},
+			want:                              []core.Policy{accessLogPolicy1HappyPath, accessLogPolicy2HappyPath},


GetAttachedPolicies() is able to return both 2 attatched accessLogPolicies

What about 3?

zijun726911 · 2023-10-17T00:35:13Z

pkg/gateway/model_build_targetgroup.go

+			t.log.Errorf("More than one TargetGroupPolicy is attached to the k8sService %s, "+
+				"only the first one TargetGroupPolicy [%s/%s] will take effect, other TargetGroupPolicies will be ignored", refObjNamespacedName, tgp.Namespace, tgp.Name)
+		}
+	}


Ideally we should do that behaviour: If there are two tgps attached to a svc(serviceExport), set one policy status to accepted and set anothers status to conflicted.

But that need more extra work considering our current code structure (GetTargetGroupPolicy logic exists in the giant route reconcile logic). We could impliment that behaviour in a seperate PR. Issue: #440

For now, we just keep old code and new code have same behaviour: just make the first tgp(same for VpcAssociationPolicy) take effect and ignore other same kind policies (print a Error for it)

…) methods for event handling - Extend GetAttachedPolicy method to make it be able to map to multiple IAMAuthPolicy and AccessLogPolicy attached policies - Add IAMAuthPolicyController stub code

xWink · 2023-10-17T17:11:25Z

controllers/eventhandlers/gateway.go

+		if gw, ok := obj.(*gateway_api.Gateway); ok {
+			policies := h.mapper.GatewayToAccessLogPolicies(context.Background(), gw)
+			for _, p := range policies {
+				h.log.Infof("Gateway [%s/%s] resource change triggers AccessLogPolicy [%s/%s] resource change", gw.Namespace, gw.Name, p.Namespace, p.Name)


Why use square brackets around the Gateway and Policy namespaced names? We don't do this elsewhere in the codebase

xWink · 2023-10-17T17:14:54Z

controllers/eventhandlers/mapper.go

@@ -73,6 +76,36 @@ func (r *resourceMapper) VpcAssociationPolicyToGateway(ctx context.Context, vap
 	return policyToTargetRefObj(r, ctx, vap, &gateway_api.Gateway{})
 }

+func (r *resourceMapper) GatewayToIAMAuthPolicies(ctx context.Context, gw *gateway_api.Gateway) []*anv1alpha1.IAMAuthPolicy {
+	policies, _ := gateway.GetAttachedPolicies(ctx, r.client, k8s.NamespacedName(gw), &anv1alpha1.IAMAuthPolicy{})


What happens if there's an error here?

xWink · 2023-10-17T17:15:23Z

controllers/iamauthpolicy_controller.go

+	authPolicyFinalizer = "iamauthpolicy.k8s.aws/resources"
+)
+
+type authPolicyReconciler struct {


iamAuthPolicyReconciler

solmonk · 2023-10-17T17:12:19Z

controllers/eventhandlers/gateway.go

@@ -21,6 +22,7 @@ import (
 	"github.com/aws/aws-application-networking-k8s/pkg/config"
 )

+// TODO: Remove `enqueueRequestsForGatewayEvent`, and use `gatewayEventHandler` only


why can we not do this now?

solmonk · 2023-10-17T17:14:19Z

controllers/eventhandlers/grpcroute.go

+		mapper: &resourceMapper{log: log, client: client}}
+}
+
+func (h *grpcRouteEventHandler) MapToIAMAuthPolicies() handler.EventHandler {


I would like to have a test code for this, easier way to test is extract logic into method level so that you can refer to it outside

I have unit tests for GetAttachedPolicies() does it enough?

solmonk · 2023-10-17T17:20:06Z

controllers/eventhandlers/mapper.go

@@ -73,6 +76,36 @@ func (r *resourceMapper) VpcAssociationPolicyToGateway(ctx context.Context, vap
 	return policyToTargetRefObj(r, ctx, vap, &gateway_api.Gateway{})
 }

+func (r *resourceMapper) GatewayToIAMAuthPolicies(ctx context.Context, gw *gateway_api.Gateway) []*anv1alpha1.IAMAuthPolicy {


I don't really like a dependency relation from eventhandler to gateway package, I think this means something is wrong. GetAttachedPolicies() will be more suitable for a separate package.

Ok, will change

xWink · 2023-10-17T17:34:36Z

pkg/gateway/utils.go

 }

-func policyTypeToPolicyListAndTargetRefGroupKind(policyType core.Policy) (core.PolicyList, gwv1beta1.Group, gwv1beta1.Kind, error) {
+func policyTypeToPolicyListAndValidTargetRefObjGKs(policyType core.Policy) (core.PolicyList, map[schema.GroupKind]interface{}, error) {


The meaning of this is unclear, what is the goal of this function? The implementation doesn't make it clear either

xWink · 2023-10-17T17:36:03Z

pkg/gateway/utils_test.go

+			expectPolicyNotFound:              false,
+		},
+		{
+			name: "Get gateway attached IAMAuthPolicy, happy path",


What is the expected result of happy path? Typically, we shouldn't put "happy path" in the name of a test, but rather state what it is we expect the outcome to be

zijun726911 force-pushed the iam-policy-event-handling branch from 248f39a to a63f1e2 Compare October 16, 2023 17:09

zijun726911 changed the title ~~EventHandling: IAMAuthPolicy/AccessLogPolicy Map To TargetRef Gateway/HTTPRoute/GRPCRoute~~ EventHandling: Gateway/HTTPRoute/GRPCRoute resource changes could enqueue attached IAMAuthPolicy/AccessLogPolicy Oct 16, 2023

zijun726911 requested review from xWink, mikhail-aws and solmonk October 16, 2023 17:11

zijun726911 changed the title ~~EventHandling: Gateway/HTTPRoute/GRPCRoute resource changes could enqueue attached IAMAuthPolicy/AccessLogPolicy~~ EventHandling: Gateway/HTTPRoute/GRPCRoute resource changes enqueues attached IAMAuthPolicy/AccessLogPolicy Oct 16, 2023

zijun726911 changed the title ~~EventHandling: Gateway/HTTPRoute/GRPCRoute resource changes enqueues attached IAMAuthPolicy/AccessLogPolicy~~ EventHandling: Gateway/HTTPRoute/GRPCRoute resource changes enqueues event for attached IAMAuthPolicy/AccessLogPolicy resource Oct 16, 2023

zijun726911 changed the title ~~EventHandling: Gateway/HTTPRoute/GRPCRoute resource changes enqueues event for attached IAMAuthPolicy/AccessLogPolicy resource~~ EventHandling: Gateway/HTTPRoute/GRPCRoute resource changes enqueue events for attached IAMAuthPolicy/AccessLogPolicy resource Oct 16, 2023

zijun726911 force-pushed the iam-policy-event-handling branch 2 times, most recently from 465b177 to 8905c08 Compare October 16, 2023 23:11

zijun726911 commented Oct 16, 2023

View reviewed changes

zijun726911 force-pushed the iam-policy-event-handling branch from 8905c08 to 15a0583 Compare October 17, 2023 00:22

zijun726911 removed request for solmonk, xWink and mikhail-aws October 17, 2023 00:23

zijun726911 force-pushed the iam-policy-event-handling branch from 15a0583 to 62366d5 Compare October 17, 2023 00:27

zijun726911 commented Oct 17, 2023

View reviewed changes

- Add resourceMapper.XXXXToIAMAuthPolicy() and XXXXToAccessLogPolicy(…

d6cd855

…) methods for event handling - Extend GetAttachedPolicy method to make it be able to map to multiple IAMAuthPolicy and AccessLogPolicy attached policies - Add IAMAuthPolicyController stub code

zijun726911 force-pushed the iam-policy-event-handling branch from 62366d5 to d6cd855 Compare October 17, 2023 00:54

xWink reviewed Oct 17, 2023

View reviewed changes

solmonk reviewed Oct 17, 2023

View reviewed changes

xWink reviewed Oct 17, 2023

View reviewed changes

zijun726911 closed this Oct 25, 2023

zijun726911 deleted the iam-policy-event-handling branch November 2, 2023 22:34

zijun726911 restored the iam-policy-event-handling branch May 18, 2024 02:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EventHandling: Gateway/HTTPRoute/GRPCRoute resource changes enqueue events for attached IAMAuthPolicy/AccessLogPolicy resource #438

EventHandling: Gateway/HTTPRoute/GRPCRoute resource changes enqueue events for attached IAMAuthPolicy/AccessLogPolicy resource #438

zijun726911 commented Oct 16, 2023 •

edited

Loading

coveralls commented Oct 16, 2023 •

edited

Loading

zijun726911 Oct 16, 2023

xWink Oct 17, 2023

zijun726911 Oct 18, 2023

zijun726911 Oct 16, 2023

xWink Oct 17, 2023

zijun726911 Oct 17, 2023 •

edited

Loading

xWink Oct 17, 2023

xWink Oct 17, 2023

xWink Oct 17, 2023

solmonk Oct 17, 2023

solmonk Oct 17, 2023

zijun726911 Oct 18, 2023

solmonk Oct 17, 2023

zijun726911 Oct 18, 2023

xWink Oct 17, 2023 •

edited

Loading

xWink Oct 17, 2023

EventHandling: Gateway/HTTPRoute/GRPCRoute resource changes enqueue events for attached IAMAuthPolicy/AccessLogPolicy resource #438

EventHandling: Gateway/HTTPRoute/GRPCRoute resource changes enqueue events for attached IAMAuthPolicy/AccessLogPolicy resource #438

Conversation

zijun726911 commented Oct 16, 2023 • edited Loading

coveralls commented Oct 16, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6541077103

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zijun726911 Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xWink Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zijun726911 commented Oct 16, 2023 •

edited

Loading

coveralls commented Oct 16, 2023 •

edited

Loading

zijun726911 Oct 17, 2023 •

edited

Loading

xWink Oct 17, 2023 •

edited

Loading