-
Notifications
You must be signed in to change notification settings - Fork 532
Stop mapping privileged ports #11508
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: main
Are you sure you want to change the base?
Stop mapping privileged ports #11508
Conversation
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: Seth Heidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
…m/sheidkamp/kgateway into stop-mapping-ports-deployer-factory Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
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.
Pull Request Overview
This PR removes automatic remapping of privileged ports (<1024) to high ports, and instead injects a sysctl into the PodSecurityContext to allow use of low ports directly. It updates translators, Helm value helpers, gateway parameter handling, and test fixtures to use the original port numbers.
- Remove
TranslatePort
usage and pass listener ports through unchanged. - Introduce
UpdateSecurityContexts
(andallowPrivilegedPorts
) to append the needed sysctl for privileged ports. - Update many test suites and YAML outputs to expect the original low port values.
- Makefile Ginkgo flake-attempts configurable via
FLAKE_ATTEMPTS
.
Reviewed Changes
Copilot reviewed 74 out of 74 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
pkg/deployer/values_helpers.go | Stop translating ports when building HelmPorts. |
pkg/deployer/gateway_parameters.go | Add UpdateSecurityContexts /allowPrivilegedPorts , inject sysctl for low ports. |
internal/kgateway/translator/listener/gateway_listener_translator.go | Replace TranslatePort calls with raw port via getListenerPort . |
test/kubernetes/e2e/features/* | Replace hard-coded mapped ports with constants and update fixtures. |
Makefile | Make Ginkgo flake attempts configurable (FLAKE_ATTEMPTS ). |
Comments suppressed due to low confidence (3)
pkg/deployer/gateway_parameters.go:35
- There isn’t a unit test directly covering UpdateSecurityContexts for both cases (with and without privileged ports). Adding focused tests will ensure that sysctls are only injected when needed.
func UpdateSecurityContexts(gwp *v1alpha1.GatewayParameters, vals *HelmConfig) {
test/kubernetes/e2e/features/basicrouting/suite.go:91
- Verify that this closing brace correctly matches the
for
loop in TestGatewayWithRoute; an extra or missing}
can introduce a syntax error or change the test’s scope.
}
pkg/deployer/values_helpers.go:58
- Since remapping logic was removed, ensure there are no remaining calls to
ports.TranslatePort
elsewhere that could cause inconsistent port behavior.
targetPort := port
internal/kgateway/translator/listener/gateway_listener_translator.go
Outdated
Show resolved
Hide resolved
Signed-off-by: sheidkamp <[email protected]>
…m/sheidkamp/kgateway into stop-mapping-ports-deployer-factory Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
based on previous conversations, we are not solving downtime during upgrade in this PR; this PR lgtm with a small nit. |
Signed-off-by: sheidkamp <[email protected]>
what are the implications of setting |
This is far less impactful than something like changing iptables settings. This is considered a "safe sysctls" since k8s 1.22, and does not require any additional privileges - All safe sysctls are enabled by default. This also enabled by default in Istio: |
Signed-off-by: sheidkamp <[email protected]>
Description
Stop mapping privileged ports (< 1024) to higher values.
When listeners use privileged ports, add
to the PodSecurityContext.
Before this change, if a user defined a listener on a privileged port, they were unable to define a listener on a mapped port. Eg, if they defined a listener on port 80, they could not define a listener on port 8080.
Change Type
Changelog