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

Jump to bottom

[ECS] Modify ELB listener rules other than defaults without adding config #4733

Merged
merged 9 commits into from
Dec 27, 2023
Merged

[ECS] Modify ELB listener rules other than defaults without adding config #4733

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bed30a4
Add a func of comparing target groups
t-kikuc Dec 21, 2023
179d1e6
Modify ModifyListeners to edit rules other than default
t-kikuc Dec 21, 2023
f510bab
Modify comments
t-kikuc Dec 21, 2023
8f2ba1e
Use ModifyLister to modify the default rules
t-kikuc Dec 21, 2023
727e13c
Use map to compare target groups
t-kikuc Dec 26, 2023
fa63127
Fix log and use make()
t-kikuc Dec 26, 2023
1293ece
Refactored to memory efficient
t-kikuc Dec 27, 2023
eacfa11
Fix reference of loop var of testcases
t-kikuc Dec 27, 2023
fd13dff
Modify loop variable name in test
t-kikuc Dec 27, 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
76 changes: 43 additions & 33 deletions pkg/app/piped/platformprovider/ecs/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,48 +438,58 @@
}

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),

Check warning on line 442 in pkg/app/piped/platformprovider/ecs/client.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/piped/platformprovider/ecs/client.go#L441-L442 

Added lines #L441 - L442 were not covered by tests
})
if err != nil {
return fmt.Errorf("error describing listener %s: %w", listenerArn, err)
return fmt.Errorf("failed to describe rules of listener %s: %w", listenerArn, err)

Check warning on line 445 in pkg/app/piped/platformprovider/ecs/client.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/piped/platformprovider/ecs/client.go#L445 

Added line #L445 was not covered by tests
}

// 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 {
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.
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)),
},

Check warning on line 465 in pkg/app/piped/platformprovider/ecs/client.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/piped/platformprovider/ecs/client.go#L448-L465 

Added lines #L448 - L465 were not covered by tests
},
},
},
}
modifiedActions = append(modifiedActions, modifiedAction)
} else {
modifiedActions = append(modifiedActions, action)

Check warning on line 471 in pkg/app/piped/platformprovider/ecs/client.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/piped/platformprovider/ecs/client.go#L468-L471 

Added lines #L468 - L471 were not covered by tests
}
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)
// 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)
}

Check warning on line 491 in pkg/app/piped/platformprovider/ecs/client.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/piped/platformprovider/ecs/client.go#L476-L491 

Added lines #L476 - L491 were not covered by tests
}
}
}
return nil
Expand Down
23 changes: 23 additions & 0 deletions pkg/app/piped/platformprovider/ecs/routing_traffic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,32 @@

package ecs

import (
"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
}

cMap := make(map[string]bool)
for _, item := range c {
cMap[item.TargetGroupArn] = true
}

for _, target := range forwardActionTargets {
if !cMap[*target.TargetGroupArn] {
return false
}
}
Copy link
Member

Choose a reason for hiding this comment

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

[nit] It would be nice to use struct{} empty struct :)
This is more efficient because it does not allocate memory.

Suggested change
cMap := make(map[string]bool)
for _, item := range c {
cMap[item.TargetGroupArn] = true
}
for _, target := range forwardActionTargets {
if !cMap[*target.TargetGroupArn] {
return false
}
}
cMap := make(map[string]struct{})
for _, item := range c {
cMap[item.TargetGroupArn] = struct{}{}
}
for _, target := range forwardActionTargets {
if _, ok := cMap[*target.TargetGroupArn]; !ok {
return false
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@ffjlabo Thank you so much! I fixed

Copy link
Member Author

Choose a reason for hiding this comment

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

@ffjlabo
I fixed again to use tc:=tc as you suggested instead of tc:=testcase
because I found an article that recommends to use the same name.

https://go.dev/wiki/CommonMistakes


return true
}
134 changes: 134 additions & 0 deletions pkg/app/piped/platformprovider/ecs/routing_traffic_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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:<region>:<account-id>:targetgroup/xxx/yyy1",
Weight: 100,
},
{
TargetGroupArn: "arn:aws:elasticloadbalancing:<region>:<account-id>:targetgroup/xxx/yyy2",
Weight: 0,
},
},
actionTargets: []types.TargetGroupTuple{
{
TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:<region>:<account-id>:targetgroup/xxx/yyy1"),
Weight: aws.Int32(100),
},
{
TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:<region>:<account-id>: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:<region>:<account-id>:targetgroup/xxx/yyy1",
Weight: 100,
},
{
TargetGroupArn: "arn:aws:elasticloadbalancing:<region>:<account-id>:targetgroup/xxx/yyy2",
Weight: 0,
},
},
actionTargets: []types.TargetGroupTuple{
{
TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:<region>:<account-id>:targetgroup/xxx/yyy2"),
Weight: aws.Int32(0),
},
{
TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:<region>:<account-id>:targetgroup/xxx/yyy1"),
Weight: aws.Int32(100),
},
},
expected: true,
},
{
name: "the number of target groups are different",
cfg: RoutingTrafficConfig{
{
TargetGroupArn: "arn:aws:elasticloadbalancing:<region>:<account-id>:targetgroup/xxx/yyy1",
Weight: 100,
},
},
actionTargets: []types.TargetGroupTuple{
{
TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:<region>:<account-id>:targetgroup/xxx/yyy1"),
Weight: aws.Int32(0),
},
{
TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:<region>:<account-id>:targetgroup/xxx/yyy2"),
Weight: aws.Int32(100),
},
},
expected: false,
},
{
name: "has a different target group",
cfg: RoutingTrafficConfig{
{
TargetGroupArn: "arn:aws:elasticloadbalancing:<region>:<account-id>:targetgroup/xxx/yyy1",
Weight: 100,
},
{
TargetGroupArn: "arn:aws:elasticloadbalancing:<region>:<account-id>:targetgroup/xxx/yyy2",
Weight: 0,
},
},
actionTargets: []types.TargetGroupTuple{
{
TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:<region>:<account-id>:targetgroup/xxx/yyy1"),
Weight: aws.Int32(0),
},
{
TargetGroupArn: aws.String("arn:aws:elasticloadbalancing:<region>:<account-id>:targetgroup/xxx/yyy3"),
Weight: aws.Int32(100),
},
},
expected: false,
},
}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

tc := tc is needed 👀
ref: https://www.tddfellow.com/blog/2016/01/08/why-do-you-need-to-be-careful-with-loop-variable-in-go/

Copy link
Member Author

Choose a reason for hiding this comment

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

@ffjlabo Thank you so much! I fixed

hasSame := tc.cfg.hasSameTargets(tc.actionTargets)
assert.Equal(t, tc.expected, hasSame)
})
}
}
Loading