From bed30a44a458c9aa0e03c15bf2337011772ff29c Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Thu, 21 Dec 2023 11:32:32 +0900 Subject: [PATCH 1/9] Add a func of comparing target groups Signed-off-by: t-kikuc --- .../platformprovider/ecs/routing_traffic.go | 28 ++++ .../ecs/routing_traffic_test.go | 134 ++++++++++++++++++ 2 files changed, 162 insertions(+) create mode 100644 pkg/app/piped/platformprovider/ecs/routing_traffic_test.go diff --git a/pkg/app/piped/platformprovider/ecs/routing_traffic.go b/pkg/app/piped/platformprovider/ecs/routing_traffic.go index 4c7a94c647..8a84c6577a 100644 --- a/pkg/app/piped/platformprovider/ecs/routing_traffic.go +++ b/pkg/app/piped/platformprovider/ecs/routing_traffic.go @@ -14,9 +14,37 @@ package ecs +import ( + "sort" + + "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2/types" +) + type RoutingTrafficConfig []targetGroupWeight type targetGroupWeight struct { TargetGroupArn string Weight int } + +func (c RoutingTrafficConfig) hasSameTargets(forwardActionTargets []types.TargetGroupTuple) bool { + if len(c) != len(forwardActionTargets) { + return false + } + + // Sort to the both slices have the same target groups. + sort.Slice(c, func(i, j int) bool { + return c[i].TargetGroupArn < c[j].TargetGroupArn + }) + sort.Slice(forwardActionTargets, func(i, j int) bool { + return *forwardActionTargets[i].TargetGroupArn < *forwardActionTargets[j].TargetGroupArn + }) + + for i := range c { + if c[i].TargetGroupArn != *forwardActionTargets[i].TargetGroupArn { + return false + } + } + + return true +} diff --git a/pkg/app/piped/platformprovider/ecs/routing_traffic_test.go b/pkg/app/piped/platformprovider/ecs/routing_traffic_test.go new file mode 100644 index 0000000000..33cbf17491 --- /dev/null +++ b/pkg/app/piped/platformprovider/ecs/routing_traffic_test.go @@ -0,0 +1,134 @@ +// Copyright 2023 The PipeCD Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package ecs + +import ( + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2/types" + "github.com/stretchr/testify/assert" +) + +func TestHasSameTargets(t *testing.T) { + t.Parallel() + + testcases := []struct { + name string + cfg RoutingTrafficConfig + actionTargets []types.TargetGroupTuple + expected bool + }{ + { + name: "has the same target groups in the same order", + cfg: RoutingTrafficConfig{ + { + TargetGroupArn: "arn:aws:elasticloadbalancing:::targetgroup/xxx/yyy1", + Weight: 100, + }, + { + TargetGroupArn: "arn:aws:elasticloadbalancing:::targetgroup/xxx/yyy2", + Weight: 0, + }, + }, + actionTargets: []types.TargetGroupTuple{ + { + TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:::targetgroup/xxx/yyy1"), + Weight: aws.Int32(100), + }, + { + TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:::targetgroup/xxx/yyy2"), + Weight: aws.Int32(0), + }, + }, + expected: true, + }, + { + name: "has the same target groups in the different order", + cfg: RoutingTrafficConfig{ + { + TargetGroupArn: "arn:aws:elasticloadbalancing:::targetgroup/xxx/yyy1", + Weight: 100, + }, + { + TargetGroupArn: "arn:aws:elasticloadbalancing:::targetgroup/xxx/yyy2", + Weight: 0, + }, + }, + actionTargets: []types.TargetGroupTuple{ + { + TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:::targetgroup/xxx/yyy2"), + Weight: aws.Int32(0), + }, + { + TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:::targetgroup/xxx/yyy1"), + Weight: aws.Int32(100), + }, + }, + expected: true, + }, + { + name: "the number of target groups are different", + cfg: RoutingTrafficConfig{ + { + TargetGroupArn: "arn:aws:elasticloadbalancing:::targetgroup/xxx/yyy1", + Weight: 100, + }, + }, + actionTargets: []types.TargetGroupTuple{ + { + TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:::targetgroup/xxx/yyy1"), + Weight: aws.Int32(0), + }, + { + TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:::targetgroup/xxx/yyy2"), + Weight: aws.Int32(100), + }, + }, + expected: false, + }, + { + name: "has a different target group", + cfg: RoutingTrafficConfig{ + { + TargetGroupArn: "arn:aws:elasticloadbalancing:::targetgroup/xxx/yyy1", + Weight: 100, + }, + { + TargetGroupArn: "arn:aws:elasticloadbalancing:::targetgroup/xxx/yyy2", + Weight: 0, + }, + }, + actionTargets: []types.TargetGroupTuple{ + { + TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:::targetgroup/xxx/yyy1"), + Weight: aws.Int32(0), + }, + { + TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:::targetgroup/xxx/yyy3"), + Weight: aws.Int32(100), + }, + }, + expected: false, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + hasSame := tc.cfg.hasSameTargets(tc.actionTargets) + assert.Equal(t, tc.expected, hasSame) + }) + } +} From 179d1e6b37326bf68b49bcce008b6811ab7f45e6 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Thu, 21 Dec 2023 12:24:14 +0900 Subject: [PATCH 2/9] Modify ModifyListeners to edit rules other than default Signed-off-by: t-kikuc --- pkg/app/piped/platformprovider/ecs/client.go | 65 +++++++++---------- .../platformprovider/ecs/routing_traffic.go | 2 +- 2 files changed, 33 insertions(+), 34 deletions(-) diff --git a/pkg/app/piped/platformprovider/ecs/client.go b/pkg/app/piped/platformprovider/ecs/client.go index e466991e1a..748c33b820 100644 --- a/pkg/app/piped/platformprovider/ecs/client.go +++ b/pkg/app/piped/platformprovider/ecs/client.go @@ -438,48 +438,47 @@ func (c *client) ModifyListeners(ctx context.Context, listenerArns []string, rou } for _, listenerArn := range listenerArns { - // Describe the listener to get the current actions - describeListenersOutput, err := c.elbClient.DescribeListeners(ctx, &elasticloadbalancingv2.DescribeListenersInput{ - ListenerArns: []string{listenerArn}, + describeRulesOutput, err := c.elbClient.DescribeRules(ctx, &elasticloadbalancingv2.DescribeRulesInput{ + ListenerArn: aws.String(listenerArn), }) if err != nil { - return fmt.Errorf("error describing listener %s: %w", listenerArn, err) + return fmt.Errorf("error describing rules %s: %w", listenerArn, err) } - // Prepare the actions to be modified - var modifiedActions []elbtypes.Action - for _, action := range describeListenersOutput.Listeners[0].DefaultActions { - if action.Type == elbtypes.ActionTypeEnumForward { - // Modify only the forward action - modifiedAction := elbtypes.Action{ - Type: elbtypes.ActionTypeEnumForward, - ForwardConfig: &elbtypes.ForwardActionConfig{ - TargetGroups: []elbtypes.TargetGroupTuple{ - { - TargetGroupArn: aws.String(routingTrafficCfg[0].TargetGroupArn), - Weight: aws.Int32(int32(routingTrafficCfg[0].Weight)), - }, - { - TargetGroupArn: aws.String(routingTrafficCfg[1].TargetGroupArn), - Weight: aws.Int32(int32(routingTrafficCfg[1].Weight)), + for _, rule := range describeRulesOutput.Rules { + var modifiedActions []elbtypes.Action + for _, action := range rule.Actions { + if action.Type == elbtypes.ActionTypeEnumForward && routingTrafficCfg.hasSameTargets(action.ForwardConfig.TargetGroups) { + // Modify only the forward action which has the same target groups. + modifiedAction := elbtypes.Action{ + Type: elbtypes.ActionTypeEnumForward, + Order: action.Order, + ForwardConfig: &elbtypes.ForwardActionConfig{ + TargetGroups: []elbtypes.TargetGroupTuple{ + { + TargetGroupArn: aws.String(routingTrafficCfg[0].TargetGroupArn), + Weight: aws.Int32(int32(routingTrafficCfg[0].Weight)), + }, + { + TargetGroupArn: aws.String(routingTrafficCfg[1].TargetGroupArn), + Weight: aws.Int32(int32(routingTrafficCfg[1].Weight)), + }, }, }, - }, + } + modifiedActions = append(modifiedActions, modifiedAction) + } else { + modifiedActions = append(modifiedActions, action) } - modifiedActions = append(modifiedActions, modifiedAction) - } else { - // Keep other actions unchanged - modifiedActions = append(modifiedActions, action) } - } - // Modify the listener - _, err = c.elbClient.ModifyListener(ctx, &elasticloadbalancingv2.ModifyListenerInput{ - ListenerArn: aws.String(listenerArn), - DefaultActions: modifiedActions, - }) - if err != nil { - return fmt.Errorf("error modifying listener %s: %w", listenerArn, err) + _, err := c.elbClient.ModifyRule(ctx, &elasticloadbalancingv2.ModifyRuleInput{ + RuleArn: rule.RuleArn, + Actions: modifiedActions, + }) + if err != nil { + return fmt.Errorf("error modifying rule %s: %w", *rule.RuleArn, err) + } } } return nil diff --git a/pkg/app/piped/platformprovider/ecs/routing_traffic.go b/pkg/app/piped/platformprovider/ecs/routing_traffic.go index 8a84c6577a..3ba81fa995 100644 --- a/pkg/app/piped/platformprovider/ecs/routing_traffic.go +++ b/pkg/app/piped/platformprovider/ecs/routing_traffic.go @@ -32,7 +32,7 @@ func (c RoutingTrafficConfig) hasSameTargets(forwardActionTargets []types.Target return false } - // Sort to the both slices have the same target groups. + // Sort so that the both slices have the same target groups. sort.Slice(c, func(i, j int) bool { return c[i].TargetGroupArn < c[j].TargetGroupArn }) From f510babc08c14248a2943fac0b1956630a2c501a Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Thu, 21 Dec 2023 13:50:21 +0900 Subject: [PATCH 3/9] Modify comments Signed-off-by: t-kikuc --- pkg/app/piped/platformprovider/ecs/client.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/app/piped/platformprovider/ecs/client.go b/pkg/app/piped/platformprovider/ecs/client.go index 748c33b820..df1dd1aee3 100644 --- a/pkg/app/piped/platformprovider/ecs/client.go +++ b/pkg/app/piped/platformprovider/ecs/client.go @@ -442,7 +442,7 @@ func (c *client) ModifyListeners(ctx context.Context, listenerArns []string, rou ListenerArn: aws.String(listenerArn), }) if err != nil { - return fmt.Errorf("error describing rules %s: %w", listenerArn, err) + return fmt.Errorf("failed to describe rules %s: %w", listenerArn, err) } for _, rule := range describeRulesOutput.Rules { @@ -477,7 +477,7 @@ func (c *client) ModifyListeners(ctx context.Context, listenerArns []string, rou Actions: modifiedActions, }) if err != nil { - return fmt.Errorf("error modifying rule %s: %w", *rule.RuleArn, err) + return fmt.Errorf("failed to modify rule %s: %w", *rule.RuleArn, err) } } } From 8f2ba1e17dfded9c244675a1a5d233339d8e40ad Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Thu, 21 Dec 2023 15:19:56 +0900 Subject: [PATCH 4/9] Use ModifyLister to modify the default rules Signed-off-by: t-kikuc --- pkg/app/piped/platformprovider/ecs/client.go | 23 +++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/pkg/app/piped/platformprovider/ecs/client.go b/pkg/app/piped/platformprovider/ecs/client.go index df1dd1aee3..34053f5d86 100644 --- a/pkg/app/piped/platformprovider/ecs/client.go +++ b/pkg/app/piped/platformprovider/ecs/client.go @@ -472,12 +472,23 @@ func (c *client) ModifyListeners(ctx context.Context, listenerArns []string, rou } } - _, err := c.elbClient.ModifyRule(ctx, &elasticloadbalancingv2.ModifyRuleInput{ - RuleArn: rule.RuleArn, - Actions: modifiedActions, - }) - if err != nil { - return fmt.Errorf("failed to modify rule %s: %w", *rule.RuleArn, err) + // The default rule needs to be modified by ModifyListener API. + if rule.IsDefault { + _, err := c.elbClient.ModifyListener(ctx, &elasticloadbalancingv2.ModifyListenerInput{ + ListenerArn: &listenerArn, + DefaultActions: modifiedActions, + }) + if err != nil { + return fmt.Errorf("failed to modify default rule %s: %w", *rule.RuleArn, err) + } + } else { + _, err := c.elbClient.ModifyRule(ctx, &elasticloadbalancingv2.ModifyRuleInput{ + RuleArn: rule.RuleArn, + Actions: modifiedActions, + }) + if err != nil { + return fmt.Errorf("failed to modify rule %s: %w", *rule.RuleArn, err) + } } } } From 727e13c6e407793ee28a8bff8d5ec5e11728c482 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Tue, 26 Dec 2023 12:56:46 +0900 Subject: [PATCH 5/9] Use map to compare target groups Signed-off-by: t-kikuc --- .../platformprovider/ecs/routing_traffic.go | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/pkg/app/piped/platformprovider/ecs/routing_traffic.go b/pkg/app/piped/platformprovider/ecs/routing_traffic.go index 3ba81fa995..c089b77b31 100644 --- a/pkg/app/piped/platformprovider/ecs/routing_traffic.go +++ b/pkg/app/piped/platformprovider/ecs/routing_traffic.go @@ -15,8 +15,6 @@ package ecs import ( - "sort" - "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2/types" ) @@ -32,16 +30,13 @@ func (c RoutingTrafficConfig) hasSameTargets(forwardActionTargets []types.Target return false } - // Sort so that the both slices have the same target groups. - sort.Slice(c, func(i, j int) bool { - return c[i].TargetGroupArn < c[j].TargetGroupArn - }) - sort.Slice(forwardActionTargets, func(i, j int) bool { - return *forwardActionTargets[i].TargetGroupArn < *forwardActionTargets[j].TargetGroupArn - }) + cMap := make(map[string]bool) + for _, item := range c { + cMap[item.TargetGroupArn] = true + } - for i := range c { - if c[i].TargetGroupArn != *forwardActionTargets[i].TargetGroupArn { + for _, target := range forwardActionTargets { + if !cMap[*target.TargetGroupArn] { return false } } From fa63127c9924ffea53a149085d4c376e0d857dd8 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Tue, 26 Dec 2023 20:16:40 +0900 Subject: [PATCH 6/9] Fix log and use make() Signed-off-by: t-kikuc --- pkg/app/piped/platformprovider/ecs/client.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/app/piped/platformprovider/ecs/client.go b/pkg/app/piped/platformprovider/ecs/client.go index 34053f5d86..0e4b6c1d1c 100644 --- a/pkg/app/piped/platformprovider/ecs/client.go +++ b/pkg/app/piped/platformprovider/ecs/client.go @@ -442,11 +442,11 @@ func (c *client) ModifyListeners(ctx context.Context, listenerArns []string, rou ListenerArn: aws.String(listenerArn), }) if err != nil { - return fmt.Errorf("failed to describe rules %s: %w", listenerArn, err) + return fmt.Errorf("failed to describe rules of listener %s: %w", listenerArn, err) } for _, rule := range describeRulesOutput.Rules { - var modifiedActions []elbtypes.Action + modifiedActions := make([]elbtypes.Action, 0, len(rule.Actions)) for _, action := range rule.Actions { if action.Type == elbtypes.ActionTypeEnumForward && routingTrafficCfg.hasSameTargets(action.ForwardConfig.TargetGroups) { // Modify only the forward action which has the same target groups. From 1293ece96aa41f33539aab98760215387ce81d84 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 27 Dec 2023 14:52:38 +0900 Subject: [PATCH 7/9] Refactored to memory efficient Signed-off-by: t-kikuc --- pkg/app/piped/platformprovider/ecs/routing_traffic.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/app/piped/platformprovider/ecs/routing_traffic.go b/pkg/app/piped/platformprovider/ecs/routing_traffic.go index c089b77b31..df89fc99c3 100644 --- a/pkg/app/piped/platformprovider/ecs/routing_traffic.go +++ b/pkg/app/piped/platformprovider/ecs/routing_traffic.go @@ -30,13 +30,13 @@ func (c RoutingTrafficConfig) hasSameTargets(forwardActionTargets []types.Target return false } - cMap := make(map[string]bool) + cMap := make(map[string]struct{}) for _, item := range c { - cMap[item.TargetGroupArn] = true + cMap[item.TargetGroupArn] = struct{}{} } for _, target := range forwardActionTargets { - if !cMap[*target.TargetGroupArn] { + if _, ok := cMap[*target.TargetGroupArn]; !ok { return false } } From eacfa11abb812ba7ed951a37f625fc23ff3d29ac Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 27 Dec 2023 15:18:00 +0900 Subject: [PATCH 8/9] Fix reference of loop var of testcases Signed-off-by: t-kikuc --- pkg/app/piped/platformprovider/ecs/routing_traffic_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/app/piped/platformprovider/ecs/routing_traffic_test.go b/pkg/app/piped/platformprovider/ecs/routing_traffic_test.go index 33cbf17491..59cd7bf53b 100644 --- a/pkg/app/piped/platformprovider/ecs/routing_traffic_test.go +++ b/pkg/app/piped/platformprovider/ecs/routing_traffic_test.go @@ -125,7 +125,8 @@ func TestHasSameTargets(t *testing.T) { }, } - for _, tc := range testcases { + for _, testcase := range testcases { + tc := testcase t.Run(tc.name, func(t *testing.T) { hasSame := tc.cfg.hasSameTargets(tc.actionTargets) assert.Equal(t, tc.expected, hasSame) From fd13dffee0997f7256d3845cb0d8d8bb52a2d021 Mon Sep 17 00:00:00 2001 From: t-kikuc Date: Wed, 27 Dec 2023 15:33:09 +0900 Subject: [PATCH 9/9] Modify loop variable name in test Signed-off-by: t-kikuc --- pkg/app/piped/platformprovider/ecs/routing_traffic_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/app/piped/platformprovider/ecs/routing_traffic_test.go b/pkg/app/piped/platformprovider/ecs/routing_traffic_test.go index 59cd7bf53b..ce87da6824 100644 --- a/pkg/app/piped/platformprovider/ecs/routing_traffic_test.go +++ b/pkg/app/piped/platformprovider/ecs/routing_traffic_test.go @@ -125,8 +125,8 @@ func TestHasSameTargets(t *testing.T) { }, } - for _, testcase := range testcases { - tc := testcase + for _, tc := range testcases { + tc := tc t.Run(tc.name, func(t *testing.T) { hasSame := tc.cfg.hasSameTargets(tc.actionTargets) assert.Equal(t, tc.expected, hasSame)