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

Supporting UNIX sockets in address matching. #655

Merged
merged 1 commit into from
Jul 26, 2022
Merged

Supporting UNIX sockets in address matching. #655

merged 1 commit into from
Jul 26, 2022

Conversation

alejandro-colomar
Copy link
Contributor

No description provided.

@alejandro-colomar alejandro-colomar linked an issue Mar 2, 2022 that may be closed by this pull request
@alejandro-colomar
Copy link
Contributor Author

Simplified it to support any unix socket, by specifying the string "unix". A path shall not be specified.

@@ -47,6 +47,16 @@ nxt_http_route_addr_pattern_parse(nxt_mp_t *mp,

nxt_str_null(&port);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nulling port variable has no sense in case of "unix" socket. This should be after the "unix" socket condition, not before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Right.

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense to move "unix" socket condition also above the addr.length < 2 check. Because that check is directly related to the following condition with addr.start[0] == '*' && addr.start[1] == ':'. Keeping them together makes code self-explainable about why length should be not less than 2.

src/nxt_http_route.c Outdated Show resolved Hide resolved
@VBart
Copy link
Contributor

VBart commented Mar 28, 2022

You also need to add a feature to changes.xml.

@alejandro-colomar
Copy link
Contributor Author

You also need to add a feature to changes.xml.

Done. I still need to add a pytest.

docs/changes.xml Outdated
@@ -37,6 +37,12 @@ compatibility with GCC 12.
</para>
</change>

<change type="feature">
<para>
supporting UNIX sockets in address matching
Copy link
Contributor

Choose a reason for hiding this comment

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

The dot at the end of sentence is missing.

Also I usually try to estimate subjectively the amount of users affected or importance of the feature/fix for them and sort relatively to that estimation. In this case, this feature seems more important than support for a not-yet-released compiler version (so it should go above).

Also 2, it makes sense to consult with Artem about the best wording. Commit log for devs, change log for users. The current wording is good enough, but I have some doubts that many users will understand what "address matching" is meant here. Maybe it's worth to describe specifically, that this is about address matching in routing and "client_ip" options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dot at the end of sentence is missing.

D'oh.

Also I usually try to estimate subjectively the amount of users affected or importance of the feature/fix for them and sort relatively to that estimation. In this case, this feature seems more important than support for a not-yet-released compiler version (so it should go above).

Makes sense.

Also 2, it makes sense to consult with Artem about the best wording. Commit log for devs, change log for users. The current wording is good enough, but I have some doubts that many users will understand what "address matching" is meant here. Maybe it's worth to describe specifically, that this is about address matching in routing and "client_ip" options.

Yes, and yes, I'll add those details regarding client_ip.

@alejandro-colomar
Copy link
Contributor Author

I'm getting EACCESS from unlink(2) :(

@alejandro-colomar alejandro-colomar changed the title Supporting UNIX sockets in address matching Supporting UNIX sockets. Jul 18, 2022
@alejandro-colomar alejandro-colomar changed the title Supporting UNIX sockets. Supporting UNIX sockets in address matching. Jul 19, 2022
@alejandro-colomar
Copy link
Contributor Author

I broke the patch I added yesterday for unlinking sockets to a new separate PR.

This closes #645 issue on GitHub.

(Also moved a changelog line that was misplaced in a previous commit.)
@nginx-hg-mirror nginx-hg-mirror merged commit 6e36584 into nginx:master Jul 26, 2022
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.

X-Forwarded-For (client_ip->source) with unix:
3 participants