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

feat: sync created Backend SG tags #3990

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: sync created Backend SG tags #3990

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c679bfd
init
phuhung273 Dec 19, 2024
3f312a0
unit test
phuhung273 Dec 20, 2024
597eb70
cleanup
phuhung273 Dec 20, 2024
e175472
improve reconcile tag logic
phuhung273 Jan 14, 2025
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
20 changes: 20 additions & 0 deletions pkg/networking/backend_sg_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,12 @@ func (p *defaultBackendSGProvider) allocateBackendSG(ctx context.Context, resour
if len(sgID) > 1 {
p.logger.V(1).Info("Existing SG found", "id", sgID)
p.autoGeneratedSG = sgID

if err = p.syncBackendSGTags(ctx, sgID); err != nil {
p.logger.Error(err, "failed to synchronize tags of existing securityGroup", sgID)
return err
}
p.logger.Info("added resource tags", "resourceID", sgID)
return nil
}

Expand All @@ -253,6 +259,20 @@ func (p *defaultBackendSGProvider) allocateBackendSG(ctx context.Context, resour
return nil
}

func (p *defaultBackendSGProvider) syncBackendSGTags(ctx context.Context, sgID string) error {
tagSpecifications := p.buildBackendSGTags(ctx)

createReq := &ec2sdk.CreateTagsInput{
Resources: []string{sgID},
Tags: tagSpecifications[0].Tags,
}

if _, err := p.ec2Client.CreateTagsWithContext(ctx, createReq); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please add a check to ensure that this create call will actually do something? Ideally, we shouldn't call the CreateTags API when there is no changes to be made.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review, I found this code with the same reconcilation logic

aws-load-balancer-controller/pkg/deploy/ec2/tagging_manager.go

Line 80 in 2ff2e59

func (m *defaultTaggingManager) ReconcileTags(ctx context.Context, resID string, desiredTags map[string]string, opts ...ReconcileTagsOption) error {

But we cannot import since circular dependency, so I copied here. Do you think we should refactor to remove duplication ?

return err
}
return nil
}

func (p *defaultBackendSGProvider) buildBackendSGTags(_ context.Context) []ec2types.TagSpecification {
var defaultTags []ec2types.Tag
for key, val := range p.defaultTags {
Expand Down
145 changes: 139 additions & 6 deletions pkg/networking/backend_sg_provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,19 @@ func Test_defaultBackendSGProvider_Get(t *testing.T) {
resp *ec2sdk.CreateSecurityGroupOutput
err error
}
type createTagsWithContextCall struct {
req *ec2sdk.CreateTagsInput
resp *ec2sdk.CreateTagsOutput
err error
}
type fields struct {
backendSG string
ingResources []*networking.Ingress
svcResource *corev1.Service
defaultTags map[string]string
describeSGCalls []describeSecurityGroupsAsListCall
createSGCalls []createSecurityGroupWithContexCall
backendSG string
ingResources []*networking.Ingress
svcResource *corev1.Service
defaultTags map[string]string
describeSGCalls []describeSecurityGroupsAsListCall
createSGCalls []createSecurityGroupWithContexCall
createTagsWithContextCalls []createTagsWithContextCall
}
defaultEC2Filters := []ec2types.Filter{
{
Expand Down Expand Up @@ -110,10 +116,134 @@ func Test_defaultBackendSGProvider_Get(t *testing.T) {
},
},
},
createTagsWithContextCalls: []createTagsWithContextCall{
{
req: &ec2sdk.CreateTagsInput{
Resources: []string{"sg-autogen"},
Tags: []ec2types.Tag{
{
Key: awssdk.String("elbv2.k8s.aws/cluster"),
Value: awssdk.String(defaultClusterName),
},
{
Key: awssdk.String("elbv2.k8s.aws/resource"),
Value: awssdk.String("backend-sg"),
},
},
},
},
},
ingResources: []*networking.Ingress{ing, ing1},
},
want: "sg-autogen",
},
{
name: "backend sg enabled, auto-gen, SG exists, try to sync tags",
fields: fields{
describeSGCalls: []describeSecurityGroupsAsListCall{
{
req: &ec2sdk.DescribeSecurityGroupsInput{
Filters: defaultEC2Filters,
},
resp: []ec2types.SecurityGroup{
{
GroupId: awssdk.String("sg-autogen"),
},
},
},
},
createTagsWithContextCalls: []createTagsWithContextCall{
{
req: &ec2sdk.CreateTagsInput{
Resources: []string{"sg-autogen"},
Tags: []ec2types.Tag{
{
Key: awssdk.String("KubernetesCluster"),
Value: awssdk.String(defaultClusterName),
},
{
Key: awssdk.String("defaultTag"),
Value: awssdk.String("specified"),
},
{
Key: awssdk.String("zzzKey"),
Value: awssdk.String("value"),
},
{
Key: awssdk.String("elbv2.k8s.aws/cluster"),
Value: awssdk.String(defaultClusterName),
},
{
Key: awssdk.String("elbv2.k8s.aws/resource"),
Value: awssdk.String("backend-sg"),
},
},
},
},
},
defaultTags: map[string]string{
"zzzKey": "value",
"KubernetesCluster": defaultClusterName,
"defaultTag": "specified",
},
ingResources: []*networking.Ingress{ing, ing1},
},
want: "sg-autogen",
},
{
name: "backend sg enabled, auto-gen, SG exists, tags sync error",
fields: fields{
describeSGCalls: []describeSecurityGroupsAsListCall{
{
req: &ec2sdk.DescribeSecurityGroupsInput{
Filters: defaultEC2Filters,
},
resp: []ec2types.SecurityGroup{
{
GroupId: awssdk.String("sg-autogen"),
},
},
},
},
createTagsWithContextCalls: []createTagsWithContextCall{
{
req: &ec2sdk.CreateTagsInput{
Resources: []string{"sg-autogen"},
Tags: []ec2types.Tag{
{
Key: awssdk.String("KubernetesCluster"),
Value: awssdk.String(defaultClusterName),
},
{
Key: awssdk.String("defaultTag"),
Value: awssdk.String("specified"),
},
{
Key: awssdk.String("zzzKey"),
Value: awssdk.String("value"),
},
{
Key: awssdk.String("elbv2.k8s.aws/cluster"),
Value: awssdk.String(defaultClusterName),
},
{
Key: awssdk.String("elbv2.k8s.aws/resource"),
Value: awssdk.String("backend-sg"),
},
},
},
err: &smithy.GenericAPIError{Code: "Some.Other.Error", Message: "unable to tag security group"},
},
},
defaultTags: map[string]string{
"zzzKey": "value",
"KubernetesCluster": defaultClusterName,
"defaultTag": "specified",
},
svcResource: svc,
},
wantErr: errors.New("api error Some.Other.Error: unable to tag security group"),
},
{
name: "backend sg enabled, auto-gen new SG",
fields: fields{
Expand Down Expand Up @@ -283,6 +413,9 @@ func Test_defaultBackendSGProvider_Get(t *testing.T) {
for _, call := range tt.fields.createSGCalls {
ec2Client.EXPECT().CreateSecurityGroupWithContext(context.Background(), call.req).Return(call.resp, call.err)
}
for _, call := range tt.fields.createTagsWithContextCalls {
ec2Client.EXPECT().CreateTagsWithContext(context.Background(), call.req).Return(call.resp, call.err)
}
k8sClient := mock_client.NewMockClient(ctrl)
sgProvider := NewBackendSGProvider(defaultClusterName, tt.fields.backendSG,
defaultVPCID, ec2Client, k8sClient, tt.fields.defaultTags, logr.New(&log.NullLogSink{}))
Expand Down