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

t-kikuc · 2023-12-21T06:45:58Z

What this PR does / why we need it:

Modifying listener rules which is not the default rule without specifying in app.pipecd.config, unlike #4726.

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?: no

How are users affected by this change: no
Is this breaking change: no
How to migrate (if breaking change): no

** A short explanation **

If the ELB and app.pipecd.yaml are as below, the rules that will be modified are rule-A1,rule-B1 and rule-B2
since they have the same target groups as app.pipecd.yaml.

ELB-X
|-- Listener-A
|    |-- rule-A1 (default)
|        |-- targetGroup-p
|        |-- targetGroup-c
|-- Listener-B
    |-- rule-B1 (default)
    |    |-- targetGroup-p
    |    |-- targetGroup-c
    |-- rule-B2   [path-pattern: /xxx/*]
    |    |-- targetGroup-p
    |    |-- targetGroup-c
    |-- rule-B3   [path-pattern: /yyy/*]
        |-- targetGroup-x
        |-- targetGroup-y

app.pipecd.yaml:

apiVersion: pipecd.dev/v1beta1
kind: ECSApp
spec:
  input:
    targetGroups:
      primary:
        targetGroupArn: arn:aws:elasticloadbalancing:ap-northeast-1:<account-id>:targetgroup/xxx/yyy1 # targetGroup-p
      canary:
        targetGroupArn: arn:aws:elasticloadbalancing:ap-northeast-1:<account-id>:targetgroup/xxx/yyy2 # targetGroup-c

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

codecov · 2023-12-21T06:50:15Z

Codecov Report

Attention: 41 lines in your changes are missing coverage. Please review.

Comparison is base (847b578) 30.84% compared to head (fd13dff) 30.84%.
Report is 10 commits behind head on master.

Files	Patch %	Lines
pkg/app/piped/platformprovider/ecs/client.go	0.00%	41 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4733      +/-   ##
==========================================
- Coverage   30.84%   30.84%   -0.01%     
==========================================
  Files         221      222       +1     
  Lines       25993    26037      +44     
==========================================
+ Hits         8018     8030      +12     
- Misses      17325    17357      +32     
  Partials      650      650

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

t-kikuc · 2023-12-21T09:19:54Z

/review

github-actions · 2023-12-21T09:20:35Z

PR Analysis

Main theme

ECS Load Balancer Rule Modification

PR summary

The PR changes the approach for modifying ECS load balancer rules by describing ECS rules and subsequently modifying them based on certain conditions. It introduces a check to only modify rules that have the same target groups as defined in the new `RoutingTrafficConfig`.

Type of PR

Enhancement

PR Feedback:

General suggestions

The PR enhances the rule modification logic of ECS load balancers by being more selective in the rules it updates, which seems like a good improvement for accuracy and potentially efficiency. To further improve the code, consider handling possible edge cases and addressing any minor code quality concerns, such as variable declaration placement.

Code feedback

- relevant file: `pkg/app/piped/platformprovider/ecs/client.go`
  suggestion: |
    Consider moving the declaration of `var modifiedActions []elbtypes.Action` outside of the for-loop to avoid re-declaration every loop iteration. Although it is scoped within the for-loop, it's not dependent on the loop iteration and could be reused which may aid in readability.
  relevant line: |
    `+			var modifiedActions []elbtypes.Action`

- relevant file: `pkg/app/piped/platformprovider/ecs/routing_traffic.go`
  suggestion: |
    Ensure that you handle potential null pointer dereferences when working with AWS SDK objects. In the `hasSameTargets` method, add null checks for `forwardActionTargets[i].TargetGroupArn` before dereferencing it to prevent runtime panics. (important)
  relevant line: |
    `+		return *forwardActionTargets[i].TargetGroupArn < *forwardActionTargets[j].TargetGroupArn`

- relevant file: `pkg/app/piped/platformprovider/ecs/routing_traffic.go`
  suggestion: |
    Implement a guard clause in the `hasSameTargets` method for early return if `c` or `forwardActionTargets` are nil to prevent unnecessary sorting or comparison and enhance code clarity. (medium)
  relevant line: |
    `+	if len(c) != len(forwardActionTargets) {`

Security concerns:

no

The PR does not introduce changes that are directly related to common security issues. It primarily deals with load balancer rule modifications and does not involve user input handling, database operations, or other areas typically associated with security vulnerabilities like SQL injection, XSS, or CSRF.

t-kikuc · 2023-12-22T06:28:28Z

@moko-poi @nnnkkk7
Does this PR satisfy your usecases?

khanhtc1202 · 2023-12-25T04:39:24Z

pkg/app/piped/platformprovider/ecs/routing_traffic.go

+	// 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
+	})
+
+	for i := range c {
+		if c[i].TargetGroupArn != *forwardActionTargets[i].TargetGroupArn {
+			return false
+		}
+	}


Instead of sorting both, then iterating through, using an extra map with TargetGroupArn as key could provide a better performance. It's a more straightforward approach.

@khanhtc1202 Thanks! I'll try

@khanhtc1202
I fixed on 727e13c

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

khanhtc1202 · 2023-12-26T10:12:19Z

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

 		})
 		if err != nil {
-			return fmt.Errorf("error describing listener %s: %w", listenerArn, err)
+			return fmt.Errorf("failed to describe rules %s: %w", listenerArn, err)


Suggested change

return fmt.Errorf("failed to describe rules %s: %w", listenerArn, err)

return fmt.Errorf("failed to describe rules of listener %s: %w", listenerArn, err)

khanhtc1202 · 2023-12-26T10:14:12Z

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

-								TargetGroupArn: aws.String(routingTrafficCfg[1].TargetGroupArn),
-								Weight:         aws.Int32(int32(routingTrafficCfg[1].Weight)),
+		for _, rule := range describeRulesOutput.Rules {
+			var modifiedActions []elbtypes.Action


Suggested change

var modifiedActions []elbtypes.Action

modifiedActions := make([]elbtypes.Action, 0, len(rule.Actions))

khanhtc1202 · 2023-12-26T10:16:20Z

LGTM with some nits 👍

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

t-kikuc · 2023-12-26T11:18:06Z

@khanhtc1202
Thank you for reviewing, I fixed two

khanhtc1202

Great improvement 👍

khanhtc1202 · 2023-12-27T01:29:34Z

@ffjlabo plz check 👀👀

ffjlabo · 2023-12-27T01:53:10Z

pkg/app/piped/platformprovider/ecs/routing_traffic_test.go

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


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/

@ffjlabo Thank you so much! I fixed

ffjlabo · 2023-12-27T02:01:35Z

pkg/app/piped/platformprovider/ecs/routing_traffic.go

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


[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

}

}

@ffjlabo Thank you so much! I fixed

@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

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

ffjlabo

LGTM 🚀

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

khanhtc1202

🚀

t-kikuc · 2024-01-31T06:43:24Z

I'll add the below diagrams to the doc later.

Previously, only the default rule was controlled during deployments:
Now piped can also control the rules other than the default:
Besides, piped controls all rules that have the common target groups:

…nfig (pipe-cd#4733) * Add a func of comparing target groups Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Modify ModifyListeners to edit rules other than default Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Modify comments Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Use ModifyLister to modify the default rules Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Use map to compare target groups Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Fix log and use make() Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Refactored to memory efficient Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Fix reference of loop var of testcases Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Modify loop variable name in test Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> --------- Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

t-kikuc requested review from nghialv, khanhtc1202, kentakozuka and ffjlabo as code owners December 21, 2023 06:45

github-actions bot added the area/go label Dec 21, 2023

t-kikuc added 4 commits December 21, 2023 15:46

Add a func of comparing target groups

bed30a4

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Modify ModifyListeners to edit rules other than default

179d1e6

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Modify comments

f510bab

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Use ModifyLister to modify the default rules

8f2ba1e

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

t-kikuc force-pushed the ecs-elb-modify-rules branch from d79472e to 8f2ba1e Compare December 21, 2023 06:46

t-kikuc changed the title ~~Ecs elb modify rules~~ [ECS] Modify ELB listener rules other than defaults without adding config Dec 21, 2023

t-kikuc mentioned this pull request Dec 21, 2023

[ECS] Enable selection of listener rules to modify #4726

Closed

khanhtc1202 reviewed Dec 25, 2023

View reviewed changes

Use map to compare target groups

727e13c

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

khanhtc1202 reviewed Dec 26, 2023

View reviewed changes

Fix log and use make()

fa63127

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

khanhtc1202 previously approved these changes Dec 26, 2023

View reviewed changes

ffjlabo reviewed Dec 27, 2023

View reviewed changes

Refactored to memory efficient

1293ece

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

t-kikuc dismissed khanhtc1202’s stale review via 1293ece December 27, 2023 06:16

Fix reference of loop var of testcases

eacfa11

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

ffjlabo previously approved these changes Dec 27, 2023

View reviewed changes

Modify loop variable name in test

fd13dff

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

t-kikuc dismissed ffjlabo’s stale review via fd13dff December 27, 2023 06:33

t-kikuc requested review from khanhtc1202 and ffjlabo December 27, 2023 06:37

ffjlabo approved these changes Dec 27, 2023

View reviewed changes

khanhtc1202 approved these changes Dec 27, 2023

View reviewed changes

t-kikuc merged commit 8c9876a into pipe-cd:master Dec 27, 2023
11 of 13 checks passed

github-actions bot mentioned this pull request Feb 6, 2024

Release v0.46.0 #4775

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

t-kikuc commented Dec 21, 2023

codecov bot commented Dec 21, 2023 •

edited

Loading

t-kikuc commented Dec 21, 2023

github-actions bot commented Dec 21, 2023

t-kikuc commented Dec 22, 2023

khanhtc1202 Dec 25, 2023

t-kikuc Dec 26, 2023

t-kikuc Dec 26, 2023

khanhtc1202 Dec 26, 2023

khanhtc1202 Dec 26, 2023

khanhtc1202 commented Dec 26, 2023

t-kikuc commented Dec 26, 2023

khanhtc1202 left a comment

khanhtc1202 commented Dec 27, 2023

ffjlabo Dec 27, 2023

t-kikuc Dec 27, 2023

ffjlabo Dec 27, 2023

t-kikuc Dec 27, 2023

t-kikuc Dec 27, 2023

ffjlabo left a comment

khanhtc1202 left a comment

t-kikuc commented Jan 31, 2024

	return fmt.Errorf("failed to describe rules %s: %w", listenerArn, err)
	return fmt.Errorf("failed to describe rules of listener %s: %w", listenerArn, err)

	var modifiedActions []elbtypes.Action
	modifiedActions := make([]elbtypes.Action, 0, len(rule.Actions))

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

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

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

Conversation

t-kikuc commented Dec 21, 2023

codecov bot commented Dec 21, 2023 • edited Loading

Codecov Report

t-kikuc commented Dec 21, 2023

github-actions bot commented Dec 21, 2023

PR Analysis

Main theme

PR summary

Type of PR

PR Feedback:

General suggestions

Code feedback

Security concerns:

t-kikuc commented Dec 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khanhtc1202 commented Dec 26, 2023

t-kikuc commented Dec 26, 2023

khanhtc1202 left a comment

Choose a reason for hiding this comment

khanhtc1202 commented Dec 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffjlabo left a comment

Choose a reason for hiding this comment

khanhtc1202 left a comment

Choose a reason for hiding this comment

t-kikuc commented Jan 31, 2024

codecov bot commented Dec 21, 2023 •

edited

Loading