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

Client GSSENC Request Fix #797

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

AndrewJackson2020
Copy link
Contributor

@AndrewJackson2020 AndrewJackson2020 commented Sep 5, 2024

This PR fixes an issue where a client that attempts to connect to pgcat with GSS encoding is rejected with a confusing error message. By default libpq will attempt to connect with GSS encoding if there is a valid kerberos ticket available on the system which can be checked with the klist command. The server should reject this request at which point libpq will attempt to connect again without gss encoding. This behavior is documented in the postgres documentation.

Do not have any automated tests right now, will need to set up a kerberos environment in the container. Can do this a bit later.

Fixes #792

@AndrewJackson2020
Copy link
Contributor Author

Just implemented the automated test with the most recent commit. The test simply tries to connect to the database after a valid kerberos ticket has been made available. Can confirm that this test fails without this patch and passes with it.

@drdrsh
Copy link
Collaborator

drdrsh commented Sep 6, 2024

Can you please run

cargo fmt
# and
cargo clippy --all --all-targets -- -Dwarnings

and commit any required changes.

@AndrewJackson2020
Copy link
Contributor Author

Can you please run

cargo fmt
# and
cargo clippy --all --all-targets -- -Dwarnings

and commit any required changes.

Done

@AndrewJackson2020
Copy link
Contributor Author

I believe that these tests ran locally because the environment created by start_test_env.sh runs on root. It looks like this is not the case with the circlci environment and thus it fails when it tries to write some of the kerberos config files into root space. Further I had to add a few packages to the test docker container but it looks like the circlecli config.yaml does not build the container from scratch but uses the image ghcr.io/postgresml/pgcat-ci:latest.

Given this I am not sure how to make this new test pass CI on my end. Maybe this would best be a local only test? Let me know how you want to proceed.

@drdrsh
Copy link
Collaborator

drdrsh commented Sep 6, 2024

Maybe you can create a PR that updates the pgcat-ci docker image with the changes you want (https://github.com/postgresml/pgcat/blob/main/Dockerfile.ci) and merge that PR

Perhaps you can make start_test_env.sh run as non-root and see if you can reproduce the failure and work around it?

Sorry, for the hassle. The workflow can be improved and I am trying to reduce the friction for future contributors so your help is appreciated!

@AndrewJackson2020
Copy link
Contributor Author

I think what you are saying makes a lot of sense. Will see what I can do.

@AndrewJackson2020
Copy link
Contributor Author

AndrewJackson2020 commented Sep 6, 2024

Looking through the repo there are currently 5 different docker files and 3 different docker compose files.

In particular it seems like there is a lot of overlap between the docker assets in ./tests/docker and ./dev as its seems like they have the same objective of creating a dev/local test environment where tests can be run, pgcat can be built, etc. Does it makes sense to have both of these or consolidate them in some way?

Also its seem like ./dev/Dockerfile and ./tests/docker/Dockerfile both use rust:bullseye while ./Dockerfile.ci uses cimg/rust:1.79.0 as their base images. Does it make sense to have different base images between CI and dev environments? Would be nice if the CI environment and the dev/test environments can be as similar as possible.

Also not seeing any reference to ./Dockerfile.dev in ./README.md, comments, etc. Would be nice to get some sort of mention in the ./README.md or as a comment on ./Dockerfile.dev as to what it is used for and how it is different from ./dev/Dockerfile and ./tests/docker/Dockerfile.

Also any reason why go is not installed in ./dev/Dockerfile?

$ find . | grep docker-compose.yml
dev/docker-compose.yaml
docker-compose.yml
tests/docker/docker-compose.yml
 find . | grep Docker
Dockerfile
Dockerfile.ci
Dockerfile.dev
dev/Dockerfile
tests/docker/Dockerfile

@drdrsh
Copy link
Collaborator

drdrsh commented Sep 6, 2024

Yes, this is actually one of the things that I really wanted to fix for the longest time.

For Dockerfile, I think we only really need an official one that is used to build the official image. I think we created the Dockerfile.ci to make running tests faster/cleaner. That's the only files we really need.

As for docker-compose.yml files, we probably only need one for running integration tests and for local dev.

So the grand total should be, two Dockerfiles and one docker-compose.yml

I was planning to add another docker-compose file but that would be for benchmarking so I am thinking I could add a benchmarking directory and puts this setup under it. But for now, yea we only need 2 docker files and one docker compose

@AndrewJackson2020
Copy link
Contributor Author

Agree, would definitely help DRY out the repo and improve developer experience.

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

Successfully merging this pull request may close these issues.

PGcat raises confusing error when client attempts to connect with GSSAPI encoding request
2 participants