Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read VpcAssociationPolicy value in the model_build_servicenetwork #407

Merged
merged 3 commits into from
Sep 21, 2023
Merged

Read VpcAssociationPolicy value in the model_build_servicenetwork #407

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8893809
Read VpcAssociationPolicy value in the model_build_servicenetwork
Sep 20, 2023
504ba94
Address PR comments
Sep 21, 2023
3b161aa
Address PR comments
Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion controllers/gateway_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func RegisterGatewayController(
scheme := mgr.GetScheme()
evtRec := mgr.GetEventRecorderFor("gateway")

modelBuilder := gateway.NewServiceNetworkModelBuilder()
modelBuilder := gateway.NewServiceNetworkModelBuilder(mgrClient)
stackDeployer := deploy.NewServiceNetworkStackDeployer(cloud, mgrClient)
stackMarshaller := deploy.NewDefaultStackMarshaller()

Expand Down
4 changes: 3 additions & 1 deletion pkg/deploy/lattice/service_network_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ package lattice
import (
"context"
"errors"
"github.com/aws/aws-application-networking-k8s/pkg/aws/services"

"github.com/golang/glog"

"github.com/aws/aws-application-networking-k8s/pkg/aws/services"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/vpclattice"

Expand Down
8 changes: 6 additions & 2 deletions pkg/deploy/lattice/service_network_synthesizer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (

"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
gateway_api "sigs.k8s.io/gateway-api/apis/v1beta1"

Expand Down Expand Up @@ -87,14 +86,19 @@ func Test_SynthesizeTriggeredGateways(t *testing.T) {
},
}

c := gomock.NewController(t)
defer c.Finish()
k8sClient := mock_client.NewMockClient(c)
k8sClient.EXPECT().List(context.Background(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()

for _, tt := range tests {

fmt.Printf("Testing >>>>> %v\n", tt.name)
c := gomock.NewController(t)
defer c.Finish()
ctx := context.TODO()

builder := gateway.NewServiceNetworkModelBuilder()
builder := gateway.NewServiceNetworkModelBuilder(k8sClient)

stack, mesh, _ := builder.Build(context.Background(), tt.gw)

Expand Down
137 changes: 127 additions & 10 deletions pkg/gateway/model_build_service_network_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,25 +5,46 @@ import (
"fmt"
"testing"

"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"
"sigs.k8s.io/gateway-api/apis/v1alpha2"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
gateway_api "sigs.k8s.io/gateway-api/apis/v1beta1"

mock_client "github.com/aws/aws-application-networking-k8s/mocks/controller-runtime/client"
"github.com/aws/aws-application-networking-k8s/pkg/apis/applicationnetworking/v1alpha1"
)

func Test_MeshModelBuild(t *testing.T) {
now := metav1.Now()
trueBool := true
falseBool := false
notRelatedVpcAssociationPolicy := v1alpha1.VpcAssociationPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: "another-vpc-association-policy",
},
Spec: v1alpha1.VpcAssociationPolicySpec{
TargetRef: &v1alpha2.PolicyTargetReference{
Group: gateway_api.GroupName,
Kind: "Gateway",
Name: "another-mesh",
},
AssociateWithVpc: &falseBool,
},
}
tests := []struct {
name string
gw *gateway_api.Gateway
wantErr error
wantName string
wantNamespace string
wantIsDeleted bool
associateToVPC bool
name string
gw *gateway_api.Gateway
vpcAssociationPolicy *v1alpha1.VpcAssociationPolicy
wantErr error
wantName string
wantNamespace string
wantIsDeleted bool
associateToVPC bool
}{
{
name: "Adding Mesh in default namespace, no annotation on VPC association",
name: "Adding Mesh in default namespace, no annotation on VPC association, associate to VPC by default",
gw: &gateway_api.Gateway{
ObjectMeta: metav1.ObjectMeta{
Name: "mesh1",
Expand Down Expand Up @@ -92,13 +113,106 @@ func Test_MeshModelBuild(t *testing.T) {
wantIsDeleted: true,
associateToVPC: true,
},
{
name: "Gateway has attached VpcAssociationPolicy found, VpcAssociationPolicy SecurityGroupIds are not empty",
gw: &gateway_api.Gateway{
ObjectMeta: metav1.ObjectMeta{
Name: "mesh1",
Finalizers: []string{"gateway.k8s.aws/resources"},
},
},
vpcAssociationPolicy: &v1alpha1.VpcAssociationPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: "test-vpc-association-policy",
},
Spec: v1alpha1.VpcAssociationPolicySpec{
TargetRef: &v1alpha2.PolicyTargetReference{
Group: gateway_api.GroupName,
Kind: "Gateway",
Name: "mesh1",
},
SecurityGroupIds: []v1alpha1.SecurityGroupId{"sg-123456", "sg-654321"},
},
},
wantErr: nil,
wantName: "mesh1",
wantNamespace: "",
wantIsDeleted: false,
associateToVPC: true,
},
{
name: "Gateway does not have LatticeVPCAssociationAnnotation, it has attached VpcAssociationPolicy found, which AssociateWithVpc field set to true",
gw: &gateway_api.Gateway{
ObjectMeta: metav1.ObjectMeta{
Name: "mesh1",
Finalizers: []string{"gateway.k8s.aws/resources"},
},
},
vpcAssociationPolicy: &v1alpha1.VpcAssociationPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: "test-vpc-association-policy",
},
Spec: v1alpha1.VpcAssociationPolicySpec{
TargetRef: &v1alpha2.PolicyTargetReference{
Group: gateway_api.GroupName,
Kind: "Gateway",
Name: "mesh1",
},
AssociateWithVpc: &trueBool,
},
},
wantErr: nil,
wantName: "mesh1",
wantNamespace: "",
wantIsDeleted: false,
associateToVPC: true,
},
{
name: "Gateway does not have LatticeVPCAssociationAnnotation, it has attached VpcAssociationPolicy found, which AssociateWithVpc field set to false",
gw: &gateway_api.Gateway{
ObjectMeta: metav1.ObjectMeta{
Name: "mesh1",
Finalizers: []string{"gateway.k8s.aws/resources"},
},
},
vpcAssociationPolicy: &v1alpha1.VpcAssociationPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: "test-vpc-association-policy",
},
Spec: v1alpha1.VpcAssociationPolicySpec{
TargetRef: &v1alpha2.PolicyTargetReference{
Group: gateway_api.GroupName,
Kind: "Gateway",
Name: "mesh1",
},
AssociateWithVpc: &falseBool,
},
},
wantErr: nil,
wantName: "mesh1",
wantNamespace: "",
wantIsDeleted: false,
associateToVPC: false,
},
}
c := gomock.NewController(t)
defer c.Finish()
mock_client := mock_client.NewMockClient(c)
ctx := context.Background()

for _, tt := range tests {
fmt.Printf("Testing >>> %v\n", tt.name)
t.Run(tt.name, func(t *testing.T) {
builder := NewServiceNetworkModelBuilder()

mock_client.EXPECT().List(ctx, gomock.Any(), gomock.Any()).DoAndReturn(
func(ctx context.Context, policyList *v1alpha1.VpcAssociationPolicyList, arg3 ...interface{}) error {
policyList.Items = append(policyList.Items, notRelatedVpcAssociationPolicy)
if tt.vpcAssociationPolicy != nil {
policyList.Items = append(policyList.Items, *tt.vpcAssociationPolicy)
}
return nil
},
)
builder := NewServiceNetworkModelBuilder(mock_client)
_, got, err := builder.Build(context.Background(), tt.gw)

if tt.wantErr != nil {
Expand All @@ -108,6 +222,9 @@ func Test_MeshModelBuild(t *testing.T) {
assert.Equal(t, tt.wantNamespace, got.Spec.Namespace)
assert.Equal(t, tt.wantIsDeleted, got.Spec.IsDeleted)
assert.Equal(t, tt.associateToVPC, got.Spec.AssociateToVPC)
if tt.vpcAssociationPolicy != nil {
assert.Equal(t, securityGroupIdsToStringPointersSlice(tt.vpcAssociationPolicy.Spec.SecurityGroupIds), got.Spec.SecurityGroupIds)
}
}

})
Expand Down
62 changes: 41 additions & 21 deletions pkg/gateway/model_build_servicenetwork.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ import (
"context"

corev1 "k8s.io/api/core/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
gateway_api "sigs.k8s.io/gateway-api/apis/v1beta1"

"github.com/aws/aws-application-networking-k8s/pkg/apis/applicationnetworking/v1alpha1"
"github.com/aws/aws-application-networking-k8s/pkg/config"
"github.com/aws/aws-application-networking-k8s/pkg/k8s"
"github.com/aws/aws-application-networking-k8s/pkg/model/core"
Expand All @@ -25,25 +27,30 @@ type ServiceNetworkModelBuilder interface {
}

type serviceNetworkModelBuilder struct {
client client.Client
defaultTags map[string]string
}

func NewServiceNetworkModelBuilder() *serviceNetworkModelBuilder {
return &serviceNetworkModelBuilder{}
func NewServiceNetworkModelBuilder(client client.Client) *serviceNetworkModelBuilder {
return &serviceNetworkModelBuilder{client: client}
}
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{})
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if the policy is not found?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

if err != nil {
return nil, nil, err
}
task := &serviceNetworkModelBuildTask{
gateway: gw,
stack: stack,
gateway: gw,
vpcAssociationPolicy: vpcAssociationPolicy,
stack: stack,
}

if err := task.run(ctx); err != nil {
return nil, nil, corev1.ErrIntOverflowGenerated
}

return task.stack, task.mesh, nil
return task.stack, task.serviceNetwork, nil
}

func (t *serviceNetworkModelBuildTask) run(ctx context.Context) error {
Expand All @@ -63,25 +70,29 @@ func (t *serviceNetworkModelBuildTask) buildModel(ctx context.Context) error {

func (t *serviceNetworkModelBuildTask) buildServiceNetwork(ctx context.Context) error {
spec := latticemodel.ServiceNetworkSpec{
Name: t.gateway.Name,
Namespace: t.gateway.Namespace,
Account: config.AccountID,
AssociateToVPC: false,
Name: t.gateway.Name,
Namespace: t.gateway.Namespace,
Account: config.AccountID,
}

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

Choose a reason for hiding this comment

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

I believe there's no need to check this here

if value, exist := t.gateway.Annotations[LatticeVPCAssociationAnnotation]; exist {
if value == "true" {
spec.AssociateToVPC = true
} else {
spec.AssociateToVPC = false
}
associateToVPC = value == "true"
}
}

if t.vpcAssociationPolicy != nil {
if t.vpcAssociationPolicy.Spec.AssociateWithVpc != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

have you considered setting default value via kubebuilder annotation?

associateToVPC = *t.vpcAssociationPolicy.Spec.AssociateWithVpc
}

if t.vpcAssociationPolicy.Spec.SecurityGroupIds != nil {
spec.SecurityGroupIds = securityGroupIdsToStringPointersSlice(t.vpcAssociationPolicy.Spec.SecurityGroupIds)
Copy link
Contributor

@scottlaiaws scottlaiaws Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
}
spec.AssociateToVPC = associateToVPC
defaultSN, err := config.GetClusterLocalGateway()

if err == nil && defaultSN != t.gateway.Name {
Expand All @@ -95,15 +106,24 @@ func (t *serviceNetworkModelBuildTask) buildServiceNetwork(ctx context.Context)
spec.IsDeleted = false
}

t.mesh = latticemodel.NewServiceNetwork(t.stack, ResourceIDServiceNetwork, spec)
t.serviceNetwork = latticemodel.NewServiceNetwork(t.stack, ResourceIDServiceNetwork, spec)

return nil
}

type serviceNetworkModelBuildTask struct {
gateway *gateway_api.Gateway

mesh *latticemodel.ServiceNetwork
gateway *gateway_api.Gateway
vpcAssociationPolicy *v1alpha1.VpcAssociationPolicy
serviceNetwork *latticemodel.ServiceNetwork

stack core.Stack
}

func securityGroupIdsToStringPointersSlice(sgIds []v1alpha1.SecurityGroupId) []*string {
var ret []*string
for _, sgId := range sgIds {
sgIdStr := string(sgId)
ret = append(ret, &sgIdStr)
}
return ret
}
11 changes: 6 additions & 5 deletions pkg/model/lattice/servicenetwork.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,12 @@ type ServiceNetwork struct {

type ServiceNetworkSpec struct {
// The name of the ServiceNetwork
Name string `json:"name"`
Namespace string `json:"namespace"`
Account string `json:"account"`
AssociateToVPC bool
IsDeleted bool
Name string `json:"name"`
Namespace string `json:"namespace"`
Account string `json:"account"`
SecurityGroupIds []*string `json:"securityGroupIds"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

AssociateToVPC bool
IsDeleted bool
}

type ServiceNetworkStatus struct {
Expand Down