-
Notifications
You must be signed in to change notification settings - Fork 285
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
feat: add knob for switching container port #3333
Conversation
api/v1alpha1/envoyproxy_types.go
Outdated
// This allows the container to bind to the port without needing a CAP_NET_BIND_SERVICE capability. | ||
// | ||
// +optional | ||
DisablePortShift *bool `json:"disablePortShift,omitempty"` |
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.
any other alternate name suggestions ?DisablePortShift
is too vague imo and doesnt account for the fact that this issue is tied to ContainerPort
cc @envoyproxy/gateway-maintainers @envoyproxy/gateway-reviewers
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.
To come up with variable names is harder than writing code.
Something like DisableShiftContainerPort
?
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.
Could it be an other way around, like BindPort
or StaticPort
bool value?
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.
BindPort
and StaticPort
looks like fields for port value. Not for bool
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.
I prefer UseListenerPortAsContainerPort
from https://github.com/envoyproxy/gateway/pull/2405/files
@envoyproxy/gateway-maintainers / @envoyproxy/gateway-reviewers wdyt
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.
+1 for UseListenerPortAsContainerPort
.
I can tell what UseListenerPortAsContainerPort
does by its name, but can't infer what DisablePortShift
without looking into the comment. :-)
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.
No problem. I can use this variable. However, I see a small logical issue. If I plan to use a port higher than 1024 in the Listener (for example, 3000), there will be no 'port offsets'. That means the UseListenerPortAsContainerPort
variable will be set to false
(the default value), and at the same time, the listener port and the container port will be equal.
|
||
// DisablePortShift disables the port shifting feature in the Envoy Proxy. | ||
// When set to false (default value), if the service port is a privileged port (1-1023), add a constant to the value converting it into an ephemeral port. | ||
// This allows the container to bind to the port without needing a CAP_NET_BIND_SERVICE capability. |
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 this require user involvement, or does EG automatically add the CAP_NET_BIND_SERVICE
capability to the Envoy container?
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.
I'm not sure about "automatically".
In my case I set this CAP in envoyProxy, like:
apiVersion: gateway.envoyproxy.io/v1alpha1
kind: EnvoyProxy
metadata:
name: default
namespace: envoy-gateway
spec:
provider:
kubernetes:
disablePortShift: true. # tmp name
envoyDaemonSet:
pod:
nodeSelector:
node-role.kubernetes.io/gateway: "true"
container:
image: envoyproxy/envoy:v1.29.2
securityContext:
allowPrivilegeEscalation: true
capabilities:
add:
- NET_BIND_SERVICE
drop:
- ALL
seccompProfile:
type: RuntimeDefault
patch:
type: StrategicMerge
value:
spec:
template:
spec:
hostNetwork: true
dnsPolicy: ClusterFirstWithHostNet
type: Kubernetes
And need to use NOT distroless
images, bc distroless
can't bind on 1-1024 ports
886c1df
to
f7a4ced
Compare
I rebase my branch from main and change field name from But I can't run e2e test localy. If something broken - I will test all and fix in monday. (I'm not sure about route.go... Many changes. Now this package don't collect container ports) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3333 +/- ##
==========================================
+ Coverage 67.41% 67.43% +0.01%
==========================================
Files 166 166
Lines 19186 19197 +11
==========================================
+ Hits 12934 12945 +11
Misses 5322 5322
Partials 930 930 ☔ View full report in Codecov by Sentry. |
Signed-off-by: zvlb <vl.zemtsov@gmail.com>
f7a4ced
to
65253e1
Compare
Just rebase |
Test error: |
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.
branch needs rebase again but lgtm
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.
LGTM thanks !
issue - #3226, #2310
Previos PR - #2405 (not my, not finished)
@arkodg I add new field to EnvoyProxy (not EnvoyGateway), bc with this EnvoyProxy We can use kubernetes Gateway CRD for functionality)
Local tests finished good