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

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

Closed
wants to merge 8 commits into from

Conversation

t-kikuc
Copy link
Member

@t-kikuc t-kikuc commented Dec 17, 2023

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #4725

Does this PR introduce a user-facing change?: Add an option in ECSApp's input.

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

@t-kikuc
Copy link
Member Author

t-kikuc commented Dec 17, 2023

This PR is similar to #4668 , but adds ListenerRuleSelector to ECSDeploymentInput

Copy link

codecov bot commented Dec 17, 2023

Codecov Report

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

Comparison is base (847b578) 30.84% compared to head (acbead1) 30.77%.
Report is 6 commits behind head on master.

Files Patch % Lines
pkg/app/piped/platformprovider/ecs/client.go 0.00% 44 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4726      +/-   ##
==========================================
- Coverage   30.84%   30.77%   -0.08%     
==========================================
  Files         221      221              
  Lines       25993    26040      +47     
==========================================
- Hits         8018     8013       -5     
- Misses      17325    17376      +51     
- Partials      650      651       +1     

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

@t-kikuc
Copy link
Member Author

t-kikuc commented Dec 17, 2023

How to use this feature?:

Specify listenerRuleSelector.listenerRuleArns in app.pipecd.yaml

apiVersion: pipecd.dev/v1beta1
kind: ECSApp
spec:
  input:
    serviceDefinitionFile: /path/to/servicedef.yaml
    taskDefinitionFile: /path/to/taskdef.yaml
    targetGroups:
      primary:
        targetGroupArn: arn:aws:elasticloadbalancing:xyz
        containerName: web
        containerPort: 80
    listenerRuleSelector:  # HERE.   If not specified, the default rule will be changed.
      listenerRuleArns:  # Which rules to modify.
        - arn:aws:elasticloadbalancing:ap-northeast-1:<account-id>:listener-rule/app/<elb-name>/xxx/yyy/zzz
        - arn:aws:elasticloadbalancing:ap-northeast-1:<account-id>:listener-rule/app/<elb-name>/xxx/yyy/zzz2

You can find the listener rule ARNs by CLI aws elbv2 describe-rules ... or the ARN button in the Console.
image

@khanhtc1202
Copy link
Member

/review

Copy link
Contributor

## PR Analysis
  ### Main theme
    ECS Traffic Routing Improvements
  ### PR summary
    This PR introduces the ability to modify specific listener rules for ECS services during deployment and rollback processes instead of modifying the default listener rule. It also includes adjustments to the existing ECS client and configuration models to support this new functionality.
  ### Type of PR
    Enhancement

## PR Feedback:
  ### General suggestions
    The PR adds a meaningful enhancement allowing for fine-grained control over the load balancer's listener rules, enabling users to modify specific rules identified by ARNs. This capability will be particularly beneficial in complex production environments where multiple listener rules are configured for a service. The changes are encapsulated well within the ECS execution logic and utilize the existing ECS client interface effectively.
  
  ### Code feedback
    - relevant file: `pkg/app/piped/executor/ecs/deploy.go`
      suggestion: |
        To improve readability, consider extracting the logic for modifying listener rules or listeners (based on whether the listenerRuleSelector is specified) into a separate function. This refactoring will help reduce the complexity of the `routing` function and centralize the conditional logic.

        ```
        func modifyRouting(ctx context.Context, client elbClient, listenerRuleSelector ELBListenerRuleSelector, routingTrafficCfg RoutingTrafficConfig) error {
            if listenerRuleSelector.IsSpecified() {
                // current logic for modifying rules based on listenerRuleSelector
            } else {
                // current logic for modifying default listeners
            }
        }
        ``` 
      relevant line: `+	if !routing(ctx, &e.Input, e.platformProviderName, e.platformProviderCfg, *primary, *canary, e.appCfg.Input.ListenerRuleSelector) {`

    - relevant file: `pkg/app/piped/executor/ecs/ecs.go`
      suggestion: |
        To prevent potential future errors and to follow best practices, validate the index before accessing the first element of any slices like `describeRulesOutput.Rules`. This will prevent potential index-out-of-range panics.
        ```
        if len(describeRulesOutput.Rules) == 0 {
            return fmt.Errorf("no rules found for ARN %s", ruleArn)
        }
        ``` 
      relevant line: `+		for _, action := range describeRulesOutput.Rules[0].Actions {`

    - relevant file: `pkg/app/piped/executor/ecs/rollback.go`
      suggestion: |
        Similarly to the suggestion for `deploy.go`, consider refactoring the rollback routine to extract the listener rules modification into a separate function. This change would make the `rollback` function more concise and focused on its primary responsibility.
      relevant line: `+	if !rollback(ctx, &e.Input, platformProviderName, platformProviderCfg, taskDefinition, serviceDefinition, primary, canary, appCfg.Input.ListenerRuleSelector) {`

    - relevant file: `pkg/app/piped/platformprovider/ecs/client.go`
      suggestion: |
        Make sure to apply the same index validation as suggested for `ecs.go` to `client.go` when accessing slices, to protect against index-out-of-range errors.
      relevant line: `+		for _, action := range describeRulesOutput.Rules[0].Actions {`

  ### Security concerns:
    no
    This PR does not introduce any apparent security concerns. The changes are localized to traffic routing logic and do not interact with potentially sensitive data in a way that would introduce vulnerabilities such as SQL injection, XSS, or CSRF. The addition of listener rule ARNs does not by itself present a security risk, provided that proper access controls and permissions are maintained on the AWS account.

@khanhtc1202
Copy link
Member

@moko-poi Could you review this change to ensure this fit your usecase 👀

@t-kikuc
Copy link
Member Author

t-kikuc commented Dec 21, 2023

I created another PR (#4733) so that we don't need to specify in app.pipecd.yaml.

This would be simpler for users.

@khanhtc1202
Copy link
Member

Nice, I prefer to go with the approach provided by PR #4733 . Let's hold this PR!

@t-kikuc
Copy link
Member Author

t-kikuc commented Dec 27, 2023

I close this PR because #4733 is better and merged.

@t-kikuc t-kikuc closed this Dec 27, 2023
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.

[ECS] Support modifying listener rules other than defaults
2 participants