chore: validate health check protocol #5338
Conversation
CaptainCarpensir
requested review from
efekarakus
and removed request for
a team
|
🍕 Here are the new binary sizes!
|
|
This validates additional listener and routing rules as if they also create a healthcheck. Only the main rules of each are used for the default healthcheck ports. Will fix and then set to be ready for review again. |
internal/pkg/manifest/validate.go
Outdated
| func validateHealthCheckPorts(opts validateHealthCheckPortsOpts) error { | ||
| if opts.alb != nil { | ||
| for _, rule := range opts.alb.RoutingRules() { | ||
| // healthCheckPort is defined by RoutingRule.HealthCheck.Port, with fallback on RoutingRule.TargetPort, then image.port. |
There was a problem hiding this comment.
is this logic to figure out the health check port defined elsewhere already? (maybe package stack?) if so, maybe we should put this HealthCheckPort(imagePort) uint16 logic onto the routing rule 🤔
There was a problem hiding this comment.
Unfortunately the behavior isn't written in one place. Maybe this should be changed though 🤔 Currently the port is half determined by stack/transformers.go and half determined by the logic within the template. I think this can be easily changed though so there's exactly one determining function in the codebase for each TargetGroup health check port!
There was a problem hiding this comment.
I'm unsure what exactly to do about this now because I've realized that in order to change the deploy/cloudformation/stack package to use the function I've written in the manifest package as the source of truth for the HealthCheckPort, we would need to update some people's stack. Basically if they have the following manifest, the target group would undergo a change.
nlb:
port: 80
Would cause the target group to go from:
NLBTargetGroup:
Port: 80
to:
NLBTargetGroup:
HealthCheckPort: 80
This doesn't cause any change in behavior but requires a stack update. Maybe this is a change that should go along with updating the NLB/TargetGroup when the user specifies UDP?
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## mainline #5338 +/- ##
=========================================
Coverage 69.86% 69.86%
=========================================
Files 297 297
Lines 45020 45110 +90
Branches 287 287
=========================================
+ Hits 31453 31518 +65
- Misses 12038 12057 +19
- Partials 1529 1535 +6
☔ View full report in Codecov by Sentry. |
internal/pkg/manifest/validate.go
Outdated
| if err = validateHealthCheckPorts(validateHealthCheckPortsOpts{ | ||
| exposedPorts: exposedPortsIndex, | ||
| mainContainerPort: l.ImageConfig.Port, | ||
| alb: &l.HTTPOrBool.HTTP, | ||
| nlb: &l.NLBConfig, | ||
| }); err != nil { | ||
| return fmt.Errorf("validate load balancer health check ports: %w", err) |
There was a problem hiding this comment.
Does it make more sense to be
| if err = validateHealthCheckPorts(validateHealthCheckPortsOpts{ | |
| exposedPorts: exposedPortsIndex, | |
| mainContainerPort: l.ImageConfig.Port, | |
| alb: &l.HTTPOrBool.HTTP, | |
| nlb: &l.NLBConfig, | |
| }); err != nil { | |
| return fmt.Errorf("validate load balancer health check ports: %w", err) | |
| if err = validateALBHealthCheckPorts(validateHealthCheckPortsOpts{ | |
| exposedPorts: exposedPortsIndex, | |
| mainContainerPort: l.ImageConfig.Port, | |
| alb: &l.HTTPOrBool.HTTP, | |
| }); err != nil { | |
| return fmt.Errorf("validate application load balancer health check ports: %w", err) |
And do the same thing to NLB separately? Because validateHealthCheckPorts seems to deal ALB and NLB separately.
There was a problem hiding this comment.
I don't think this is a good idea because it could lead to the error messages being slightly misleading. When a health check is exposed on a port using udp, the solution is either to change the NLB port exposing udp, or to change the health check port set to the port using UDP traffic. Because the solution can be approached from either direction I think it's more logically sound to validate them together.
dannyrandall
added
the
do-not-merge
CaptainCarpensir
removed
the
do-not-merge
Validates that Load Balancer HealthChecks don't use invalid protocol for load balancer listeners. For aws#4767 By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.
Validates that Load Balancer HealthChecks don't use invalid protocol for load balancer listeners.
For #4767
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.