Skip to content

socket: Add network namespace parameter to SocketAddress proto#38945

Merged
zuercher merged 3 commits intoenvoyproxy:mainfrom
tonya11en:tallen/netns_socket
Apr 11, 2025
Merged

socket: Add network namespace parameter to SocketAddress proto#38945
zuercher merged 3 commits intoenvoyproxy:mainfrom
tonya11en:tallen/netns_socket

Conversation

@tonya11en
Copy link
Copy Markdown
Member

@tonya11en tonya11en commented Mar 27, 2025

Extends SocketAddress to allow us to specify the network namespace of the socket. This is an API-only change and it is hidden, since there is no implementation.

Addresses #38947

@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #38945 was opened by tonya11en.

see: more, trace.

@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @abeyad
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #38945 was opened by tonya11en.

see: more, trace.

The new field is not implemented and hidden.

Signed-off-by: Tony Allen <txallen@google.com>
abeyad
abeyad previously approved these changes Mar 29, 2025
Copy link
Copy Markdown
Contributor

@abeyad abeyad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm api

based on discussion in #38947

@tonya11en tonya11en marked this pull request as ready for review April 4, 2025 20:05
@tonya11en
Copy link
Copy Markdown
Member Author

/retest

@tonya11en tonya11en changed the title [draft] socket: Add network namespace parameter to SocketAddress proto socket: Add network namespace parameter to SocketAddress proto Apr 4, 2025
Signed-off-by: Tony Allen <txallen@google.com>
Signed-off-by: Tony Allen <txallen@google.com>
@tonya11en
Copy link
Copy Markdown
Member Author

/retest

@tonya11en tonya11en requested a review from abeyad April 7, 2025 21:38
@tonya11en
Copy link
Copy Markdown
Member Author

@botengyao / @yanavlasov you are required reviewers on this, since you're code owners.

Copy link
Copy Markdown
Member

@botengyao botengyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! This is an API-only change and has been discussed in #38947. The proposal makes sense to me.

bool ipv4_compat = 6;

// The Linux network namespace to bind the socket to. If this is set, Envoy will
// create the socket in the specified network namespace. Only supported on Linux.
Copy link
Copy Markdown
Member

@botengyao botengyao Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any format requirement for the network namespace file path we should follow, e.g., /proc/<pid>/ns/net? Maybe add more comments?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also take into account named network namespaces, which are solved via bind mounts to /var/run/netns/<name>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to follow this PR up with the implementation, so I'll beef up the comments with examples in the next patch.

@botengyao
Copy link
Copy Markdown
Member

Aha, apologies, and we have the Slack notifier for the @assignee not the reviewers.

@botengyao
Copy link
Copy Markdown
Member

I think this is ok to merge with an api-shepherds approval and the rest.

/assign-from @envoyproxy/senior-maintainers

since Adi replied in the channel.

@repokitteh-read-only
Copy link
Copy Markdown

@envoyproxy/senior-maintainers assignee is @zuercher

🐱

Caused by: a #38945 (comment) was created by @botengyao.

see: more, trace.

@zuercher zuercher merged commit 86ca8d7 into envoyproxy:main Apr 11, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants