Read VpcAssociationPolicy value in the model_build_servicenetwork #407

zijun726911 · 2023-09-20T20:45:37Z

What type of PR is this? feature

Which issue does this PR fix: #199 part of security group api support, this PR don't include targets_synthesizer.go changes, so this PR will not pass security groups ids to the aws vpc lattice sdk, will do that in a next seperate PR

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

Testing done on this change:

Add extra unit tests
e2etest all pass

Ran 31 of 31 Specs in 2239.955 seconds
SUCCESS! -- 31 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestIntegration (2240.27s)
PASS

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?:

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

coveralls · 2023-09-20T20:50:10Z

Pull Request Test Coverage Report for Build 6264742445

30 of 33 (90.91%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 39.353%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
controllers/gateway_controller.go	0	1	0.0%
pkg/gateway/model_build_servicenetwork.go	30	32	93.75%

Totals
Change from base Build 6253500649:	0.07%
Covered Lines:	4075
Relevant Lines:	10355

💛 - Coveralls

zijun726911 · 2023-09-20T20:53:59Z

pkg/model/lattice/servicenetwork.go

+	Name             string    `json:"name"`
+	Namespace        string    `json:"namespace"`
+	Account          string    `json:"account"`
+	SecurityGroupIds []*string `json:"securityGroupIds"`


aws vpc lattice go sdk use the []*string type to pass the security group ids, keep it in the same type here

scottlaiaws · 2023-09-21T16:46:31Z

pkg/gateway/model_build_servicenetwork.go

-	mesh *latticemodel.ServiceNetwork
+	gateway              *gateway_api.Gateway
+	vpcAssociationPolicy *v1alpha1.VpcAssociationPolicy
+	mesh                 *latticemodel.ServiceNetwork


can you rename mesh here?

scottlaiaws · 2023-09-21T16:47:15Z

pkg/gateway/model_build_servicenetwork.go

 			} else {
-				spec.AssociateToVPC = false
+				associateToVPC = false


nit: can be associateToVPC = value == "true"

scottlaiaws · 2023-09-21T16:48:55Z

pkg/gateway/model_build_servicenetwork.go

 		}

+		if t.vpcAssociationPolicy.Spec.SecurityGroupIds != nil {
+			spec.SecurityGroupIds = securityGroupIdsToStringPointersSlice(t.vpcAssociationPolicy.Spec.SecurityGroupIds)


you can use aws.StringSlice or aws.StringValueSlice here to convert from []string to []*string and vice versa

I found the aws.StringSlice(src []string) can only accept []string input param, but the t.vpcAssociationPolicy.Spec.SecurityGroupIds is actually []SecurityGroupId cannot use that method

scottlaiaws

LGTM

scottlaiaws · 2023-09-21T17:16:56Z

Just to be safe, can you rebase/merge with upsteram/main and run the e2e tests again? I added a test case in my previous PR #406

solmonk · 2023-09-21T19:24:57Z

pkg/gateway/model_build_servicenetwork.go

-	// by default it is true
-	spec.AssociateToVPC = true
+	// default associateToVPC is true
+	associateToVPC := true
 	if len(t.gateway.ObjectMeta.Annotations) > 0 {


I believe there's no need to check this here

solmonk · 2023-09-21T19:29:40Z

pkg/gateway/model_build_servicenetwork.go


+	if t.vpcAssociationPolicy != nil {
+		if t.vpcAssociationPolicy.Spec.AssociateWithVpc != nil {


did you set default value as true in the new CRD?

No, In CRD, by default it's empty

aws-application-networking-k8s/pkg/apis/applicationnetworking/v1alpha1/vpcassociationpolicy_types.go

Line 58 in 5204483

AssociateWithVpc *bool `json:"associateWithVpc,omitempty"`

In the controller for both old and new logic, by default it is true

have you considered setting default value via kubebuilder annotation?

solmonk · 2023-09-21T19:31:36Z

pkg/gateway/model_build_servicenetwork.go

 }
 func (b *serviceNetworkModelBuilder) Build(ctx context.Context, gw *gateway_api.Gateway) (core.Stack, *latticemodel.ServiceNetwork, error) {
 	stack := core.NewDefaultStack(core.StackID(k8s.NamespacedName(gw)))
-
+	vpcAssociationPolicy, err := GetAttachedPolicy(ctx, b.client, k8s.NamespacedName(gw), &v1alpha1.VpcAssociationPolicy{})


what happens if the policy is not found?

If policy not found, GetAttachedPolicy() retrun nil, nil, it will go ahead to execute the rest of logic. Unit test covered that

aws-application-networking-k8s/pkg/gateway/utils_test.go

Line 205 in 5204483

assert.Nil(t, err)

zijun726911 · 2023-09-21T22:09:32Z

New e2etest also pass

Ran 32 of 32 Specs in 2112.850 seconds
SUCCESS! -- 32 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestIntegration (2113.15s)
PASS

zijun726911 requested review from solmonk and scottlaiaws September 20, 2023 20:45

zijun726911 force-pushed the sg_api_model_build_servicenetwork_changes branch 2 times, most recently from efd824e to 062f883 Compare September 20, 2023 20:49

zijun726911 force-pushed the sg_api_model_build_servicenetwork_changes branch from 062f883 to bc3607c Compare September 20, 2023 20:53

zijun726911 commented Sep 20, 2023

View reviewed changes

zijun726911 removed request for solmonk and scottlaiaws September 20, 2023 20:58

Read VpcAssociationPolicy value in the model_build_servicenetwork

8893809

zijun726911 force-pushed the sg_api_model_build_servicenetwork_changes branch from bc3607c to 8893809 Compare September 20, 2023 20:59

zijun726911 requested a review from solmonk September 20, 2023 23:15

scottlaiaws reviewed Sep 21, 2023

View reviewed changes

Address PR comments

504ba94

scottlaiaws approved these changes Sep 21, 2023

View reviewed changes

solmonk reviewed Sep 21, 2023

View reviewed changes

Address PR comments

3b161aa

solmonk approved these changes Sep 21, 2023

View reviewed changes

zijun726911 merged commit d716a0f into aws:main Sep 21, 2023
5 checks passed

zijun726911 deleted the sg_api_model_build_servicenetwork_changes branch October 3, 2023 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read VpcAssociationPolicy value in the model_build_servicenetwork #407

Read VpcAssociationPolicy value in the model_build_servicenetwork #407

zijun726911 commented Sep 20, 2023 •

edited

Loading

coveralls commented Sep 20, 2023 •

edited

Loading

zijun726911 Sep 20, 2023

scottlaiaws Sep 21, 2023

scottlaiaws Sep 21, 2023

scottlaiaws Sep 21, 2023 •

edited

Loading

zijun726911 Sep 21, 2023

scottlaiaws left a comment

scottlaiaws commented Sep 21, 2023

solmonk Sep 21, 2023

solmonk Sep 21, 2023

zijun726911 Sep 21, 2023

solmonk Sep 21, 2023

solmonk Sep 21, 2023

zijun726911 Sep 21, 2023

zijun726911 commented Sep 21, 2023


		if t.vpcAssociationPolicy != nil {
		if t.vpcAssociationPolicy.Spec.AssociateWithVpc != nil {

Read VpcAssociationPolicy value in the model_build_servicenetwork #407

Read VpcAssociationPolicy value in the model_build_servicenetwork #407

Conversation

zijun726911 commented Sep 20, 2023 • edited Loading

coveralls commented Sep 20, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6264742445

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottlaiaws Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottlaiaws left a comment

Choose a reason for hiding this comment

scottlaiaws commented Sep 21, 2023

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

zijun726911 commented Sep 21, 2023

zijun726911 commented Sep 20, 2023 •

edited

Loading

coveralls commented Sep 20, 2023 •

edited

Loading

scottlaiaws Sep 21, 2023 •

edited

Loading