refactor: exposed port validation#5303
Conversation
CaptainCarpensir
requested review from
huanjani
and removed request for
a team
|
🍕 Here are the new binary sizes!
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## mainline #5303 +/- ##
=========================================
Coverage 69.76% 69.77%
=========================================
Files 296 296
Lines 44581 44610 +29
Branches 286 286
=========================================
+ Hits 31102 31125 +23
- Misses 11971 11975 +4
- Partials 1508 1510 +2
☔ View full report in Codecov by Sentry. |
KollaAdithya
added
the
do-not-merge
huanjani
left a comment
huanjani
left a comment
There was a problem hiding this comment.
Is there a unit test for errContainerPortExposedWithMultipleProtocol?
Good catch! There is now 😄 |
CaptainCarpensir
removed
the
do-not-merge
Lou1415926
left a comment
Lou1415926
left a comment
There was a problem hiding this comment.
LGTM!! Sorry for the late feedbacks : ( Thankfully, I don't have any major comments!
| } | ||
| containerNameFor[aws.Uint16Value(opts.mainContainerPort)] = opts.mainContainerName | ||
|
|
||
| targetPort := aws.Uint16Value(opts.mainContainerPort) |
There was a problem hiding this comment.
just sharing my thought here, this is not a request 👀 I personally would refrain from using the term target in the variable names, because up until this point, target means that a port a) not only is exposed, b) but also functions as a port that receives traffic from an NLB/ALB.
A port could be exposed, but only receive traffic from within the task - for example, a sidecar could communicate with another sidecar through a port. These port satisfy a), but not b). They are generally not considered "a target port", because they are not the target of anything.
http:
target_container: nginx
sidecars:
logging:
port: 81 # Exposed, but not an HTTP target nor an NLB target.
nginx:
port: 82| if existingContainerNameAndProtocol, ok := portExposedTo[targetPort]; ok { | ||
| targetProtocol = existingContainerNameAndProtocol.containerProtocol | ||
| } |
There was a problem hiding this comment.
Is this added so that populateAndValidateMainContainerPort works no matter what order it is called in relative to the other populateAndValidateXPort functions? Based on the order in which these functions are called, it seems like ok would never be true 👀 !
There was a problem hiding this comment.
It's for the case where the NLB specifies a protocol that's not TCP to be used for the main container. In this case the NLB specifies that and it gets populated in the map.
An example manifest:
image:
port: 80
nlb:
port: 80/udp
Without this statement here the program would error out saying container "mockMainContainer" is exposing the same port 80 with protocol TCP and UDP
There was a problem hiding this comment.
oh I just realized you moved the function call from the first to the last! Then I think we are good 👍🏼
| // Prefer `nlb.port`, then existing exposed port mapping, then fallback on default protocol | ||
| targetProtocol := defaultProtocol | ||
| if existingContainerNameAndProtocol, ok := portExposedTo[targetPort]; ok { | ||
| targetProtocol = existingContainerNameAndProtocol.containerProtocol |
There was a problem hiding this comment.
I feel like nlb is a little different than the other. Without protocol, default protocol for nlb should be tcp, instead of any protocol that is already exposed. Consider this manifest:
nlb:
port: 81
sidecars:
nginx:
port: 81/udpI would expect this manifest to error out, honestly. The nlb.port being 81 means it listens on 81 for TCP port, and route the traffic to port 81 of the nginx container, and expect that target port to be tcp. However, the same port is already exposed as udp. So error.
I think basically, the question is, when we think of the protocol for nlb, do we think of it in a NLB-first way, or a container-first way. My proposal above is NLB-first, the code that I'm commenting on is container-first.
Refactors the validation for exposed ports to make code more readable, preserve functionality, and include protocol validation. Step in 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.
6 participants
Refactors the validation for exposed ports to make code more readable, preserve functionality, and include protocol validation.
Step in #4767
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.