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

Sticky Sessions: Tolerate ClickHouse Session ID Mechanism (Better Tests Coverage) + FIPS Support based on BoringSSL #373

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

pavelnemirovsky
Copy link
Contributor

@pavelnemirovsky pavelnemirovsky commented Nov 16, 2023

Scope of change;

  1. Reopened pull request based on Session_timeout bug fix & Local Build Process fixup #119 and continuation of Sticky Sessions: Tolerate ClickHouse Session ID Mechanism #117.
  2. Add make release-build-docker-fips to be able to build FIPS version of ClickHouse Proxy

@pavelnemirovsky pavelnemirovsky changed the title Session_timeout bug fix & Local Build Process fixup Sticky Sessions: Tolerate ClickHouse Session ID Mechanism (Better Tests Coverage) Nov 17, 2023
@pavelnemirovsky
Copy link
Contributor Author

Guys, I am going to add in this PR FIPS 140-2 support based on Borring SSL as well. (Don't review yet)

@pavelnemirovsky pavelnemirovsky changed the title Sticky Sessions: Tolerate ClickHouse Session ID Mechanism (Better Tests Coverage) Sticky Sessions: Tolerate ClickHouse Session ID Mechanism (Better Tests Coverage) + FIPS Support based on BoringSSL Nov 17, 2023
@pavelnemirovsky
Copy link
Contributor Author

@sigua-cs PING

@mga-chka
Copy link
Collaborator

mga-chka commented Nov 20, 2023

FYI, don't mind the ci error. Your PR is correct and pass the tests, it just that I broke the ci recently to improve our code coverage tracking.
All the maintainers are a bit busy so we might take time to review the PR.

@pavelnemirovsky
Copy link
Contributor Author

pavelnemirovsky commented Nov 21, 2023

@mga-chka understood, thx let's try to push forward since the originally provided functionally is not properly covered by tests now.

@Blokje5
Copy link
Collaborator

Blokje5 commented Nov 24, 2023

@pavelnemirovsky if I understand correctly this is two PRs in one:

  1. CH Sticky Sessions
  2. Fips support

Would you be able to seperate the PRs? That leads to a cleaner commit history and will make it easier to review.

@pavelnemirovsky
Copy link
Contributor Author

@Blokje5 Practically speaking, it is one PR PR since FIPS 140-2 is just a mode in Makefile that allows to compile ClickHouse Proxy with BorringSSL, so I suggest merging it that way.

@pavelnemirovsky
Copy link
Contributor Author

Do you want me to add some documentation regarding the ability to build FIPS version of the app?

@Blokje5
Copy link
Collaborator

Blokje5 commented Nov 29, 2023

@pavelnemirovsky If you would add some documentation regarding the new build that would be great 👍 .

@pavelnemirovsky
Copy link
Contributor Author

@sigua-cs @Blokje5 @mga-chka documentation has been updated. Please check, and let's merge it.

@pavelnemirovsky
Copy link
Contributor Author

@sigua-cs @Blokje5 @mga-chka PING

@mga-chka
Copy link
Collaborator

mga-chka commented Jan 3, 2024

Sorry for the long delay but we were all off. We'll have a look this week. Can you fix the conflict while we review the PR?

@mga-chka
Copy link
Collaborator

mga-chka commented Jan 3, 2024

Hi, I took a first look.
The go part regarding the sessionId looks ok.
I'm quite rusty regarding docker so I might be wrong (the last time I touched a docker file was 8 years ago). But, from what I get, there are some problems:

  • you're fixing the go version to 1.21 whereas we're setting another value in the release workflow for the moment the version is 1.19, so there is no issue, but the day we need a 1.22+, we'll likely produce failing docker images since there are no tests relating to docker image generation
  • the change in the Makefile makes the file Dockerfile not used anymore, why keeping it?
  • in the 2 new Dockerfiles, you rely on a golang:${GO_VER}-alpine image, which is experimental cf docker hub This variant is highly experimental, and not officially supported by the Go project (see [golang/go#19938](https://github.com/golang/go/issues/19938) for details). whereas the previous image we were using a rock solid debian image
  • the new images don't install the packages ca-certificates and curl, are they already included in the alpine image?

@mga-chka
Copy link
Collaborator

@pavelnemirovsky (I'm pinging you just in case you didn't see my previous msgs)

@sconnole
Copy link

sconnole commented Oct 17, 2024

I created a new PR for this issue here: #481

UPDATE

Pavel gave me write permission, so all changes will be here.

Added tests for dockerfile to ensure that GoVersions in dockerfiles stay in sync with Go.mod version.
Updated dockerfile to use a more reliable base image.
Install ca-certificates and curl
@sconnole
Copy link

@sigua-cs @Blokje5 @mga-chka Pinging this on behalf of Pavel

@mga-chka
Copy link
Collaborator

FYI we're in the middle of the holiday in France so most of the maintainer are off, which is why we're not very responsive.
Expect a review in 1-3 weeks (because we will have our yearly contentsquare hackathon once we're not off)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants