feat: add service connect mft

[iamhopaul123]

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

[codecov-commenter]

Codecov Report

Merging #4046 (3353d45) into mainline (101b95d) will increase coverage by 0.04%.
The diff coverage is 89.36%.

@@             Coverage Diff              @@
##           mainline    #4046      +/-   ##
============================================
+ Coverage     69.12%   69.17%   +0.04%     
============================================
  Files           248      248              
  Lines         35511    35598      +87     
  Branches        264      264              
============================================
+ Hits          24548    24625      +77     
- Misses         9770     9779       +9     
- Partials       1193     1194       +1     

Impacted Files	Coverage Δ
...rnal/pkg/deploy/cloudformation/stack/worker_svc.go	59.67% <ø> (ø)
internal/pkg/template/workload.go	42.58% <ø> (ø)
internal/pkg/manifest/validate.go	84.08% <73.07%> (-0.27%)	⬇️
internal/pkg/manifest/workload.go	82.54% <81.25%> (-0.06%)	⬇️
...nal/pkg/deploy/cloudformation/stack/backend_svc.go	88.67% <100.00%> (+0.38%)	⬆️
...rnal/pkg/deploy/cloudformation/stack/lb_web_svc.go	78.03% <100.00%> (+0.51%)	⬆️
...al/pkg/deploy/cloudformation/stack/transformers.go	77.04% <100.00%> (+0.11%)	⬆️
internal/pkg/manifest/backend_svc.go	88.67% <100.00%> (+1.44%)	⬆️
internal/pkg/manifest/lb_web_svc.go	89.09% <100.00%> (+0.20%)	⬆️
internal/pkg/manifest/transform.go	98.36% <100.00%> (+0.07%)	⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[efekarakus]

Thanks boss looks great, just a refactor request and then

Feel free to remove the label once addressed

[efekarakus]

Ah I get why now you moved this method to be part of *BackendService instead of ServiceConnect because you need access to the other fields in the manifest like target container.

What do you think of changing the method name to be more generic, for example:

// IsDiscoverable returns true if the service publishes a port that can be reached by other services in the VPC.
func (svc *BackendService) IsDiscoverable() bool {
   // This logic.
}

func (svc *LoadBalancedWebService) IsDiscoverable() bool {
   // returns true if service connect is enabled.
}

// IsDiscoverable is  false for workers as queue processing services don't accept network traffic.
func (svc *WorkerService) IsDiscoverable() bool { return false }

// IsDiscoverable is false as ephemeral tasks cannot be reached consistently.
func (job *ScheduledJob) IsDiscoverable() bool { return false }

[iamhopaul123]

Hmm ServiceConnectEnabled might still be better since it isn't really whether the service is discoverable within the VPC because they could use SD.

[efekarakus]

Ah I see what you mean, perhaps you're right and right now it's not worthwhile to introduce more abstraction.
So let's keep it as is 👍 , nit: maybe just rename to IsServiceConnectEnabled?

[efekarakus]

We talked offline but for visibility: we need to add a feature flag here otherwise it'll start deploying all new services with the updated template

[Lou1415926]

Do we not want to keep some integration test for when service connect is enabled (i.e. it's not opted out)? Or are we looking to add those integration test in the future?

[iamhopaul123]

I think we have all three cases...connect false, connect true and no config.

[efekarakus]

Adding the label for "feat:" PRs until we release a patch release for v1.22.1