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.

Sign up for GitHub

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

Jump to bottom

Add support for unix socket as upstream #1866

Merged
merged 6 commits into from
Oct 26, 2023
Merged

Add support for unix socket as upstream #1866

merged 6 commits into from
Oct 26, 2023

Conversation

babs
Copy link
Contributor

@babs babs commented Oct 29, 2022 •
edited
Loading

Description

Add support for unix socket at upstream
Might require some rework.

Should be able to close #1865

Motivation and Context

Some apps might require to go through a unix socket when listening on a tcp port, even bound locally is not secure enough and socket ownership is used as an extra isolation layer.

How Has This Been Tested?

I created a simple uvicorn app that listen to unix socket and used --upstream=unix:///path/pf/the/unix.sock .

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@babs babs requested a review from a team as a code owner October 29, 2022 11:28
@babs babs mentioned this pull request Oct 29, 2022
Closed
JoelSpeed
JoelSpeed reviewed Oct 29, 2022
View reviewed changes
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Would like to see some tests, similar to the HTTP tests, but we will need to work out how to create a temporary unix socket. I wonder if the HTTP test server supports this already? 🤔

pkg/upstream/http.go Outdated Show resolved Hide resolved
pkg/upstream/http.go Outdated Show resolved Hide resolved
@babs
Copy link
Contributor Author

babs commented Oct 29, 2022

How is currently started the upstream http to test against ?

@JoelSpeed
Copy link
Member

We create an http testserver in the suite setup, perhaps we can create something there that sets up a unix socket?

Otherwise code LGTM

Apologies for the delay in getting back to you

@babs
Copy link
Contributor Author

babs commented Dec 23, 2022

I remember myself doing the server portion, but can't put my hand on it...

@github-actions
Copy link
Contributor

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

@github-actions github-actions bot added the Stale label Feb 23, 2023
@babs
Copy link
Contributor Author

babs commented Feb 23, 2023

still alive

@JoelSpeed JoelSpeed removed the Stale label Feb 23, 2023
@JoelSpeed JoelSpeed mentioned this pull request Mar 5, 2023
Merged
3 tasks
@babs
Copy link
Contributor Author

babs commented Mar 7, 2023 •
edited
Loading

Unix mock is available at unixServerAddr (sock://test.sock)
Help wanted to adapt http tests to add a pass with this mock as backend @JoelSpeed

@github-actions
Copy link
Contributor

github-actions bot commented May 7, 2023

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

@github-actions github-actions bot added the Stale label May 7, 2023
@github-actions github-actions bot closed this May 14, 2023
@babs
Copy link
Contributor Author

babs commented May 14, 2023

I miss the reminder of the bot.
Shouldn't be closed, still alive as far as I know.
Just need a little help to adapt tests to use the mock just like the regular http tests.

@JoelSpeed JoelSpeed reopened this May 29, 2023
@JoelSpeed JoelSpeed removed the Stale label May 29, 2023
JoelSpeed
JoelSpeed reviewed May 29, 2023
View reviewed changes
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Tests wise, lets make sure there's something added to the proxy tests.

You'll need to add a new unix based upstream to the upstreams and then add one or more Entry blocks that exercises that new endpoint

pkg/upstream/http.go Show resolved Hide resolved
pkg/upstream/http.go Show resolved Hide resolved
@github-actions
Copy link
Contributor

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

@github-actions github-actions bot added the Stale label Jul 29, 2023
@babs
Copy link
Contributor Author

babs commented Jul 31, 2023

Review needed still relevant

@github-actions github-actions bot removed the Stale label Aug 1, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2023

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

@github-actions github-actions bot added the Stale label Oct 1, 2023
@github-actions github-actions bot closed this Oct 9, 2023
@babs
Copy link
Contributor Author

babs commented Oct 9, 2023

damn, I missed the reminder @JoelSpeed :/

@kvanzuijlen kvanzuijlen reopened this Oct 9, 2023
@github-actions github-actions bot removed the Stale label Oct 10, 2023
@babs
Copy link
Contributor Author

babs commented Oct 18, 2023

Knowing that test have been added, is there anything else to do to see this PR merged ?

@tuunit
Copy link
Member

tuunit commented Oct 20, 2023

@babs I'll try to find some time to test it on the weekend :)

@tuunit tuunit added the LGTM PR is ready to merge label Oct 24, 2023
JoelSpeed
JoelSpeed reviewed Oct 25, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@babs babs force-pushed the support-unix-socket-upstream branch from a721672 to 06a8424 Compare October 25, 2023 19:19
@babs
Copy link
Contributor Author

babs commented Oct 25, 2023

@tuunit @JoelSpeed rebased, changelog updated ;)

JoelSpeed
JoelSpeed approved these changes Oct 26, 2023
View reviewed changes
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Thanks @babs

@JoelSpeed JoelSpeed merged commit 70571d9 into oauth2-proxy:master Oct 26, 2023
8 checks passed
@BrewTestBot BrewTestBot mentioned this pull request Feb 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoelSpeed JoelSpeed JoelSpeed approved these changes

Assignees
No one assigned
Labels
changelog docs go LGTM PR is ready to merge tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: support unix socket as upstream
4 participants
@babs @JoelSpeed @tuunit @kvanzuijlen