frontend/dockerfile: remove uses of github.com/docker/go-connections/nat for parsing EXPOSE #6128
Conversation
github-actions
bot
added
area/dockerfile
area/dependencies
thaJeztah
force-pushed
the
remove_go_connections
branch
from
fc8c253 to
37aecb6
Compare
thaJeztah
force-pushed
the
remove_go_connections
branch
from
37aecb6 to
1287add
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
thaJeztah
force-pushed
the
remove_go_connections
branch
from
1287add to
f07ded2
Compare
| return start, end, err | ||
| } | ||
|
|
||
| parts := strings.Split(ports, "-") |
There was a problem hiding this comment.
I think I had some changes stashed in go-connections to use strings.Cut here as well, but kept the implementation here to be like it was in the current version (may be opening follow-ups).
There was a problem hiding this comment.
I can apply similar changes here in a follow-up.
|
/cc @crazy-max @tonistiigi PTAL |
| switch proto { | ||
| case "": | ||
| return "tcp", port, nil | ||
| case "tcp", "udp", "sctp": |
There was a problem hiding this comment.
I think this is fine to keep sctp as long as host kernel and network stack support SCTP.
There was a problem hiding this comment.
Yeah, also went looking what's defined in the OCI spec, but it leaves it open as to what protocols must be supported. Not even sure if BuildKit should know about protocols, or just "anything goes" - at most, it should probably be defined in the OCI what format the proto takes (a-z (A-Z?), max length?);
https://github.com/opencontainers/image-spec/blob/2daaaaf0e7c16a6a147be91fde277f38573be672/config.md#properties
| // TODO(thaJeztah): mapping IP-addresses should not be allowed for EXPOSE; see https://github.com/moby/buildkit/issues/2173 | ||
| if ip != "" && ip[0] == '[' { |
There was a problem hiding this comment.
Yeah ip doesn't make sense in a Dockerfile. Maybe we can keep it for now to avoid breaking people and implement a build check to warn users about it first?
There was a problem hiding this comment.
Yeah, wasn't sure how the build-checks worked, but that was also in the back of my head (warn, not fail at start).
Leaving that for a follow-up for sure, not for this PR.
| } | ||
|
|
||
| func validateProto(proto string) error { | ||
| proto = strings.ToLower(proto) |
There was a problem hiding this comment.
It might be good to have a build check for this one as well to warn if proto is not lower case.
There was a problem hiding this comment.
Yeah, I hate that we have way too many places where we were a bit too permissive; it means we need to check multiple variations everywhere (and while perhaps TCP or tcp would be fine, I'm much less a fan of TcP or tCp being considered "valid").
There was a problem hiding this comment.
Yes will look to have a build check for this
Let me have a look! |
Fork the nat.ParsePortSpecs and related functions. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
BuildKit doesn't use the port-bindings, as EXPOSE only allows for the ports (and protos) to expose. Simplify the function to return just that. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
remove_go_connections
branch
from
f07ded2 to
656d524
Compare
|
Updated; also applied the changes from docker/go-connections#143 |
The EXPOSE instruction in the Dockerfile does not use mapping, only specifies a range of ports to expose, so we can skip creating nat.PortMapping for each. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Apply the improvements from; docker/go-connections@ccab5a8 Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
remove_go_connections
branch
from
656d524 to
9962429
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR removes the dependency on github.com/docker/go-connections/nat by forking and simplifying the port parsing functionality specifically for the EXPOSE instruction in Dockerfiles. The changes maintain the same functionality while reducing external dependencies and tailoring the code to BuildKit's specific needs.
- Forked
nat.ParsePortSpecsand related functions into the BuildKit codebase - Simplified the parsing logic by removing port-binding functionality (EXPOSE only needs port/protocol info)
- Added comprehensive test coverage for the new parsing functions
Reviewed Changes
Copilot reviewed 4 out of 10 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| go.mod | Removes the dependency on github.com/docker/go-connections |
| frontend/dockerfile/dockerfile2llb/convert_expose.go | Implements forked and simplified port parsing functions for EXPOSE instruction |
| frontend/dockerfile/dockerfile2llb/convert_expose_test.go | Adds comprehensive test coverage for the new port parsing functionality |
| frontend/dockerfile/dockerfile2llb/convert.go | Removes the old dispatchExpose function and import of nat package |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
This forks the parsing-code from github.com/docker/go-connections/nat to remove the dependency on github.com/docker/go-connections.
Some follow-ups may be needed to also address;
frontend/dockerfile: fork nat.ParsePortSpecs
Fork the nat.ParsePortSpecs and related functions.
frontend/dockerfile: parsePortSpecs: simplify
BuildKit doesn't use the port-bindings, as EXPOSE only
allows for the ports (and protos) to expose. Simplify
the function to return just that.
frontend/dockerfile: parsePortSpec: remove unused code
The EXPOSE instruction in the Dockerfile does not use mapping,
only specifies a range of ports to expose, so we can skip
creating nat.PortMapping for each.
frontend/dockerfile: inline validateProto