chproxy icon indicating copy to clipboard operation
chproxy copied to clipboard

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

Open pavelnemirovsky opened this issue 1 year ago • 14 comments

Scope of change;

  1. Reopened pull request based on https://github.com/ContentSquare/chproxy/pull/119 and continuation of https://github.com/ContentSquare/chproxy/pull/117.
  2. Add make release-build-docker-fips to be able to build FIPS version of ClickHouse Proxy

pavelnemirovsky avatar Nov 16 '23 17:11 pavelnemirovsky

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

pavelnemirovsky avatar Nov 17 '23 09:11 pavelnemirovsky

@sigua-cs PING

pavelnemirovsky avatar Nov 19 '23 08:11 pavelnemirovsky

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 avatar Nov 20 '23 16:11 mga-chka

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

pavelnemirovsky avatar Nov 21 '23 15:11 pavelnemirovsky

@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.

Blokje5 avatar Nov 24 '23 10:11 Blokje5

@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 avatar Nov 29 '23 11:11 pavelnemirovsky

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

pavelnemirovsky avatar Nov 29 '23 11:11 pavelnemirovsky

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

Blokje5 avatar Nov 29 '23 13:11 Blokje5

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

pavelnemirovsky avatar Dec 17 '23 17:12 pavelnemirovsky

@sigua-cs @Blokje5 @mga-chka PING

pavelnemirovsky avatar Dec 18 '23 21:12 pavelnemirovsky

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 avatar Jan 03 '24 16:01 mga-chka

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 avatar Jan 03 '24 23:01 mga-chka

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

mga-chka avatar Jan 15 '24 13:01 mga-chka