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

fix(core): relocate unix sockets to a subdirectory #13409

Merged
merged 4 commits into from
Aug 6, 2024
Merged

Conversation

flrgh
Copy link
Contributor

@flrgh flrgh commented Jul 23, 2024

This moves all internal unix sockets managed by Kong from {{prefix}} into a subdirectory at {{prefix}}/sockets. The conf loader makes this available via the new socket_path field, which is intended as a step towards relocating this path outside of the prefix tree in a later release.

KAG-4947

@flrgh
Copy link
Contributor Author

flrgh commented Jul 23, 2024

@Tieske per your in-ticket comments:

changing the unix-socket location from the top of the prefix to a subdirectory

Can you confirm if this is what you had in mind?

@Tieske
Copy link
Member

Tieske commented Jul 23, 2024

@Tieske per your in-ticket comments:

changing the unix-socket location from the top of the prefix to a subdirectory

Can you confirm if this is what you had in mind?

some background for the casual reader:

underlying problem to solve: the unix sockets cause trouble if they are located on a mount. This frequently done using containers during PoC evaluations of Kong and when testing plugins (Pongo does this).

Long term solution is moving them out of the prefix, but that is deemed to big a change for minor/patch releases. Moving them into a subdir allows us to apply a workaround by symlinking that subdir to a path outside the prefix, so the same net effect. (you cannot symlink the socket itself).

So yes, this is what we need short term, though for PoC's it is still a hassle and a non-optimal experience for potential Kong users.

@fffonion please have a look as well, since you authored the workaround in the Pongo branch: Kong/kong-pongo@master...support/3.4

Copy link
Member

@Tieske Tieske left a comment

Choose a reason for hiding this comment

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

I like the rename to "runtime_prefix".

kong/clustering/utils.lua Outdated Show resolved Hide resolved
@flrgh flrgh force-pushed the chore/move-sockets branch from d6d5c37 to badc124 Compare July 24, 2024 18:22
@flrgh flrgh changed the title [WIP] feat(core): relocate unix sockets to a subdirectory fix(core): relocate unix sockets to a subdirectory Jul 24, 2024
@flrgh flrgh marked this pull request as ready for review July 24, 2024 18:23
@curiositycasualty
Copy link
Contributor

My only qualm is that the runtime_prefix directory just be named "run" or "var/run" (as how /var/run is used to house unix socket files in many linux distros). But that's a minor knit pick.

@flrgh flrgh added the pr/discussion This PR is being debated. Probably just a few details. label Jul 24, 2024
@flrgh
Copy link
Contributor Author

flrgh commented Jul 24, 2024

Ready for code review, but we'll hold for merge until some internal discussion is finished.

@fffonion
Copy link
Contributor

fffonion commented Jul 25, 2024

I would actually suggest we just name it sockets and don't let user to configure it. Having it being named a broad name and make user facing may make usage of this directory out of our control in the future.
From time to time we or user may decided to put some other stuff in it because it's called runtime_prefix, then break some setup again.

@locao
Copy link
Contributor

locao commented Jul 25, 2024

don't let user to configure it

I second this opinion. We need this bugfix, but we don't want more knobs, so let's keep it hardcoded.

put some other stuff in it because it's called runtime_prefix

Strongly agree. Anyone without the background on this bugfix would think it's a suitable directory for anything runtime related. Heck, a few months from now I can see myself doing that.

@flrgh
Copy link
Contributor Author

flrgh commented Jul 29, 2024

put some other stuff in it because it's called runtime_prefix

Strongly agree. Anyone without the background on this bugfix would think it's a suitable directory for anything runtime related. Heck, a few months from now I can see myself doing that.

@locao this is actually more-or-less my intent :) Many of our struggles with the prefix directory originate from it being used as a catch-all location for different types of data with different lifecycles (configuration, user state, internal state, transient files, etc). This PR aims to be a step towards organizing data by use case and lifecycle.

@flrgh flrgh force-pushed the chore/move-sockets branch from badc124 to 35f20bd Compare July 29, 2024 17:41
@flrgh
Copy link
Contributor Author

flrgh commented Jul 29, 2024

@fffonion @outsinre I've renamed runtime_prefix to socket_path and /runtime to /sockets.

Eventually I expect that we will want to consolidate all types of runtime-scoped files to a new tree so that we can segregate them from the rest of the stateful contents of kong.configuration.prefix (therefore runtime_prefix), but I agree that it is fine to make the change explicit to sockets for now.

kong/tools/stream_api.lua Outdated Show resolved Hide resolved
@flrgh flrgh force-pushed the chore/move-sockets branch from 571e67e to 4465375 Compare July 30, 2024 16:48
@flrgh flrgh dismissed curiositycasualty’s stale review July 30, 2024 16:54

there have been significant changes since this approving review was made

@flrgh flrgh added this to the 3.8.0 milestone Jul 31, 2024
@fffonion
Copy link
Contributor

fffonion commented Aug 2, 2024

we might also want a backport to 3.4, as that's HSBC has been staying. please confirm in the ticket.

@flrgh
Copy link
Contributor Author

flrgh commented Aug 5, 2024

we might also want a backport to 3.4

@fffonion glad you asked. Backporting to 3.4 is the plan, but I noticed all the other backport release/x.x.x labels aside from 3.7 are gone. I'm not sure if I can re-create them or just need to do things manually.

@flrgh flrgh merged commit 126df19 into master Aug 6, 2024
25 checks passed
@flrgh flrgh deleted the chore/move-sockets branch August 6, 2024 17:02
@team-gateway-bot
Copy link
Collaborator

Cherry-pick failed for master, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git remote add upstream https://github.com/kong/kong-ee
git fetch upstream master
git worktree add -d .worktree/cherry-pick-13409-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-13409-to-master-to-upstream
git checkout -b cherry-pick-13409-to-master-to-upstream
ancref=$(git merge-base 3a1eeed4a7011259b845cc51b9585e1e1c54de76 a596f6e01bea6232ff60fd031021afd27d1b4338)
git cherry-pick -x $ancref..a596f6e01bea6232ff60fd031021afd27d1b4338

@github-actions github-actions bot added the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Aug 6, 2024
@flrgh
Copy link
Contributor Author

flrgh commented Aug 6, 2024

Removing the 3.7 backport label for now until I confirm that we will be backporting OSS/CE and not just EE.

@flrgh flrgh removed the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Aug 6, 2024
@fffonion
Copy link
Contributor

fffonion commented Aug 7, 2024

@flrgh Ah yes, only backports to EE 3.4. No backports to CE and other versions of EE from I understand. (I thought we were in the EE repo 😂 )

flrgh added a commit to Kong/docker-kong that referenced this pull request Oct 24, 2024
This adapts the fix from d3411e0 to additionally search $PREFIX/sockets
for leftover unix sockets.

See also: Kong/kong#13409
flrgh added a commit to Kong/docker-kong that referenced this pull request Oct 31, 2024
This adapts the fix from d3411e0 to additionally search $PREFIX/sockets
for leftover unix sockets.

See also: Kong/kong#13409
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build/bazel cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/cli core/clustering core/configuration core/proxy core/templates pr/discussion This PR is being debated. Probably just a few details. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants