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

add auth_query_database config #521

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

Conversation

sebastianwebber
Copy link
Contributor

@sebastianwebber sebastianwebber commented Jul 20, 2023

This PR adds the new config auth_query_database to allow a different database to get the hashes from the database instance. This is useful to avoid recreating the lookup function between all databases the pooler connects.

I updated the example folder with functional code to validate the new feature and updated the config docs.

Can you guys give me some tips about running the ruby specs locally so I can add a new commit to test this change?

@levkk
Copy link
Contributor

levkk commented Jul 20, 2023

Hey, cool PR.

To run the specs locally, you can install Docker & docker-compose & run:

bash dev/script/console

# Inside the container
cargo build
cd tests/ruby
bundle install
bundle exec rspec *_spec.rb
# Or a specific spec you want to run

@sebastianwebber sebastianwebber force-pushed the sebastian/add-query-auth-db branch from b64668a to c3e3b34 Compare July 20, 2023 22:29
This commit adds the new config auth_query_database to allow use a
different database to get the hashes from the database instance.

I updated the example folder with functional code to validate the new
feature and updated the config docs.

Also updated ruby code to use the new env var LOG_LEVEL.

Signed-off-by: Sebastian Webber <[email protected]>
@sebastianwebber sebastianwebber force-pushed the sebastian/add-query-auth-db branch from c3e3b34 to cdd03e7 Compare July 24, 2023 15:00
Copy link
Contributor

@levkk levkk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good to me. Let me know if it's ready to merge.

@sebastianwebber
Copy link
Contributor Author

hey @levkk still didn't have time to add more tests in the ruby code. =/

i did try to change the existing tests/ruby/auth_query_spec.rb but it is not failing in the simple changes that I made. Something like this:

diff --git a/tests/ruby/auth_query_spec.rb b/tests/ruby/auth_query_spec.rb
index efa5ee5..532a82c 100644
--- a/tests/ruby/auth_query_spec.rb
+++ b/tests/ruby/auth_query_spec.rb
@@ -57,6 +57,24 @@ describe "Auth Query" do
         )
       end
 
+      context "with different lookup database" do
+        let(:config) {
+          { "general" => {
+            "auth_query" => "SELECT * FROM public.user_lookup2('$1');",
+            "auth_query_user" => "md5_auth_use2r",
+            "auth_query_password" => "secret",
+            "auth_query_database" => "lookup_db", ## doesn't exist yet
+          } }
+        }
+
+        it "it uses obtained passwords" do
+          connection_string = processes.pgcat.connection_string("sharded_db", pg_user["username"])
+          conn = PG.connect(connection_string)
+
+          expect(conn.exec("SELECT 1 + 2")).not_to be_nil
+        end
+      end
+
       context "with correct global parameters" do
         let(:config) { { "general" => { "auth_query" => "SELECT * FROM public.user_lookup('$1');", "auth_query_user" => "md5_auth_user", "auth_query_password" => "secret" } } }
         context "and with cleartext passwords set" do
diff --git a/tests/ruby/helpers/auth_query_helper.rb b/tests/ruby/helpers/auth_query_helper.rb
index 60e8571..5b06131 100644
--- a/tests/ruby/helpers/auth_query_helper.rb
+++ b/tests/ruby/helpers/auth_query_helper.rb
@@ -1,3 +1,5 @@
+require "awesome_print"
+
 module Helpers
   module AuthQuery
     def self.single_shard_auth_query(
@@ -44,7 +46,7 @@ module Helpers
       pgcat_cfg["general"]["port"] = pgcat.port
       pgcat.update_config(pgcat_cfg)
       pgcat.start
-      
+
       pgcat.wait_until_ready(
         pgcat.connection_string(
           "sharded_db",
@@ -53,6 +55,7 @@ module Helpers
         )
       ) if wait_until_ready
 
+      pp pgcat.current_config
       OpenStruct.new.tap do |struct|
         struct.pgcat = pgcat
         struct.primary = primary
@@ -98,7 +101,7 @@ module Helpers
             },
           },
           "users" => { "0" => user.merge(config_user) }
-        }                                  
+        }
       end
       # Main proxy configs
       pgcat_cfg["pools"] = {
@@ -109,7 +112,6 @@ module Helpers
       pgcat_cfg["general"]["port"] = pgcat.port
       pgcat.update_config(pgcat_cfg.deep_merge(extra_conf))
       pgcat.start
-      
       pgcat.wait_until_ready(pgcat.connection_string("sharded_db0", pg_user['username'], pg_user['password']))
 
       OpenStruct.new.tap do |struct|

Since the db doesn't exist, I'm expecting some errors but is saying that is ok:

pgcat@dev-container:~/tests/ruby (sebastian/add-query-auth-db)$ bundle exec rspec --format documentation auth_query_spec.rb  -e "with different lookup database"
Run options: include {:full_description=>/with\ different\ lookup\ database/}

Auth Query
  when auth_query is configured
    with global configuration
      with different lookup database
{"general"=>
  {"admin_password"=>"admin_pass",
   "admin_username"=>"admin_user",
   "auth_query"=>"SELECT * FROM public.user_lookup2('$1');",
   "auth_query_database"=>"lookup_db",
   "auth_query_password"=>"secret",
   "auth_query_user"=>"md5_auth_use2r",
   "autoreload"=>15000,
   "ban_time"=>60,
   "connect_timeout"=>5000,
   "enable_prometheus_exporter"=>false,
   "healthcheck_delay"=>30000,
   "healthcheck_timeout"=>1000,
   "host"=>"0.0.0.0",
   "idle_client_in_transaction_timeout"=>0,
   "idle_timeout"=>30000,
   "log_client_connections"=>false,
   "log_client_disconnections"=>false,
   "port"=>32431,
   "prepared_statements"=>true,
   "prepared_statements_cache_size"=>500,
   "prometheus_exporter_port"=>9930,
   "server_lifetime"=>86400000,
   "server_tls"=>false,
   "shutdown_timeout"=>60000,
   "tcp_keepalives_count"=>5,
   "tcp_keepalives_idle"=>5,
   "tcp_keepalives_interval"=>5,
   "verify_server_certificate"=>false,
   "worker_threads"=>5},
 "plugins"=>
  {"intercept"=>
    {"enabled"=>true,
     "queries"=>
      {"0"=>
        {"query"=>
          "select current_database() as a, current_schemas(false) as b",
         "result"=>[["${DATABASE}", "{public}"]],
         "schema"=>[["a", "text"], ["b", "text"]]},
       "1"=>
        {"query"=>"select current_database(), current_schema(), current_user",
         "result"=>[["${DATABASE}", "public", "${USER}"]],
         "schema"=>
          [["current_database", "text"],
           ["current_schema", "text"],
           ["current_user", "text"]]}}},
   "prewarmer"=>
    {"enabled"=>false, "queries"=>["SELECT pg_prewarm('pgbench_accounts')"]},
   "query_logger"=>{"enabled"=>false},
   "table_access"=>
    {"enabled"=>false, "tables"=>["pg_user", "pg_roles", "pg_database"]}},
 "pools"=>
  {"sharded_db"=>
    {"default_role"=>"any",
     "load_balancing_mode"=>"random",
     "pool_mode"=>"transaction",
     "primary_reads_enabled"=>false,
     "query_parser_enabled"=>false,
     "sharding_function"=>"pg_bigint_hash",
     "shards"=>
      {"0"=>
        {"database"=>"shard0",
         "servers"=>
          [["localhost", "15432", "primary"],
           ["localhost", "20432", "replica"]]}},
     "users"=>
      {"0"=>
        {"password"=>"sharding_user",
         "pool_size"=>10,
         "statement_timeout"=>0,
         "username"=>"sharding_user"}}}}}
        it uses obtained passwords

Finished in 1.07 seconds (files took 1.36 seconds to load)
1 example, 0 failures

I hope to have some time over the weekend to figure out what is wrong so I can add the missing tests and finish the PR.

context 'when auth_query is configured' do
context 'with global configuration' do
context "when auth_query is configured" do
context "with global configuration" do
around(:example) do |example|

# Set up auth query
Helpers::AuthQuery.set_up_auth_query_for_user(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do those methods actually reload pgcat?

Signed-off-by: Sebastian Webber <[email protected]>
@sebastianwebber sebastianwebber force-pushed the sebastian/add-query-auth-db branch from 7b9fa60 to e41ed75 Compare August 7, 2023 18:30
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.

4 participants