jackc / pgx

PostgreSQL driver and toolkit for Go
MIT License
10.8k stars 838 forks source link

No retry when pg_hba.conf error when using sslmode=allow or prefer #1581

Open wenlive opened 1 year ago

wenlive commented 1 year ago

in postgres Documentation

sslmode
This option determines whether or with what priority a secure SSL TCP/IP connection will be negotiated with the server. There are six modes:

disable
only try a non-SSL connection

allow
first try a non-SSL connection; if that fails, try an SSL connection

prefer (default)
first try an SSL connection; if that fails, try a non-SSL connection

VERSION: I'm using pgx/v4 v4.16.1

Detail: When using sslmode=allow to establish a connection, according to the documentation, it should first try to establish a non-ssl connection and then try to establish an ssl connection after failure. In the implementation, the ConnectConfig function will handle some special errors to skip subsequent retries.

       for _, fc := range fallbackConfigs {
        pgConn, err = connect(ctx, config, fc)
        if err == nil {
            break
        } else if pgerr, ok := err.(*PgError); ok {
            err = &connectError{config: config, msg: "server error", err: pgerr}
            const ERRCODE_INVALID_PASSWORD = "28P01"                    // wrong password
            const ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION = "28000" // wrong password or bad pg_hba.conf settings
            const ERRCODE_INVALID_CATALOG_NAME = "3D000"                // db does not exist
            const ERRCODE_INSUFFICIENT_PRIVILEGE = "42501"              // missing connect privilege
            if pgerr.Code == ERRCODE_INVALID_PASSWORD ||
                pgerr.Code == ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION ||
                pgerr.Code == ERRCODE_INVALID_CATALOG_NAME ||
                pgerr.Code == ERRCODE_INSUFFICIENT_PRIVILEGE {
                break
            }
        }
    }

When the previous attempt triggers a bad pg_hba.conf settings error, there will be no subsequent retries, but in actual use, occasionally it is necessary to set pg_hba.conf to block all non-ssl connections, and the client in allow mode will be unusable at this time Is there any way that can be used in this scenario?Or should it not break on ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION error?

jackc commented 1 year ago

Huh. That's interesting. That code is trying to match the behavior of the C libpq library when dealing with multiple hosts. There are certain errors that should terminate the attempt instead of trying the next host. I looked around at the PostgreSQL source code and didn't see how INVALID_AUTHORIZATION_SPECIFICATION would apply. However, it was specifically added in https://github.com/jackc/pgconn/pull/68 to resolve another issue with multiple hosts and SSL. Not sure how to reconcile these issues.

wenlive commented 1 year ago

Thanks for the reply, I understand the reason for this design, and I will pay attention to it in the future.

bck01215 commented 1 year ago

Another issue that seems potentially related:

I have a postgres compose stack using pg_auto_failover. In thise scenario, only one host can be read write. If my conn string specifies a host that is currently read-only, I get this error:

2023/06/14 12:34:56 Unable to connect to database: failed to connect to `host=localhost user=ad database=analytics`: server error (FATAL: no pg_hba.conf entry for host "172.20.0.1", user "ad", database "analytics", SSL off (SQLSTATE 28000))

Expected behavior would be to have the it continue until it finds the right host. libpq does this.

My pool:

        // does not work
    dbPool, err := pgxpool.Connect(context.Background(), "postgres://ad:@localhost:5401,localhost:5402,localhost:5403/analytics?target_session_attrs=read-write")
# works
psql -d 'postgres://ad:@localhost:5402,localhost:5401,localhost:5403/analytics?target_session_attrs=read-write'
bck01215 commented 1 year ago

Leaving a comment here, pgconn tries to correct the ssl. analytics?target_session_attrs=read-write will be removed and ssl will be turned off on the first failure. A fix from a user standpoint is to do: analytics?target_session_attrs=read-write&sslmode=require. This does not mirror the C library, but I can understand its implementation. Will continue diving into the code to look for possible solutions that can fix this example without modifying the ssl auto-correction

jackc commented 1 year ago

I think this should be resolved in v5.4.2.