Skip to content
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

Add socket-mark-id support for marking sockets. #10349

Merged
merged 6 commits into from Apr 20, 2022

Conversation

devnexen
Copy link
Contributor

@devnexen devnexen commented Feb 27, 2022

Add a configuration option to attach an operating system-specific identifier to Redis sockets, supporting advanced network configurations using iptables (Linux) or ipfw (FreeBSD).

@devnexen devnexen force-pushed the fbsd_sock_cookie branch 5 times, most recently from 1e512b6 to 1f1bb97 Compare February 27, 2022 11:48
@yossigo
Copy link
Member

yossigo commented Mar 8, 2022

@devnexen Generally this looks good to me, but I'd like to consider if this should be a FreeBSD-specific feature or something more generic that we implement also on Linux (as a connection mark maybe? not sure, need to look into that).

@yossigo yossigo added the state:major-decision Requires core team consensus label Mar 8, 2022
@devnexen
Copy link
Contributor Author

that might be a tricky decision indeed, might be a risk to give a false sense of equivalence, so far I have not seen anything even remotely ressemblant on Linux, socket options in this system have very different contexts (but which can be used in redis though but as system specific like controlling thp support in some sense).

adding the possibility to tag sockets with an ID when netfiltering
 filters above operates in that level.
@devnexen
Copy link
Contributor Author

finally taking back what I said, I found some equivalence :)

@yossigo
Copy link
Member

yossigo commented Apr 14, 2022

@devnexen It sounded this ID is the equivalent of a conn mark but I didn't have the bandwidth to dig deep and validate, thank you!

@yossigo
Copy link
Member

yossigo commented Apr 14, 2022

Calling @redis/core-team for approval, this is a small change but potentially high value in some environments - see top comments.

@yossigo yossigo added the approval-needed Waiting for core team approval to be merged label Apr 14, 2022
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

I'm a bit uncomfortable adding an interface that's not supported on Linux.
but i guess it won't harm anyone, and for some it could be useful.
so @yossigo if you're ok with it, i'm too.

src/anet.c Outdated
Comment on lines 696 to 698
if (id > 0)
anetSetError(err,"anetSockid unsupported on this platform");
return ANET_OK;
Copy link
Member

Choose a reason for hiding this comment

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

are we sure we wanna return OK here?
alternatively, maybe we should add an isValid callback to the config, and fail the configuration when this is not supported?

@devnexen
Copy link
Contributor Author

devnexen commented Apr 14, 2022

In fact Linux is supported (finally). Originally not planned but found a way.

@oranagra
Copy link
Member

ohh, i missed it.

@oranagra oranagra added this to Backlog in 7.0 via automation Apr 14, 2022
@oranagra oranagra moved this from Backlog to Awaits merge in 7.0 Apr 14, 2022
redis.conf Outdated Show resolved Hide resolved
src/config.h Outdated
@@ -80,6 +80,8 @@
/* MSG_NOSIGNAL. */
#ifdef __linux__
#define HAVE_MSG_NOSIGNAL 1
#define HAVE_SOCKOPTID 1
#define SOCKOPTID SO_MARK
Copy link
Collaborator

Choose a reason for hiding this comment

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

SO_MARK (since Linux 2.6.25)
Check with #ifdef SO_MARK?

src/anet.c Outdated Show resolved Hide resolved
src/config.c Outdated Show resolved Hide resolved
src/anet.c Outdated Show resolved Hide resolved
* A bit more generalized, OS-agnostic terms.
* Avoid any setsockopt() call by default.
@yossigo yossigo changed the title socket-id new config setting proposal. Add socket-mark-id support for marking sockets. Apr 19, 2022
@yossigo
Copy link
Member

yossigo commented Apr 19, 2022

@devnexen I've slightly modified the PR to be more generalized, and avoid calling setsockopt() if not explicitly enabled. I assume that a zero value is anyway the default and that there's no need to make this explicit call, please let me know if these changes make sense.

@devnexen
Copy link
Contributor Author

Yes I ve seen it, it s fine by me.

@yossigo yossigo added the release-notes indication that this issue needs to be mentioned in the release notes label Apr 20, 2022
@yossigo yossigo merged commit aba2865 into redis:unstable Apr 20, 2022
@oranagra oranagra moved this from Awaits merge to Unreleased in 7.0 Apr 20, 2022
@oranagra oranagra mentioned this pull request Apr 27, 2022
@oranagra oranagra moved this from Unreleased to Done in 7.0 May 2, 2022
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
Add a configuration option to attach an operating system-specific identifier to Redis sockets, supporting advanced network configurations using iptables (Linux) or ipfw (FreeBSD).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval-needed Waiting for core team approval to be merged release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus
Projects
Archived in project
7.0
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants