service_network_manager changes for SNVA security group API support #409

zijun726911 · 2023-09-22T20:31:45Z

What type of PR is this?

Which issue does this PR fix: feature, #199 service_network_manager changes for SNVA security group API support

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

Testing done on this change:

Finished following manual test steps:

kubectl apply -f examples/my-hotel-gateway.yaml, create SN and SNVA success
apply:

apiVersion: application-networking.k8s.aws/v1alpha1
kind: VpcAssociationPolicy
metadata:
    name: test-vpc-association-policy
spec:
    targetRef:
        group: "gateway.networking.k8s.io"
        kind: Gateway
        name: my-hotel
    securityGroupIds:
        - sg-0063d957ef47d3020
    associateWithVpc: true

Update SNVA security group success
3. apply

apiVersion: application-networking.k8s.aws/v1alpha1
kind: VpcAssociationPolicy
metadata:
    name: test-vpc-association-policy
spec:
    targetRef:
        group: "gateway.networking.k8s.io"
        kind: Gateway
        name: my-hotel
    associateWithVpc: true

lattice.UpdateServiceNetworkVpcAssociationWithContext() return validation error and the controller keep LATTICE_RETRY as expected

apply

apiVersion: application-networking.k8s.aws/v1alpha1
kind: VpcAssociationPolicy
metadata:
    name: test-vpc-association-policy
spec:
    targetRef:
        group: "gateway.networking.k8s.io"
        kind: Gateway
        name: my-hotel
    associateWithVpc: false

SNVA deleted as expected

apply

apiVersion: application-networking.k8s.aws/v1alpha1
kind: VpcAssociationPolicy
metadata:
    name: test-vpc-association-policy
spec:
    targetRef:
        group: "gateway.networking.k8s.io"
        kind: Gateway
        name: my-hotel

SNVA is created, (as the controller by default will create SNVA, if the policy don't have associateWithVpc field)

Create vpc-association-policy first, then create the my-hotel gateway. vpc-association-policy can take effect, and all behavior as expected
All existing E2E test pass:

------------------------------

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

Automation added to e2e: Will add new E2E test cases in a separate PR

Will this PR introduce any new dependencies?: No

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

Does this PR introduce any user-facing change?: add new feature, no existing behavour changes

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

zijun726911 · 2023-09-22T20:32:55Z

pkg/deploy/lattice/service_network_manager_test.go


 	assert.Nil(t, err)
 	assert.Equal(t, resp.ServiceNetworkARN, meshArn)
 	assert.Equal(t, resp.ServiceNetworkID, meshId)
 }

-func Test_CreateServiceNetwork_MeshAlreadyExist_AssociateToNotAssociate(t *testing.T) {


Did the unit test name changes: from _CreateServiceNetwork_ to _CreateOrUpdateServiceNetwork_ i.e, from Test_CreateServiceNetwork_MeshAlreadyExist_AssociateToNotAssociate to Test_CreateOrUpdateServiceNetwork_MeshAlreadyExist_AssociateToNotAssociate

coveralls · 2023-09-22T20:48:39Z

Pull Request Test Coverage Report for Build 6331816403

79 of 84 (94.05%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 46.204%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/deploy/lattice/service_network_manager.go	64	69	92.75%

Totals
Change from base Build 6331357725:	0.2%
Covered Lines:	3931
Relevant Lines:	8508

💛 - Coveralls

pkg/utils/common.go

pkg/deploy/lattice/service_network_manager.go

scottlaiaws · 2023-09-26T17:34:37Z

pkg/deploy/lattice/service_network_manager.go

 		if err != nil {
 			return latticemodel.ServiceNetworkStatus{ServiceNetworkARN: "", ServiceNetworkID: ""}, err
 		}
+		if stackSn.Spec.AssociateToVPC == true && isServiceNetworkAlreadyAssociatedWithCurrentVpc == true &&


maybe you can refactor this out into a separate variable to make the condition more clear?

scottlaiaws · 2023-09-26T17:35:18Z

pkg/deploy/lattice/service_network_manager.go

-	if service_network.Spec.AssociateToVPC == true {
-		if isServiceNetworkAssociatedWithVPC == false {
+	if stackSn.Spec.AssociateToVPC == true {
+		if isServiceNetworkAlreadyAssociatedWithCurrentVpc == false {


nit: you can use the short version of the condition in if statements. like if condition or if !condition

mikhail-aws

Code looks more complex than problem. Please simplify names, use normal error handling order, cleanup logs. Also it's good time to refactor create/update function into separate create and update. For refactoring please submit PR with refactoring first, and then another PR with new feature.

pkg/deploy/lattice/service_network_manager.go

mikhail-aws · 2023-09-26T17:28:00Z

pkg/deploy/lattice/service_network_manager.go

@@ -53,66 +57,72 @@ type defaultServiceNetworkManager struct {
 // return errors.New(LATTICE_RETRY) when:
 //
 //	CreateServiceNetworkVpcAssociationInput returns ServiceNetworkVpcAssociationStatusFailed/ServiceNetworkVpcAssociationStatusCreateInProgress/MeshVpcAssociationStatusDeleteInProgress
-func (m *defaultServiceNetworkManager) Create(ctx context.Context, service_network *latticemodel.ServiceNetwork) (latticemodel.ServiceNetworkStatus, error) {
+func (m *defaultServiceNetworkManager) CreateOrUpdate(ctx context.Context, stackSn *latticemodel.ServiceNetwork) (latticemodel.ServiceNetworkStatus, error) {


why stackSn, not just sn?

Hope to to represent a meaning that this a the desired SN that user intend to create (refering the name stackDeployer)

looks cryptic, sn or serviceNetwork was ok

I found it have to differenate desiredSN and m.cloud.Lattice().FindServiceNetwork() retrieved SN, plan to change the name stackSn to desiredSN

I believe difference is apparent, Create function accept model of service network and tries to create or update lattice resources. There is no ambiguity what kind of service network it is.

Changed to serviceNetwork

pkg/deploy/lattice/service_network_manager.go

mikhail-aws · 2023-09-26T17:37:00Z

pkg/deploy/lattice/service_network_manager.go

+		return latticemodel.ServiceNetworkStatus{
+			ServiceNetworkID:     snId,
+			ServiceNetworkARN:    snArn,
+			SnvaSecurityGroupIds: nil,
+		}, errors.New(LATTICE_RETRY)


when return error, dont populate fields for struct

Will, retrun value latticemodel.ServiceNetworkStatus{} only used by unit test. I intend to do assertion for this return value

when you do unit test for error, you dont need to do assertion on ServiceNetworkStatus struct, it has no meaning when error returned - garbage value

changed to only assert err for unhappy path unit test

pkg/deploy/lattice/service_network_manager.go

pkg/utils/common.go

pkg/deploy/lattice/service_network_manager.go

zijun726911 commented Sep 22, 2023

View reviewed changes

zijun726911 requested a review from solmonk September 22, 2023 20:35

zijun726911 force-pushed the security_group_api_service_network_manager_changes branch from c793dba to e0760f8 Compare September 22, 2023 20:44

scottlaiaws reviewed Sep 26, 2023

View reviewed changes

pkg/utils/common.go Outdated Show resolved Hide resolved

scottlaiaws reviewed Sep 26, 2023

View reviewed changes

pkg/deploy/lattice/service_network_manager.go Outdated Show resolved Hide resolved

scottlaiaws reviewed Sep 26, 2023

View reviewed changes

mikhail-aws suggested changes Sep 26, 2023

View reviewed changes

zijun726911 force-pushed the security_group_api_service_network_manager_changes branch 2 times, most recently from 9704786 to 8c055ce Compare September 27, 2023 01:08

mikhail-aws reviewed Sep 27, 2023

View reviewed changes

zijun726911 force-pushed the security_group_api_service_network_manager_changes branch 2 times, most recently from bbc28e5 to 13d4cd6 Compare September 27, 2023 17:56

xWink reviewed Sep 27, 2023

View reviewed changes

pkg/deploy/lattice/service_network_manager.go Outdated Show resolved Hide resolved

xWink reviewed Sep 27, 2023

View reviewed changes

pkg/deploy/lattice/service_network_manager.go Outdated Show resolved Hide resolved

zijun726911 force-pushed the security_group_api_service_network_manager_changes branch from be3b292 to 11710aa Compare September 27, 2023 20:16

Zijun Wang added 2 commits September 27, 2023 14:40

solve conflict

4619403

Solve git conflict

eb92529

zijun726911 force-pushed the security_group_api_service_network_manager_changes branch from 11710aa to eb92529 Compare September 27, 2023 21:41

Address PR comments

2e1b0d7

mikhail-aws approved these changes Sep 27, 2023

View reviewed changes

zijun726911 merged commit 19e2299 into aws:main Sep 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

service_network_manager changes for SNVA security group API support #409

service_network_manager changes for SNVA security group API support #409

zijun726911 commented Sep 22, 2023 •

edited

Loading

zijun726911 Sep 22, 2023

coveralls commented Sep 22, 2023 •

edited

Loading

scottlaiaws Sep 26, 2023 •

edited

Loading

scottlaiaws Sep 26, 2023

mikhail-aws left a comment

mikhail-aws Sep 26, 2023

zijun726911 Sep 26, 2023

mikhail-aws Sep 26, 2023

zijun726911 Sep 26, 2023

mikhail-aws Sep 26, 2023

zijun726911 Sep 27, 2023

mikhail-aws Sep 26, 2023

zijun726911 Sep 26, 2023

mikhail-aws Sep 26, 2023

zijun726911 Sep 27, 2023

service_network_manager changes for SNVA security group API support #409

service_network_manager changes for SNVA security group API support #409

Conversation

zijun726911 commented Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

coveralls commented Sep 22, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6331816403

💛 - Coveralls

scottlaiaws Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikhail-aws left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zijun726911 commented Sep 22, 2023 •

edited

Loading

coveralls commented Sep 22, 2023 •

edited

Loading

scottlaiaws Sep 26, 2023 •

edited

Loading