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.

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(ECS): enable selection of listener rules #4540

Closed

Conversation

moko-poi
Copy link
Contributor

@moko-poi moko-poi commented Aug 7, 2023

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #4530

Does this PR introduce a user-facing change?:

  • How are users affected by this change:
apiVersion: pipecd.dev/v1beta1
kind: ECSApp
input:
  taskDefinitionFile: taskdef.yaml
  serviceDefinitionFile: servicedef.yaml
  targetGroups:
    primary:
    canary:
  • Is this breaking change: no
  • How to migrate (if breaking change):

@moko-poi moko-poi changed the title feat: selection of listener rules feat(ECS): enable selection of listener rules Aug 7, 2023
@moko-poi moko-poi marked this pull request as ready for review August 7, 2023 11:45
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

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

Comparison is base (331b323) 30.01% compared to head (cd4f1a7) 29.94%.
Report is 68 commits behind head on master.

❗ Current head cd4f1a7 differs from pull request most recent head e91ab8a. Consider uploading reports for the commit e91ab8a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4540      +/-   ##
==========================================
- Coverage   30.01%   29.94%   -0.07%     
==========================================
  Files         220      221       +1     
  Lines       25761    25893     +132     
==========================================
+ Hits         7731     7754      +23     
- Misses      17385    17492     +107     
- Partials      645      647       +2     
Files Coverage Δ
...g/app/piped/platformprovider/ecs/listener_rules.go 100.00% <100.00%> (ø)
pkg/config/application_ecs.go 9.52% <ø> (ø)
pkg/config/application_kubernetes.go 8.00% <ø> (ø)
pkg/app/piped/platformprovider/ecs/ecs.go 0.00% <0.00%> (ø)
pkg/config/piped.go 56.43% <37.50%> (+0.29%) ⬆️
...g/app/piped/platformprovider/kubernetes/kubectl.go 0.00% <0.00%> (ø)
...g/app/piped/platformprovider/kubernetes/applier.go 0.00% <0.00%> (ø)
pkg/app/piped/platformprovider/ecs/client.go 0.00% <0.00%> (ø)

... and 4 files with indirect coverage changes

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

Copy link
Member

@kentakozuka kentakozuka left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!
Can you update docs as well?

in.LogPersister.Errorf("Failed to routing traffic to PRIMARY/CANARY variants: %v", err)
return false
if len(listenerRules) == 0 {
if err := client.ModifyListeners(ctx, currListenerArns, routingTrafficCfg); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Move these lines inside this block because only this line references currListenerArns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review, @kentakozuka!

I've made the suggested changes and moved the lines inside the block as per your feedback.
bb1b2f3

I've also updated the documentation to reflect the changes made in this PR.
9a2a9c0

Please let me know if there are any other points I should address.

Copy link
Member

@kentakozuka kentakozuka left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM 🎉
@khanhtc1202 Can you review for approval?

@moko-poi
Copy link
Contributor Author

@kentakozuka @khanhtc1202
The CI has failed due to codecov/patch.

The coverage drop is due to missing test files. Adding tests might be beyond this PR's scope.
How should we address this?

Best regards,

@khanhtc1202
Copy link
Member

@moko-poi Don't worry about that. We can approve and ignore that 👌

if err := client.ModifyListeners(ctx, currListenerArns, routingTrafficCfg); err != nil {
in.LogPersister.Errorf("Failed to routing traffic to PRIMARY/CANARY variants: %v", err)
return false
if len(listenerRules) == 0 {
Copy link
Member

@khanhtc1202 khanhtc1202 Aug 10, 2023

Choose a reason for hiding this comment

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

@moko-poi let's make this easier to follow by removing the if/else condition here and return early (as pipecd code convention)
I think we can make it

if len(listenerRules) != 0 {
   // Do modify listener rules
   ...
   return true
}

  // Do modify listener
  ...
  return true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@khanhtc1202
Thank you for the feedback.

I agree that following the pipecd code convention and using early returns will make the code clearer.
I refactored the if/else condition as you suggested and updated the PR.
29554b1

@khanhtc1202
Copy link
Member

khanhtc1202 commented Aug 11, 2023

@moko-poi Thanks so much for your contribution 🙏 There are 2 points that concern me in this current implementation.

First, not sure why we need to nest the configuration as listernerRules.rules, listernerRules directly contains rules make more sense to me.

apiVersion: pipecd.dev/v1beta1
kind: ECSApp
input:
  taskDefinitionFile: taskdef.yaml
  serviceDefinitionFile: servicedef.yaml
  targetGroups:
    primary:
    canary:
  listenerRules:
    - rule1
    - rule2

Second, as far as I understand, the current implementation (ref: code), piped can perform routing via

  • Updating rule (which forwarding canary/primary target group)
  • Updating listener (currently, clear all rules of the listener and only forwarding rule remained)

We currently don't have any way to validate the rules (set by users) are the rules which should be used to perform routing or not. That could lead to unexpected behavior if users miss setting the wrong rules to be used. Across the issue requirement, how about to keep finding current listeners of the given primary workload, then describe the listeners and find the forwarding rule to update, other rules will be remained and be included in the ModifyListener input?

We can use this API to describe specified listeners: https://docs.aws.amazon.com/cli/latest/reference/elbv2/describe-listeners.html

@moko-poi
Copy link
Contributor Author

moko-poi commented Aug 18, 2023

@khanhtc1202

Thank you for the feedback! I truly appreciate your insights.

Regarding the nesting of listenerRules.rules:You're absolutely right; having listenerRules directly contain the rules seems more intuitive.

I have now made the necessary modifications based on your suggestions. Please take a look at the updated implementation and let me know if there are any further changes or adjustments needed.
36440ca

In addition to the previous changes, I've also addressed the validation of provided listener rules against the existing rules. I'd appreciate it if you could review this new implementation as well. Thank you!
cd4f1a7

@moko-poi moko-poi force-pushed the feat/selection-listener-rule-for-ecs branch from 394adbf to cd4f1a7 Compare August 18, 2023 05:54
@moko-poi
Copy link
Contributor Author

@kentakozuka @khanhtc1202
Thank you for the Public MTG the other day.

Based on the feedback received, I've decided not to add a new feature but to upgrade the existing functionality.

When you have a moment, I'd appreciate it if you could review the changes.

@khanhtc1202
Copy link
Member

@moko-poi Could you update the code to pass tests 👀

func (c *client) GetListenerRuleArns(ctx context.Context, listenerArns []string) ([]string, error) {
var ruleArns []string

// 各リスナーのルールを取得
Copy link
Member

Choose a reason for hiding this comment

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

Please use comment in English

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@khanhtc1202
Thank you for the feedback.
I'll make sure to use English in the comments moving forward.
d799ab0

@moko-poi
Copy link
Contributor Author

moko-poi commented Nov 1, 2023

@khanhtc1202
I've made the necessary updates to the code. Please take a look and let me know if there are any further changes needed. Thanks for the feedback!

Comment on lines +470 to +480
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)),
},
Copy link
Member

Choose a reason for hiding this comment

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

Will this always change a rule to ForwardAction type, which forwards traffic to primary/canary workload? if I recall correctly, we want to only modify the forward traffic actions, and keep "default forward 404" like action, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review.

The code is designed to modify only the rules with 'forward' action types; all other rules will remain unchanged.

e91ab8a

Copy link
Contributor Author

Choose a reason for hiding this comment

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

khanhtc1202 and others added 7 commits November 3, 2023 11:44
…sn't exist (pipe-cd#4504)

* feat: add spec input.AutoCreateNamespace for creating a ns when doesn't exist

Signed-off-by: hungran <[email protected]>

* not return error when namespace already exists

Signed-off-by: hungran <[email protected]>
Signed-off-by: hungran <[email protected]>

* not return err

Signed-off-by: hungran <[email protected]>

* WIP
Signed-off-by: hungran <[email protected]>

Signed-off-by: hungran <[email protected]>

* omitempty AutoCreateNamespace, add test case

Signed-off-by: hungran <[email protected]>

* test data k8s-app-helm.yaml

Signed-off-by: hungran <[email protected]>

* Redirect /docs/ to /docs-{latest}/ (pipe-cd#4495)

* Redirect /docs/ to /docs-{latest}/

Signed-off-by: Kenta Kozuka <[email protected]>

* Fix use local path

Signed-off-by: Kenta Kozuka <[email protected]>

---------

Signed-off-by: Kenta Kozuka <[email protected]>
Signed-off-by: hungran <[email protected]>

* Fix documentation link is not displayed (pipe-cd#4505)

Signed-off-by: hungran <[email protected]>

* Update docs/content/en/docs-dev/user-guide/configuration-reference.md

Co-authored-by: Khanh Tran <[email protected]>
Signed-off-by: hungran <[email protected]>

* Update pkg/config/application_kubernetes.go

Co-authored-by: Khanh Tran <[email protected]>
Signed-off-by: hungran <[email protected]>

* Update pkg/config/application_kubernetes_test.go

Co-authored-by: Khanh Tran <[email protected]>
Signed-off-by: hungran <[email protected]>

* Update pkg/config/application_test.go

Co-authored-by: Khanh Tran <[email protected]>
Signed-off-by: hungran <[email protected]>

* Bump word-wrap from 1.2.3 to 1.2.4 in /web (pipe-cd#4511)

Bumps [word-wrap](https://github.com/jonschlinkert/word-wrap) from 1.2.3 to 1.2.4.
- [Release notes](https://github.com/jonschlinkert/word-wrap/releases)
- [Commits](jonschlinkert/word-wrap@1.2.3...1.2.4)

---
updated-dependencies:
- dependency-name: word-wrap
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: hungran <[email protected]>

* Setup codecov (pipe-cd#4509)

Signed-off-by: khanhtc1202 <[email protected]>
Signed-off-by: hungran <[email protected]>

* add: caution note to user group doc (pipe-cd#4512)

Signed-off-by: seitarof <[email protected]>
Co-authored-by: Khanh Tran <[email protected]>
Signed-off-by: hungran <[email protected]>

* Update go test command (pipe-cd#4514)

* Update go test command

Signed-off-by: khanhtc1202 <[email protected]>

* Update makefile and github action command

Signed-off-by: khanhtc1202 <[email protected]>

* Revert gitignore

Signed-off-by: khanhtc1202 <[email protected]>

---------

Signed-off-by: khanhtc1202 <[email protected]>
Signed-off-by: hungran <[email protected]>

* Fix make release/docs command regenerate unused docs directory (pipe-cd#4513)

Signed-off-by: khanhtc1202 <[email protected]>
Signed-off-by: hungran <[email protected]>

* Update auth docs (pipe-cd#4517)

Signed-off-by: khanhtc1202 <[email protected]>
Signed-off-by: hungran <[email protected]>

* Add Google Tag Manager to the site (pipe-cd#4519)

* Add Google Tag Manager to the site

Signed-off-by: Kenta Kozuka <[email protected]>

* Add breakline

Signed-off-by: Kenta Kozuka <[email protected]>

* Add breakline

Signed-off-by: Kenta Kozuka <[email protected]>

---------

Signed-off-by: Kenta Kozuka <[email protected]>
Signed-off-by: hungran <[email protected]>

* Update pkg/config/application_kubernetes_test.go

Signed-off-by: hungran <[email protected]>

* Update pkg/config/application_test.go

Signed-off-by: hungran <[email protected]>

* Update pkg/config/application_test.go

Signed-off-by: hungran <[email protected]>

* Update pkg/config/application_test.go

Signed-off-by: hungran <[email protected]>

* Update pkg/config/application_test.go

Signed-off-by: hungran <[email protected]>

* Update pkg/config/application_test.go

Signed-off-by: hungran <[email protected]>

* get value from var not from pointer

Signed-off-by: hungran <[email protected]>

* handle error when namespace already exsist and ignore it

Signed-off-by: hungran <[email protected]>

* rename handle err, remove unessacarry value test

Signed-off-by: hungran <[email protected]>

* if true

Signed-off-by: hungran <[email protected]>

* fix test data

Signed-off-by: hungran <[email protected]>

* fix correct the test case

Signed-off-by: hungran <[email protected]>

* remove unuse value in k8s-app-helm

Signed-off-by: hungran <[email protected]>

---------

Signed-off-by: hungran <[email protected]>
Signed-off-by: hungran <[email protected]>
Signed-off-by: Kenta Kozuka <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: khanhtc1202 <[email protected]>
Signed-off-by: seitarof <[email protected]>
Co-authored-by: Kenta Kozuka <[email protected]>
Co-authored-by: Khanh Tran <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Seitaro Fujigaki <[email protected]>
Signed-off-by: moko-poi <[email protected]>
…dogConfig directly instead of reading files (pipe-cd#4518)

* fix: optional setting apiKey and applicationKey for AnalysisProviderDatadogConfig directly

Signed-off-by: hungran <[email protected]>

* validate and error log

Signed-off-by: hungran <[email protected]>

* add value test data

Signed-off-by: hungran <[email protected]>

* add validate, base64decode for apiKeyData & applicationKeyData

Signed-off-by: hungran <[email protected]>

---------

Signed-off-by: hungran <[email protected]>
Signed-off-by: moko-poi <[email protected]>
* Add docs for PipeCD Terraform provider

Signed-off-by: Kenta Kozuka <[email protected]>

* Fix page order

Signed-off-by: Kenta Kozuka <[email protected]>

---------

Signed-off-by: Kenta Kozuka <[email protected]>
Signed-off-by: moko-poi <[email protected]>
* Add blog entry of terraform-module-pipecd

Signed-off-by: Kenta Kozuka <[email protected]>

* Fix path and sentence

Signed-off-by: Kenta Kozuka <[email protected]>

* Update docs/content/en/blog/terraform-module-pipecd.md

Co-authored-by: Khanh Tran <[email protected]>
Signed-off-by: Kenta Kozuka <[email protected]>

* Update docs/content/en/blog/terraform-module-pipecd.md

Co-authored-by: Khanh Tran <[email protected]>
Signed-off-by: Kenta Kozuka <[email protected]>

* Update docs/content/en/blog/terraform-module-pipecd.md

Co-authored-by: Khanh Tran <[email protected]>
Signed-off-by: Kenta Kozuka <[email protected]>

* Update docs/content/en/blog/terraform-module-pipecd.md

Co-authored-by: Khanh Tran <[email protected]>
Signed-off-by: Kenta Kozuka <[email protected]>

---------

Signed-off-by: Kenta Kozuka <[email protected]>
Co-authored-by: Khanh Tran <[email protected]>
Signed-off-by: moko-poi <[email protected]>
)

This reverts commit 824f9c0.

Signed-off-by: Kenta Kozuka <[email protected]>
Signed-off-by: moko-poi <[email protected]>
ffjlabo and others added 20 commits November 3, 2023 11:44
Signed-off-by: Yoshiki Fujikane <[email protected]>
Signed-off-by: moko-poi <[email protected]>
* Support singleton ECS task

Signed-off-by: khanhtc1202 <[email protected]>

* Rename singleton to recreate

Signed-off-by: khanhtc1202 <[email protected]>

---------

Signed-off-by: khanhtc1202 <[email protected]>
Signed-off-by: moko-poi <[email protected]>
Bumps [golang.org/x/net](https://github.com/golang/net) from 0.8.0 to 0.17.0.
- [Commits](golang/net@v0.8.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: moko-poi <[email protected]>
pipe-cd#4612)

Bumps [golang.org/x/net](https://github.com/golang/net) from 0.7.0 to 0.17.0.
- [Commits](golang/net@v0.7.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Khanh Tran <[email protected]>
Signed-off-by: moko-poi <[email protected]>
…iew (pipe-cd#4611)

Bumps [golang.org/x/net](https://github.com/golang/net) from 0.7.0 to 0.17.0.
- [Commits](golang/net@v0.7.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: moko-poi <[email protected]>
)

* Fix lint/go errors (misspell)

Signed-off-by: karamaru-alpha <[email protected]>

* Fix lint/go errors (depguard)

Signed-off-by: karamaru-alpha <[email protected]>

* Fix lint/go errors (unconvert)

Signed-off-by: karamaru-alpha <[email protected]>

---------

Signed-off-by: karamaru-alpha <[email protected]>
Signed-off-by: moko-poi <[email protected]>
…d#4622)

* Fix lint/go errors (goimport)

Signed-off-by: karamaru-alpha <[email protected]>

* Fix lint/go errors (staticcheck)

Signed-off-by: karamaru-alpha <[email protected]>

* Fix lint/go errors (gosimple)

Signed-off-by: karamaru-alpha <[email protected]>

* Fix lint/go errors (gosimple)

Signed-off-by: karamaru-alpha <[email protected]>

* Fix return error by function

Signed-off-by: karamaru-alpha <[email protected]>

---------

Signed-off-by: karamaru-alpha <[email protected]>
Signed-off-by: moko-poi <[email protected]>
Signed-off-by: Kenta Kozuka <[email protected]>
Signed-off-by: moko-poi <[email protected]>
…#4624)

* Ignore mannwhitney from golangci-lint

Signed-off-by: karamaru-alpha <[email protected]>

* Fix lint/go errors (deadcode)

Signed-off-by: karamaru-alpha <[email protected]>

* Fix lint/go errors (depguard)

Signed-off-by: karamaru-alpha <[email protected]>

---------

Signed-off-by: karamaru-alpha <[email protected]>
Signed-off-by: moko-poi <[email protected]>
…t variable (pipe-cd#4628)

* Add GOCACHE env to lint/go

Signed-off-by: karamaru-alpha <[email protected]>

* Add verbose flag to lint/go

Signed-off-by: karamaru-alpha <[email protected]>

* Change timeout golangci.yml

Signed-off-by: karamaru-alpha <[email protected]>

---------

Signed-off-by: karamaru-alpha <[email protected]>
Signed-off-by: moko-poi <[email protected]>
Signed-off-by: Kenta Kozuka <[email protected]>
Signed-off-by: moko-poi <[email protected]>
Signed-off-by: Kenta Kozuka <[email protected]>
Signed-off-by: moko-poi <[email protected]>
Signed-off-by: Kenta Kozuka <[email protected]>
Signed-off-by: moko-poi <[email protected]>
* Bump google.golang.org/grpc from 1.54.0 to 1.56.3

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.54.0 to 1.56.3.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.54.0...v1.56.3)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>

* Update grpc test version

Signed-off-by: khanhtc1202 <[email protected]>

---------

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: khanhtc1202 <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: khanhtc1202 <[email protected]>
Signed-off-by: moko-poi <[email protected]>
* Fix lint/go errors (stylecheck)

Signed-off-by: karamaru-alpha <[email protected]>

* Remove nolint:stylecheck

Signed-off-by: karamaru-alpha <[email protected]>

* Change method reveiver name

Signed-off-by: karamaru-alpha <[email protected]>

* Change receiver name k to ak

Signed-off-by: karamaru-alpha <[email protected]>

---------

Signed-off-by: karamaru-alpha <[email protected]>
Signed-off-by: moko-poi <[email protected]>
Signed-off-by: moko-poi <[email protected]>
@khanhtc1202
Copy link
Member

Sorry for the late @moko-poi
The PR is containing changes that not related, please update so that we can merge 🙏

@moko-poi
Copy link
Contributor Author

@kentakozuka @khanhtc1202
Closing this PR due to conflicts and delays. A new, updated PR has been created here: #4668. Please refer to the new PR for further discussion.

I apologize for any inconvenience and invite you to join the discussion on the new PR. Thank you for your understanding and collaboration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support enable selection of listener rules