Skip to content

Commit d549f6f

Browse files
valerymovalerymo
committed
MGDAPI-5690 CRO Redis snapshots tagging logics update
1 parent 5450c62 commit d549f6f

File tree

7 files changed

+245
-10
lines changed

7 files changed

+245
-10
lines changed

‎pkg/providers/aws/aws_interfaces.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ type ElastiCacheAPI interface {
7474
CreateReplicationGroup(ctx context.Context, input *elasticache.CreateReplicationGroupInput, optFns ...func(*elasticache.Options)) (*elasticache.CreateReplicationGroupOutput, error)
7575
CreateCacheSubnetGroup(ctx context.Context, input *elasticache.CreateCacheSubnetGroupInput, optFns ...func(*elasticache.Options)) (*elasticache.CreateCacheSubnetGroupOutput, error)
7676
DeleteReplicationGroup(ctx context.Context, input *elasticache.DeleteReplicationGroupInput, optFns ...func(*elasticache.Options)) (*elasticache.DeleteReplicationGroupOutput, error)
77+
ListTagsForResource(ctx context.Context, input *elasticache.ListTagsForResourceInput, optFns ...func(*elasticache.Options)) (*elasticache.ListTagsForResourceOutput, error)
7778
}
7879

7980
type S3API interface {

‎pkg/providers/aws/aws_mock.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -465,6 +465,11 @@ func (m *mock_ElasticacheClient) CreateReplicationGroup(ctx context.Context, inp
465465
return args.Get(0).(*elasticache.CreateReplicationGroupOutput), args.Error(1)
466466
}
467467

468+
func (m *mock_ElasticacheClient) ListTagsForResource(ctx context.Context, input *elasticache.ListTagsForResourceInput, optFns ...func(*elasticache.Options)) (*elasticache.ListTagsForResourceOutput, error) {
469+
args := m.Called(ctx, input, optFns)
470+
return args.Get(0).(*elasticache.ListTagsForResourceOutput), args.Error(1)
471+
}
472+
468473
// S3 Mock
469474
type mock_S3Client struct {
470475
mock.Mock

‎pkg/providers/aws/aws_wrapper.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,10 @@ func (r *RealElasticacheClient) CreateReplicationGroup(ctx context.Context, inpu
274274
return r.Client.CreateReplicationGroup(ctx, input, optFns...)
275275
}
276276

277+
func (r *RealElasticacheClient) ListTagsForResource(ctx context.Context, input *elasticache.ListTagsForResourceInput, optFns ...func(*elasticache.Options)) (*elasticache.ListTagsForResourceOutput, error) {
278+
return r.Client.ListTagsForResource(ctx, input, optFns...)
279+
}
280+
277281
// ---------- S3 ----------
278282
func NewS3Client(cfg aws.Config) S3API {
279283
return &RealS3Client{

‎pkg/providers/aws/cluster_network_provider_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -538,6 +538,7 @@ type mockElasticacheClient struct {
538538
batchApplyUpdateActionFn func(ctx context.Context, input *elasticache.BatchApplyUpdateActionInput, opts ...func(*elasticache.Options)) (*elasticache.BatchApplyUpdateActionOutput, error)
539539
addTagsToResourceFn func(ctx context.Context, input *elasticache.AddTagsToResourceInput, opts ...func(*elasticache.Options)) (*elasticache.AddTagsToResourceOutput, error)
540540
createReplicationGroupFn func(ctx context.Context, input *elasticache.CreateReplicationGroupInput, opts ...func(*elasticache.Options)) (*elasticache.CreateReplicationGroupOutput, error)
541+
listTagsForResourceFn func(ctx context.Context, input *elasticache.ListTagsForResourceInput, opts ...func(*elasticache.Options)) (*elasticache.ListTagsForResourceOutput, error)
541542
calls struct {
542543
DescribeSnapshots []struct {
543544
In1 *elasticache.DescribeSnapshotsInput

‎pkg/providers/aws/credentials.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ var (
8181
"elasticache:ModifyCacheSubnetGroup",
8282
"elasticache:DeleteCacheSubnetGroup",
8383
"elasticache:ModifyReplicationGroup",
84+
"elasticache:ListTagsForResource",
8485
"rds:DescribeDBInstances",
8586
"rds:CreateDBInstance",
8687
"rds:DeleteDBInstance",

‎pkg/providers/aws/provider_redis.go‎

Lines changed: 52 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -455,20 +455,33 @@ func (p *RedisProvider) TagElasticacheNode(ctx context.Context, elasticacheClien
455455
// need arn in the following format arn:aws:elasticache:us-east-1:1234567890:cluster:my-mem-cluster
456456
arn := fmt.Sprintf("arn:aws:elasticache:%s:%s:cluster:%s", region, *id.Account, *cache.CacheClusterId)
457457

458+
// Get default tags
458459
cacheTags, clusterID, err := p.getDefaultElasticacheTags(ctx, r)
459460
if err != nil {
460461
msg := "Failed to build default tags"
461462
return croType.StatusMessage(msg), errorUtil.Wrapf(err, msg)
462463
}
463464

464-
// add tags
465-
_, err = elasticacheClient.AddTagsToResource(ctx, &elasticache.AddTagsToResourceInput{
466-
ResourceName: aws.String(arn),
467-
Tags: cacheTags,
468-
})
465+
// Filter out already applied tags
466+
filteredTags, err := filterAlreadyAppliedTags(ctx, elasticacheClient, arn, cacheTags)
469467
if err != nil {
470-
msg := "failed to add tags to aws elasticache :"
471-
return croType.StatusMessage(msg), err
468+
msg := "Failed to filter already applied tags"
469+
return croType.StatusMessage(msg), errorUtil.Wrapf(err, msg)
470+
}
471+
472+
// Add only new/changed tags (if any)
473+
if len(filteredTags) > 0 {
474+
_, err = elasticacheClient.AddTagsToResource(ctx, &elasticache.AddTagsToResourceInput{
475+
ResourceName: aws.String(arn),
476+
Tags: filteredTags,
477+
})
478+
if err != nil {
479+
msg := "failed to add tags to aws elasticache :"
480+
return croType.StatusMessage(msg), err
481+
}
482+
logrus.Infof("Successfully applied %d new/updated tags to cluster %s", len(filteredTags), arn)
483+
} else {
484+
logrus.Infof("Redis cluster %s: no tag changes required", arn)
472485
}
473486

474487
// if snapshots exist add tags to them
@@ -478,7 +491,7 @@ func (p *RedisProvider) TagElasticacheNode(ctx context.Context, elasticacheClien
478491

479492
// loop snapshots adding tags per found snapshot
480493
snapshotList, _ := elasticacheClient.DescribeSnapshots(ctx, inputDescribe)
481-
if len(snapshotList.Snapshots) > 0 {
494+
if len(snapshotList.Snapshots) > 0 && (len(filteredTags) > 0) {
482495
metricName := getMetricName(r.Name)
483496
// We need to reset before recreating so that metrics for deleted snapshots are not orphaned
484497
resources.ResetMetric(metricName)
@@ -1247,7 +1260,6 @@ func (p *RedisProvider) applySpecifiedSecurityUpdates(ctx context.Context, elast
12471260
}
12481261
return nil
12491262
}
1250-
12511263
func (p *RedisProvider) applyServiceUpdate(ctx context.Context, elasticacheClient ElastiCacheAPI, replicationgroupid, serviceupdateName *string) error {
12521264
logger := p.Logger.WithField("action", "applyServiceUpdate")
12531265

@@ -1278,3 +1290,34 @@ func validServiceUpdateStates(status string) bool {
12781290
}
12791291
return false
12801292
}
1293+
1294+
// filterAlreadyAppliedTags removes tags from `desired` that already exist (same key and value) on the resource.
1295+
func filterAlreadyAppliedTags(ctx context.Context, client ElastiCacheAPI, resourceARN string, desired []elasticachetypes.Tag) ([]elasticachetypes.Tag, error) {
1296+
// List current tags on the resource
1297+
resp, err := client.ListTagsForResource(ctx, &elasticache.ListTagsForResourceInput{
1298+
ResourceName: aws.String(resourceARN),
1299+
})
1300+
if err != nil {
1301+
// If we can't list tags (permission issue), fall back to applying all tags
1302+
logrus.Warnf("Could not list existing tags for %s: %v. Will attempt to apply all tags (may result in unnecessary API calls for already-applied tags).", resourceARN, err)
1303+
return desired, nil
1304+
}
1305+
1306+
// Build a map of current tags
1307+
currentTags := make(map[string]string)
1308+
for _, tag := range resp.TagList {
1309+
currentTags[*tag.Key] = *tag.Value
1310+
}
1311+
1312+
// Filter out tags that are already applied with same value
1313+
var filtered []elasticachetypes.Tag
1314+
for _, tag := range desired {
1315+
val, exists := currentTags[*tag.Key]
1316+
if !exists || val != *tag.Value {
1317+
// Either tag doesn't exist or value is different — keep it
1318+
filtered = append(filtered, tag)
1319+
}
1320+
}
1321+
1322+
return filtered, nil
1323+
}

‎pkg/providers/aws/provider_redis_test.go‎

Lines changed: 181 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,15 @@ func (m *mockStsClient) GetCallerIdentity(*sts.GetCallerIdentityInput) (*sts.Get
246246
}, nil
247247
}
248248

249+
// mock elasticache ListTagsForResource output
250+
func (m *mockElasticacheClient) ListTagsForResource(ctx context.Context, input *elasticache.ListTagsForResourceInput) (*elasticache.ListTagsForResourceOutput, error) {
251+
if resources.SafeStringDereference(input.ResourceName) == "arn:aws:elasticache:tes:test:cluster:test" {
252+
return &elasticache.ListTagsForResourceOutput{}, nil
253+
} else {
254+
return m.listTagsForResourceFn(ctx, input)
255+
}
256+
}
257+
249258
func buildTestPrometheusRule() *monitoringv1.PrometheusRule {
250259
return &monitoringv1.PrometheusRule{
251260
ObjectMeta: controllerruntime.ObjectMeta{
@@ -2462,6 +2471,7 @@ func TestAWSRedisProvider_TagElasticache(t *testing.T) {
24622471
},
24632472
}, nil)
24642473
mockElasticache.On("AddTagsToResource", mock.Anything, mock.Anything, mock.Anything).Return(&elasticache.AddTagsToResourceOutput{}, nil)
2474+
mockElasticache.On("ListTagsForResource", mock.Anything, mock.Anything, mock.Anything).Return(&elasticache.ListTagsForResourceOutput{}, nil)
24652475
mockElasticache.On("DescribeCacheClusters", mock.Anything, mock.Anything, mock.Anything).Return(&elasticache.DescribeCacheClustersOutput{
24662476
CacheClusters: buildCacheClusterList(nil),
24672477
}, nil)
@@ -2507,6 +2517,7 @@ func TestAWSRedisProvider_TagElasticache(t *testing.T) {
25072517
},
25082518
}, &mockSnapshotNotFoundError{})
25092519
mockElasticache.On("AddTagsToResource", mock.Anything, mock.Anything, mock.Anything).Return((*elasticache.AddTagsToResourceOutput)(nil), nil)
2520+
mockElasticache.On("ListTagsForResource", mock.Anything, mock.Anything, mock.Anything).Return(&elasticache.ListTagsForResourceOutput{}, nil)
25102521
mockElasticache.On("DescribeCacheClusters", mock.Anything, mock.Anything, mock.Anything).Return(&elasticache.DescribeCacheClustersOutput{
25112522
CacheClusters: buildCacheClusterList(nil),
25122523
}, nil)
@@ -2541,7 +2552,9 @@ func TestAWSRedisProvider_TagElasticache(t *testing.T) {
25412552
r: buildTestRedisCR(),
25422553
elastiCacheClient: func() ElastiCacheAPI {
25432554
mockElasticache := new(mock_ElasticacheClient)
2544-
mockElasticache.On("DescribeReplicationGroups", mock.Anything, mock.Anything, mock.Anything).Return(&elasticache.DescribeReplicationGroupsOutput{}, nil)
2555+
mockElasticache.On("DescribeReplicationGroups", mock.Anything, mock.Anything, mock.Anything).Return(&elasticache.DescribeCacheClustersOutput{
2556+
CacheClusters: buildCacheClusterList(nil),
2557+
}, nil)
25452558
mockElasticache.On("DescribeSnapshots", mock.Anything, mock.Anything, mock.Anything).Return(&elasticache.DescribeSnapshotsOutput{
25462559
Snapshots: []elasticachetypes.Snapshot{
25472560
{
@@ -2551,6 +2564,7 @@ func TestAWSRedisProvider_TagElasticache(t *testing.T) {
25512564
}, nil)
25522565
mockElasticache.On("AddTagsToResource", mock.Anything, mock.Anything, mock.Anything).Return((*elasticache.AddTagsToResourceOutput)(nil), nil).Once()
25532566
mockElasticache.On("AddTagsToResource", mock.Anything, mock.Anything, mock.Anything).Return((*elasticache.AddTagsToResourceOutput)(nil), genericAWSError)
2567+
mockElasticache.On("ListTagsForResource", mock.Anything, mock.Anything, mock.Anything).Return(&elasticache.ListTagsForResourceOutput{}, nil)
25542568
mockElasticache.On("DescribeCacheClusters", mock.Anything, mock.Anything, mock.Anything).Return(&elasticache.DescribeCacheClustersOutput{
25552569
CacheClusters: buildCacheClusterList(nil),
25562570
}, nil)
@@ -2597,6 +2611,7 @@ func TestAWSRedisProvider_TagElasticache(t *testing.T) {
25972611
}, errors.New("SnapshotAlreadyExistsFault"))
25982612
mockElasticache.On("AddTagsToResource", mock.Anything, mock.Anything, mock.Anything).Return((*elasticache.AddTagsToResourceOutput)(nil), nil).Once()
25992613
mockElasticache.On("AddTagsToResource", mock.Anything, mock.Anything, mock.Anything).Return((*elasticache.AddTagsToResourceOutput)(nil), errors.New("SnapshotAlreadyExistsFault")).Once()
2614+
mockElasticache.On("ListTagsForResource", mock.Anything, mock.Anything, mock.Anything).Return(&elasticache.ListTagsForResourceOutput{}, nil)
26002615
mockElasticache.On("DescribeCacheClusters", mock.Anything, mock.Anything, mock.Anything).Return(&elasticache.DescribeCacheClustersOutput{
26012616
CacheClusters: buildCacheClusterList(nil),
26022617
}, nil)
@@ -3564,3 +3579,168 @@ func TestRedisProvider_getElasticacheConfig(t *testing.T) {
35643579
})
35653580
}
35663581
}
3582+
3583+
// Test_filterAlreadyAppliedTags was generated with the help of Cursor AI IDE.
3584+
func Test_filterAlreadyAppliedTags(t *testing.T) {
3585+
type args struct {
3586+
ctx context.Context
3587+
client ElastiCacheAPI
3588+
resourceARN string
3589+
desired []elasticachetypes.Tag
3590+
}
3591+
tests := []struct {
3592+
name string
3593+
args args
3594+
want []elasticachetypes.Tag
3595+
wantErr bool
3596+
}{
3597+
{
3598+
name: "fallback case - ListTagsForResource fails, returns all desired tags",
3599+
args: args{
3600+
ctx: context.TODO(),
3601+
client: func() ElastiCacheAPI {
3602+
mockClient := new(mock_ElasticacheClient)
3603+
mockClient.On("ListTagsForResource", mock.Anything, mock.Anything, mock.Anything).Return(
3604+
(*elasticache.ListTagsForResourceOutput)(nil),
3605+
errors.New("API error"),
3606+
)
3607+
return mockClient
3608+
}(),
3609+
resourceARN: "arn:aws:elasticache:us-east-1:123456789012:cluster:test-cluster",
3610+
desired: []elasticachetypes.Tag{
3611+
{Key: aws.String("Environment"), Value: aws.String("test")},
3612+
},
3613+
},
3614+
want: []elasticachetypes.Tag{
3615+
{Key: aws.String("Environment"), Value: aws.String("test")},
3616+
},
3617+
wantErr: false,
3618+
},
3619+
{
3620+
name: "success case - no existing tags, all desired tags should be kept",
3621+
args: args{
3622+
ctx: context.TODO(),
3623+
client: func() ElastiCacheAPI {
3624+
mockClient := new(mock_ElasticacheClient)
3625+
mockClient.On("ListTagsForResource", mock.Anything, mock.Anything, mock.Anything).Return(
3626+
&elasticache.ListTagsForResourceOutput{
3627+
TagList: []elasticachetypes.Tag{}, // No existing tags
3628+
},
3629+
nil,
3630+
)
3631+
return mockClient
3632+
}(),
3633+
resourceARN: "arn:aws:elasticache:us-east-1:123456789012:cluster:test-cluster",
3634+
desired: []elasticachetypes.Tag{
3635+
{Key: aws.String("Environment"), Value: aws.String("test")},
3636+
{Key: aws.String("Team"), Value: aws.String("backend")},
3637+
},
3638+
},
3639+
want: []elasticachetypes.Tag{
3640+
{Key: aws.String("Environment"), Value: aws.String("test")},
3641+
{Key: aws.String("Team"), Value: aws.String("backend")},
3642+
},
3643+
wantErr: false,
3644+
},
3645+
{
3646+
name: "success case - some tags already exist with same values, should be filtered out",
3647+
args: args{
3648+
ctx: context.TODO(),
3649+
client: func() ElastiCacheAPI {
3650+
mockClient := new(mock_ElasticacheClient)
3651+
mockClient.On("ListTagsForResource", mock.Anything, mock.Anything, mock.Anything).Return(
3652+
&elasticache.ListTagsForResourceOutput{
3653+
TagList: []elasticachetypes.Tag{
3654+
{Key: aws.String("Environment"), Value: aws.String("test")},
3655+
{Key: aws.String("Owner"), Value: aws.String("platform")},
3656+
},
3657+
},
3658+
nil,
3659+
)
3660+
return mockClient
3661+
}(),
3662+
resourceARN: "arn:aws:elasticache:us-east-1:123456789012:cluster:test-cluster",
3663+
desired: []elasticachetypes.Tag{
3664+
{Key: aws.String("Environment"), Value: aws.String("test")}, // Same value - should be filtered
3665+
{Key: aws.String("Team"), Value: aws.String("backend")}, // New tag - should be kept
3666+
{Key: aws.String("Owner"), Value: aws.String("engineering")}, // Different value - should be kept
3667+
},
3668+
},
3669+
want: []elasticachetypes.Tag{
3670+
{Key: aws.String("Team"), Value: aws.String("backend")},
3671+
{Key: aws.String("Owner"), Value: aws.String("engineering")},
3672+
},
3673+
wantErr: false,
3674+
},
3675+
{
3676+
name: "success case - all tags already exist with same values, empty result",
3677+
args: args{
3678+
ctx: context.TODO(),
3679+
client: func() ElastiCacheAPI {
3680+
mockClient := new(mock_ElasticacheClient)
3681+
mockClient.On("ListTagsForResource", mock.Anything, mock.Anything, mock.Anything).Return(
3682+
&elasticache.ListTagsForResourceOutput{
3683+
TagList: []elasticachetypes.Tag{
3684+
{Key: aws.String("Environment"), Value: aws.String("test")},
3685+
{Key: aws.String("Team"), Value: aws.String("backend")},
3686+
},
3687+
},
3688+
nil,
3689+
)
3690+
return mockClient
3691+
}(),
3692+
resourceARN: "arn:aws:elasticache:us-east-1:123456789012:cluster:test-cluster",
3693+
desired: []elasticachetypes.Tag{
3694+
{Key: aws.String("Environment"), Value: aws.String("test")},
3695+
{Key: aws.String("Team"), Value: aws.String("backend")},
3696+
},
3697+
},
3698+
want: []elasticachetypes.Tag{},
3699+
wantErr: false,
3700+
},
3701+
{
3702+
name: "success case - existing tags with nil values handled gracefully",
3703+
args: args{
3704+
ctx: context.TODO(),
3705+
client: func() ElastiCacheAPI {
3706+
mockClient := new(mock_ElasticacheClient)
3707+
// Create tags with valid pointers to test the dereferencing
3708+
existingKey := "Environment"
3709+
existingValue := "production"
3710+
mockClient.On("ListTagsForResource", mock.Anything, mock.Anything, mock.Anything).Return(
3711+
&elasticache.ListTagsForResourceOutput{
3712+
TagList: []elasticachetypes.Tag{
3713+
{Key: &existingKey, Value: &existingValue},
3714+
},
3715+
},
3716+
nil,
3717+
)
3718+
return mockClient
3719+
}(),
3720+
resourceARN: "arn:aws:elasticache:us-east-1:123456789012:cluster:test-cluster",
3721+
desired: []elasticachetypes.Tag{
3722+
{Key: aws.String("Environment"), Value: aws.String("test")}, // Different value - should be kept
3723+
},
3724+
},
3725+
want: []elasticachetypes.Tag{
3726+
{Key: aws.String("Environment"), Value: aws.String("test")},
3727+
},
3728+
wantErr: false,
3729+
},
3730+
}
3731+
for _, tt := range tests {
3732+
t.Run(tt.name, func(t *testing.T) {
3733+
got, err := filterAlreadyAppliedTags(tt.args.ctx, tt.args.client, tt.args.resourceARN, tt.args.desired)
3734+
if (err != nil) != tt.wantErr {
3735+
t.Errorf("filterAlreadyAppliedTags() error = %v, wantErr %v", err, tt.wantErr)
3736+
return
3737+
}
3738+
// Handle nil vs empty slice comparison
3739+
if (got == nil && len(tt.want) == 0) || (len(got) == 0 && len(tt.want) == 0) || reflect.DeepEqual(got, tt.want) {
3740+
// Test passes - nil slice and empty slice are equivalent for our purposes
3741+
} else {
3742+
t.Errorf("filterAlreadyAppliedTags() got = %v, want %v", got, tt.want)
3743+
}
3744+
})
3745+
}
3746+
}

0 commit comments

Comments
 (0)