Support Drift Detection for ECS

[t-kikuc]

What this PR does / why we need it:

Supported DriftDetection for ECS with some restrictions.

When synced:

When out-of-sync:

Restrictions:

• If desiredCount of headManifest is 0 or not set, which means Autoscale may be enabled), diff of desiredCount will be ignored.
  • cf. Ignore desiredCount of ECS when it's 0 or not set for AutoScaling #5030
• Some parameters are ignored. (e.g. Tags, LoadBalancers)
  • See detector.go > ignoreParameters()
• Even if unintended drift tasksets exist and the diff is detected, automatic deployment will NOT remove the tasksets.
  • If needed, we should fix the deployment logic of ECS.
  • If diff of other fields exists, desiredCount's diff will also be shown (as the above image).

I will add docs about them in another PR.

Which issue(s) this PR fixes:

Fixes #5005

Does this PR introduce a user-facing change?: Yes, users can detect drift for ECS

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

[codecov]

Codecov Report

Attention: Patch coverage is 21.25000% with 252 lines in your changes missing coverage. Please review.

Project coverage is 22.77%. Comparing base (60c4a53) to head (faf695e).
Report is 74 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/app/piped/driftdetector/ecs/detector.go	22.36%	236 Missing ⚠️
pkg/app/piped/driftdetector/detector.go	0.00%	16 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5122      +/-   ##
==========================================
+ Coverage   22.73%   22.77%   +0.04%     
==========================================
  Files         410      413       +3     
  Lines       43514    44147     +633     
==========================================
+ Hits         9892    10055     +163     
- Misses      32844    33305     +461     
- Partials      778      787       +9     

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

[t-kikuc]

/review

[github-actions]

PR Analysis

Main theme

"Add drift detection support for ECS applications"

PR summary

This PR introduces ECS application drift detection capabilities to PipeCD. It allows the system to detect discrepancies between the desired state defined in Git and the actual state of ECS applications that are running in the cloud. This functionality is made possible by the addition of a new package.

Type of PR

Enhancement

PR Feedback:

General suggestions

This PR makes significant progress in integrating ECS application drift detection functionality. The code logically separates ECS-specific concerns into its namespace and makes a reasonable attempt at checking for configuration drift by using AWS SDK types and custom comparison logic. However, there are a few areas where the proposed changes could be improved to ensure better performance, avoid potential bugs, and adhere to best practices.

Code feedback

• relevant file: pkg/app/piped/driftdetector/detector.go
  suggestion: |
  Consider initializing the time.Duration for interval from the piped configuration instead of hardcoding it. This would allow operators to configure the check interval to suit their specific use-case or workload. If a configuration option is not provided, the hardcoded default can be used as a fallback.

  interval: cfg.DriftDetectorIntervalOrDefault(),

  It's good practice to allow configuration of intervals and timeouts wherever feasible to provide flexibility and adaptability to different operational environments.
  (importance: medium)
  relevant line: + interval: time.Minute,

• relevant file: pkg/app/piped/driftdetector/ecs/detector.go
  suggestion: |
  Implement cleanup of gitRepos map to ensure the removal of repositories that are no longer tracked. Keeping them in the map indefinitely might cause memory leak issues over time or could lead to outdated repository data because the repositories may have been altered or removed outside the context of the drift detector. For instance, when an application is deleted, it would be a good practice to remove its corresponding repository from this map.

  // Possible function to clean up unused git repositories from d.gitRepos
  func (d *detector) cleanupGitRepos(appsByRepo map[string][]*model.Application) {
      for repoID := range d.gitRepos {
          if _, ok := appsByRepo[repoID]; !ok {
              delete(d.gitRepos, repoID)
          }
      }
  }

  Call cleanupGitRepos within the check routine to housekeep the gitRepos map.
  (importance: medium)
  relevant line: + gitRepos map[string]git.Repo,

Security concerns:

no

The PR does not appear to introduce obvious security issues. It relies on existing abstractions for dealing with AWS services and does not directly manage API calls or credentials, which would be the usual areas of concern for security issues like leaking sensitive information or improper access control. The logic for drift detection leverages AWS SDK types and PipeCD's existing mechanisms for cloning and pulling from git repositories, which should already be covered by the security practices in place for those areas.

[ffjlabo]

🚀

[khanhtc1202]

LGTM 👍

[khanhtc1202]

LGTM 👍