-
Notifications
You must be signed in to change notification settings - Fork 1.3k
dockerfile: rules check for EXPOSE instruction #6135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
feae557
to
52f3531
Compare
} | ||
} | ||
|
||
func (ps *portSpecs) splitParts(rawport string) (hostIP, hostPort, containerPort string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why you changed these all into methods, because the ps
receiver is not used anywhere, so now it's required to first construct a (redundant
portSpecs` before being able to use these functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used in parsePort
method to handle the build check and as other functions are only useful for port specs I was thinking it would be better to encapsulate them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to have separate warnings for different invalid inputs of EXPOSE
? Maybe we should just have one that covers both the casing and the IP issue. Unless we want to make IPs error instead of warn.
return parts[0], parts[1], parts[2] | ||
default: | ||
n := len(parts) | ||
return strings.Join(parts[:n-2], ":"), parts[n-2], parts[n-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code should ultimately go away, as it's only for handling the hostIP:hostPort-hostPortEnd:containerPort-containerPortEnd/Proto
Ultimately, all that remains is;
Split <port>[-<endport>][/proto]
(which would be a split
on /
, then splitting on -
. Which may be a 5 line-function 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I'm looking to handle this as build check in follow-up for IP address case. In future releases we could just error out if an IP is detected.
Yes I think it makes sense to have a separate warning for IP address case to first warn users of removal in the future. |
Signed-off-by: CrazyMax <[email protected]>
Signed-off-by: CrazyMax <[email protected]>
52f3531
to
7f64931
Compare
Pushed extra commit to add a new rule for IP address and host-port mapping |
7f64931
to
d6c82c6
Compare
follow-up #6128 (comment)
fixes #2173
Adds two rules for
EXPOSE
instruction:ExposeProtoCasing
: Protocol in EXPOSE instruction should be lowercase.ExposeInvalidFormat
: IP address and host-port mapping should not be used in EXPOSE instruction. This will become an error in a future release.