Skip to content

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

Merged
yossigo merged 6 commits intoredis:unstablefrom
devnexen:fbsd_sock_cookie
Apr 20, 2022
Merged

Add socket-mark-id support for marking sockets.#10349
yossigo merged 6 commits intoredis:unstablefrom
devnexen:fbsd_sock_cookie

Conversation

@devnexen
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@yossigo
Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
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.

Comment thread src/anet.c Outdated
Comment on lines 696 to 698
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

devnexen commented Apr 14, 2022

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

@oranagra
Copy link
Copy Markdown
Member

ohh, i missed it.

Comment thread redis.conf Outdated
Comment thread src/config.h Outdated
Copy link
Copy Markdown
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?

Comment thread src/anet.c Outdated
Comment thread src/config.c Outdated
Comment thread src/anet.c Outdated
* 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
Copy Markdown
Collaborator

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
Copy Markdown
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 mentioned this pull request Apr 27, 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

Development

Successfully merging this pull request may close these issues.

4 participants