Address PR comments

aws · Sep 27, 2023 · be3b292 · be3b292
1 parent 13d4cd6
commit be3b292
Show file tree

Hide file tree

Showing 2 changed files with 7 additions and 25 deletions.
diff --git a/pkg/deploy/lattice/service_network_manager.go b/pkg/deploy/lattice/service_network_manager.go
@@ -3,6 +3,7 @@ package lattice
 import (
 	"context"
 	"errors"
+	"fmt"
 
 	"golang.org/x/exp/slices"
 
@@ -83,7 +84,7 @@ func (m *defaultServiceNetworkManager) CreateOrUpdate(ctx context.Context, desir
 		}
 		serviceNetworkInput.Tags[latticemodel.K8SServiceNetworkOwnedByVPC] = &config.VpcID
 
-		m.log.Debugf("Creating desiredSn %+v", serviceNetworkInput)
+		m.log.Debugf("Creating ServiceNetwork %+v", serviceNetworkInput)
 		resp, err := vpcLatticeSess.CreateServiceNetworkWithContext(ctx, &serviceNetworkInput)
 		if err != nil {
 			return latticemodel.ServiceNetworkStatus{ServiceNetworkARN: "", ServiceNetworkID: ""}, err
@@ -92,7 +93,7 @@ func (m *defaultServiceNetworkManager) CreateOrUpdate(ctx context.Context, desir
 		serviceNetworkId = aws.StringValue(resp.Id)
 		serviceNetworkArn = aws.StringValue(resp.Arn)
 	} else {
-		m.log.Debugf("desiredSn %s exists, checking its VPC association", desiredSn.Spec.Name)
+		m.log.Debugf("ServiceNetwork %s exists, checking its VPC association", desiredSn.Spec.Name)
 		serviceNetworkId = aws.StringValue(foundSnSummary.SvcNetwork.Id)
 		serviceNetworkArn = aws.StringValue(foundSnSummary.SvcNetwork.Arn)
 		isSnAlreadyAssociatedWithCurrentVpc, snvaAssociatedWithCurrentVPC, _, err = m.isServiceNetworkAlreadyAssociatedWithVPC(ctx, serviceNetworkId)
@@ -152,7 +153,7 @@ func (m *defaultServiceNetworkManager) CreateOrUpdate(ctx context.Context, desir
 			// return retry and check later if disassociation workflow finishes
 			return latticemodel.ServiceNetworkStatus{ServiceNetworkARN: "", ServiceNetworkID: ""}, errors.New(LATTICE_RETRY)
 		}
-		m.log.Debugf("Created desiredSn %s without VPC association", desiredSn.Spec.Name)
+		m.log.Debugf("Created ServiceNetwork %s without VPC association", desiredSn.Spec.Name)
 	}
 	return latticemodel.ServiceNetworkStatus{ServiceNetworkARN: serviceNetworkArn, ServiceNetworkID: serviceNetworkId}, nil
 }
@@ -294,20 +295,7 @@ func (m *defaultServiceNetworkManager) UpdateServiceNetworkVpcAssociation(ctx co
 	if err != nil {
 		return latticemodel.ServiceNetworkStatus{}, err
 	}
-
-	if len(desiredSN.Spec.SecurityGroupIds) == 0 && retrievedSnva.SecurityGroupIds != nil && len(retrievedSnva.SecurityGroupIds) > 0 {
-		// VPC lattice API have a limitation that the user can't remove all security groups if current snva already has security groups.
-		// If that case happen, the controller will not delete snva. User should manually delete it.
-		//https://docs.aws.amazon.com/vpc-lattice/latest/ug/security-groups.html#security-groups-rules
-		m.log.Errorf("Can't remove all security groups from service network vpc association, for service network %s, snvaId %s", desiredSN.Spec.Name, retrievedSnva.Id)
-
-		// TODO: Add validation webhook or update gateway (or VpcAssociationPolicy) status to indicate that error.
-		//  https://gateway-api.sigs.k8s.io/geps/gep-713/#standard-status-condition
-		return latticemodel.ServiceNetworkStatus{}, errors.New(LATTICE_RETRY)
-	}
-
 	sgIdsEqual := securityGroupIdsEqual(desiredSN.Spec.SecurityGroupIds, retrievedSnva.SecurityGroupIds)
-
 	if sgIdsEqual {
 		// desiredSN's security group ids are same with retrievedSnva's security group ids, don't need to update
 		return latticemodel.ServiceNetworkStatus{
@@ -316,25 +304,22 @@ func (m *defaultServiceNetworkManager) UpdateServiceNetworkVpcAssociation(ctx co
 			SnvaSecurityGroupIds: retrievedSnva.SecurityGroupIds,
 		}, nil
 	}
-
 	updateSnvaResp, err := m.cloud.Lattice().UpdateServiceNetworkVpcAssociationWithContext(ctx, &vpclattice.UpdateServiceNetworkVpcAssociationInput{
 		ServiceNetworkVpcAssociationIdentifier: existingSnvaId,
 		SecurityGroupIds:                       desiredSN.Spec.SecurityGroupIds,
 	})
 	if err != nil {
 		return latticemodel.ServiceNetworkStatus{}, err
 	}
-
 	if *updateSnvaResp.Status == vpclattice.ServiceNetworkVpcAssociationStatusActive {
 		return latticemodel.ServiceNetworkStatus{
 			ServiceNetworkID:     *existingSN.Id,
 			ServiceNetworkARN:    *existingSN.Arn,
 			SnvaSecurityGroupIds: updateSnvaResp.SecurityGroupIds,
 		}, nil
 	} else {
-		return latticemodel.ServiceNetworkStatus{}, errors.New(LATTICE_RETRY)
+		return latticemodel.ServiceNetworkStatus{}, fmt.Errorf("%w, update svna status: %s", RetryErr, *updateSnvaResp.Status)
 	}
-
 }
 
 func securityGroupIdsEqual(arr1, arr2 []*string) bool {

diff --git a/pkg/gateway/model_build_servicenetwork.go b/pkg/gateway/model_build_servicenetwork.go
@@ -85,10 +85,7 @@ func (t *serviceNetworkModelBuildTask) buildServiceNetwork(ctx context.Context)
 		if t.vpcAssociationPolicy.Spec.AssociateWithVpc != nil {
 			associateToVPC = *t.vpcAssociationPolicy.Spec.AssociateWithVpc
 		}
-
-		if t.vpcAssociationPolicy.Spec.SecurityGroupIds != nil {
-			spec.SecurityGroupIds = securityGroupIdsToStringPointersSlice(t.vpcAssociationPolicy.Spec.SecurityGroupIds)
-		}
+		spec.SecurityGroupIds = securityGroupIdsToStringPointersSlice(t.vpcAssociationPolicy.Spec.SecurityGroupIds)
 	}
 	spec.AssociateToVPC = associateToVPC
 	defaultSN, err := config.GetClusterLocalGateway()
@@ -118,7 +115,7 @@ type serviceNetworkModelBuildTask struct {
 }
 
 func securityGroupIdsToStringPointersSlice(sgIds []v1alpha1.SecurityGroupId) []*string {
-	var ret []*string
+	ret := make([]*string, 0)
 	for _, sgId := range sgIds {
 		sgIdStr := string(sgId)
 		ret = append(ret, &sgIdStr)