-
Notifications
You must be signed in to change notification settings - Fork 77
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
use adminport instead of host
network
#1176
base: main
Are you sure you want to change the base?
Conversation
@@ -743,8 +745,8 @@ func (cli *VanClient) gatewayStartContainer(ctx context.Context, gatewayName str | |||
"-d", | |||
"--name", | |||
gatewayName, | |||
"--network", | |||
"host", |
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 believe this would break gateway forward
as host ports won't be bound
to the gateway 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.
It is already broken in non-Linux systems. IIRC, the recommended way to proceed for Linux systems is using podman sites instead. If it is not the case, then we can add a flag to detect the OS running and keep --network host
for the ones where it is supported (at the moment, only Linux).
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.
Adding an OS specific customization in this case, makes more sense to me as well.
Eventually, if the host
network cannot be used in non-Linux systems, I believe we'd also need to disable
the forward
command (for now), as the alternative to make it work with the bridge
driver, would be to
redefine the container, so that the extra forward (host) ports can be bound to the container.
@ajssmith thoughts?
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.
@fgiorgetti Added a check to get the host OS and only do port mapping for windows
and darwin
.
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.
Along with these changes, I believe it would also be safe to throw an error inside
NewCmdForwardGateway
and NewCmdUnforwardGateway
as skupper gateway forward
and unforward
won't work without the host
network mode.
@@ -596,7 +597,7 @@ func (cli *VanClient) setupGatewayConfig(ctx context.Context, gatewayName string | |||
} | |||
gatewayConfig.Listeners["amqp"] = qdr.Listener{ | |||
Name: "amqp", | |||
Host: "localhost", | |||
Host: "0.0.0.0", |
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.
@ajssmith this is exposing the AMQP port to 0.0.0.0 instead.
Do you see any issue with it?
a first approach to address #816
Use the admin port mapped explicitly when launching the container.
This fixes the
gateway status
and most of the commands related to binding, as it does not need to expose new ports.Missing: