-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Added support for Unix Domain Sockets in Pomerium Reverse Proxy #7772
base: master
Are you sure you want to change the base?
Added support for Unix Domain Sockets in Pomerium Reverse Proxy #7772
Conversation
@biru-codeastromer Are you sure the communication goes through unix sockets and not the 8080 TCP port? I'd expect unix:///run/jenkins/jenkins.socket to come up in the config instead of http://jenkins:8080, but I have no experience with Pomerium. @cmo-pomerium could you please review this? |
Thank you for your observation, @zbynek Sir! I initially configured the route to use
Due to this, I reverted to using If this fallback behavior is acceptable, I will document it explicitly in the PR. However, I would appreciate further input, especially from @cmo-pomerium, to confirm whether this is an expected limitation or if there's a recommended workaround to enable Unix domain socket communication. Also Sir @kmartens27 may you please review and guide how to improve this .Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks very much for the work here @biru-codeastromer. I have provided some feedback on documentation formatting/grammar/other areas, but also wanted to question whether there was any testing done on lts-jdk17. As Jenkins does not suppport Java 11, it would be better to utilize a supported LTS version within the code examples provided. If you have not had a chance to test on the latest LTS, I would suggest updating everything and running through the process to ensure that it would work on a current LTS. I recognize that there are other mentions of jdk11 on the page, but these will need to be updated as well (outside of this PR). If your tests work, the other areas of this page can be updated accordingly.
Please share if you have any questions on this!
...istration/reverse-proxy-configuration-with-jenkins/reverse-proxy-configuration-pomerium.adoc
Outdated
Show resolved
Hide resolved
|
||
Starting from Jenkins version 2.452.1 , Jenkins supports Unix domain sockets. This can be particularly useful for improving security and performance. | ||
|
||
### Prerequisites |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### Prerequisites | |
=== Prerequisites |
Suggestion to use the same type of formatting as elsewhere on the page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll update the header formatting for consistency with the rest of the document by changing it.
...istration/reverse-proxy-configuration-with-jenkins/reverse-proxy-configuration-pomerium.adoc
Outdated
Show resolved
Hide resolved
...istration/reverse-proxy-configuration-with-jenkins/reverse-proxy-configuration-pomerium.adoc
Outdated
Show resolved
Hide resolved
### Configuration Steps | ||
|
||
## 1. Update Pomerium Configuration : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### Configuration Steps | |
## 1. Update Pomerium Configuration : | |
=== Configuration steps | |
==== Update you Pomerium configuration: |
Adjust header formatting to match the rest of the page, remove number from step to match rest of the page formatting, ensure the step is a smaller heading size than the Configuration Steps header. Adjust to use sentence case for header.
If these are all steps to configure, they may not need their own headers. They can be listed under the configuration steps header with their respective order (1 -> 2 -> 3) or smaller headers if they have a bunch of information attached, but since steps 3 and 4 have only one sentence, using the steps as headers is unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Sir for the feedback. I’ll update the headers to be smaller and ensure they are consistent with the rest of the page, combining some of the steps to make the flow more natural.
is: username | ||
``` | ||
|
||
## 2. Configure Jenkins : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## 2. Configure Jenkins : | |
==== Configure Jenkins: |
Adjusting formatting to be aligned with the rest of the page/site. Again, if these steps are all part of the Configuration Steps header, they do not need to be their own separate headings and could just be listed steps underneath "Configuration Steps".
...istration/reverse-proxy-configuration-with-jenkins/reverse-proxy-configuration-pomerium.adoc
Outdated
Show resolved
Hide resolved
- /var/run/jenkins:/var/run/jenkins | ||
``` | ||
|
||
## 3. Restart Services : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## 3. Restart Services : | |
==== Restart services: |
Adjusting formatting and capitalization for sentence case.
This is very minimal content and would not need its own header section. This would fit better a listed step with the other steps underneath Configuration Steps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. I will remove the header for "Restart Services" and integrate it into the rest of the steps under "Configuration Steps."
### Verification | ||
|
||
After restarting the services, verify that Jenkins is accessible through the Unix domain socket by navigating to your Jenkins URL (e.g., `https://jenkins.example.com`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### Verification | |
After restarting the services, verify that Jenkins is accessible through the Unix domain socket by navigating to your Jenkins URL (e.g., `https://jenkins.example.com`). | |
==== Verification | |
After restarting the services, verify that Jenkins is accessible through the Unix domain socket by navigating to your Jenkins URL. |
Same suggestion for the header as above, I would format these into a list of steps instead of multiple headers, especially with the last two not having enough content to justify such formatting.
In terms of the last part, avoid using e.g. when possible and if possible, the URL should be something used within the examples like https://jenkins.localhost.pomerium.io or http://jenkins:8080 based on the context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure Sir!
jenkins: | ||
networks: | ||
main: {} | ||
image: jenkins/jenkins:lts-jdk11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also be cautious to use lts-jdk11 as we do not support Java 11 any longer. If this documentation needs to be updated overall to use lts-jdk17 that can be a separate PR, but I would advise testing using a supported version.
Thank you very much @kmartens27 Sir for the detailed feedback and suggestions, and I appreciate the recognition for the work so far. Regarding the testing with lts-jdk17, I haven't had a chance to test the setup with it yet. I agree that using a supported LTS version would be ideal, so I will update the configuration and run through the process with Java 17 LTS to ensure compatibility. I will also make sure to address the mentions of jdk11 on the page and update them accordingly. Once I complete the tests and update the documentation, I will submit a follow-up to reflect those changes. If I encounter any issues or have questions, I will reach out for clarification. Thanks again for your helpful input! |
Thanks very much @biru-codeastromer, I appreciate it and please let me know if there are any questions or concerns about anything I've shared. |
Co-authored-by: Kevin Martens <[email protected]>
Thank you so much for your detailed feedback and helpful suggestions @kmartens27 Sir. I’ve committed the smaller changes related to formatting, grammar, and other one-liner suggestions. For the larger changes of suggested commits, I will work on them now and ensure all the suggestions are fully implemented. Once I complete the tests and updates, I’ll keep you updated if any issues arise, and I appreciate your continued support. Thanks again for your time and feedback! |
Hi Sir, I’ve reviewed the changes, but I wasn’t able to identify the cause of the failure. Could you please help clarify why this might be happening? If there’s anything I can do to assist in debugging the issue, I’m happy to help. Looking forward to your guidance. |
Hi @biru-codeastromer, I'm not sure why that check is failing so I have asked in the jenkins-infra gitter channel. It seems to be something that has started happening over the last few days, but I'm not sure if it is the result of something else being added/removed so I'm hoping that someone from the infra team or in the channel will have insight as to what it might be or how it can be tested. I've provided the link so you can follow along if you'd like. |
Thanks Sir for the help ! Much appreciated . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kmartens27 do you think the content of this PR is valid? As mentioned above I'm not sure that the new config actually makes use of Unix Domain Sockets, and don't even know if/how Pomerium supports them. There's no point having two descriptions of using Pomerium with TCP ports on the same page.
@zbynek I sent you a separate question by email. If you didn't receive it, could you send me email at [email protected] (Apologies for misusing a pull request comment for something unrelated to the pull request) |
Fixes #6893
Changes Made:
config.yaml
anddocker-compose.yaml
in the documentation of unix domain sockets part to demonstrate the new configuration.Testing Done:
1. Initial Setup:
docker-compose.yaml
with Jenkins and Pomerium configurations.http://localhost:8080
.2. Pomerium Configuration:
config.yaml
with placeholders for sensitive information.config.yaml
.3. Integration and Running Services:
docker-compose.yaml
to include Pomerium, linked with Jenkins using Unix domain sockets.https://jenkins.localhost.pomerium.io
.4. JWT Authentication:
5. Verification:
https://verify.localhost.pomerium.io
.6. Authorization Testing:
7. Functionality Testing:
Please review and provide feedback.
Additional note -
Removed the version field from docker-compose.yaml as it is no longer required with newer versions of Docker Compose and caused errors during local testing.