chproxy
chproxy copied to clipboard
Sticky Sessions: Tolerate ClickHouse Session ID Mechanism (Better Tests Coverage) + FIPS Support based on BoringSSL
Scope of change;
- Reopened pull request based on https://github.com/ContentSquare/chproxy/pull/119 and continuation of https://github.com/ContentSquare/chproxy/pull/117.
- Add
make release-build-docker-fips
to be able to build FIPS version of ClickHouse Proxy
Guys, I am going to add in this PR FIPS 140-2 support based on Borring SSL as well. (Don't review yet)
@sigua-cs PING
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.
@mga-chka understood, thx let's try to push forward since the originally provided functionally is not properly covered by tests now.
@pavelnemirovsky if I understand correctly this is two PRs in one:
- CH Sticky Sessions
- Fips support
Would you be able to seperate the PRs? That leads to a cleaner commit history and will make it easier to review.
@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.
Do you want me to add some documentation regarding the ability to build FIPS version of the app?
@pavelnemirovsky If you would add some documentation regarding the new build that would be great 👍 .
@sigua-cs @Blokje5 @mga-chka documentation has been updated. Please check, and let's merge it.
@sigua-cs @Blokje5 @mga-chka PING
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?
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 hubThis 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
andcurl
, are they already included in the alpine image?
@pavelnemirovsky (I'm pinging you just in case you didn't see my previous msgs)