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

Ignored port from config in Windows #38

Closed
inaneverb opened this issue Sep 21, 2021 · 11 comments
Closed

Ignored port from config in Windows #38

inaneverb opened this issue Sep 21, 2021 · 11 comments

Comments

@inaneverb
Copy link

Windows 11 Pro N, 21H2
Tern: 1.12.5
PostgreSQL 13.4 in Docker for Windows.
Docker uses WSL2 as a backend.
WSL OS: Fedora Server 34.
Port mapping in docker enabled: 5432 container -> 5431 host.

I built two different binaries: for unix:amd/64, windows:amd/64.
If I run an unix's binary it's OK. It can connect, do necessary work, etc.
If I run window's binary I getting an error "Failed to connect" and I see a default pg port: 5432, not those which is specified in config.


Output:

iyuryevich@ElevenStation ~/Development/___/migrations/ru (master+*?) $ tern.exe status
Unable to connect to PostgreSQL:
  failed to connect to `host=127.0.0.1 user=test1 database=test_dev_01`: dial error (dial tcp 127.0.0.1:5432: connectex: No connection could be made because the target machine actively refused it.)
iyuryevich@ElevenStation ~/Development/___/migrations/ru (master+*?) $ tern status
status:   up to date
version:  4 of 4
host:     127.0.0.1
database: test_dev_01
iyuryevich@ElevenStation ~/Development/___/migrations/ru (master+*?) $ tern.exe version
tern v1.12.5
iyuryevich@ElevenStation ~/Development/___/migrations/ru (master+*?) $ tern version
tern v1.12.5
iyuryevich@ElevenStation ~/Development/___/migrations/ru (master+*?) $

tern.conf

[database]
host = 127.0.0.1
port = 5431
database = test_dev_01
user = test1
password = root
version_table = schema_version
@jackc
Copy link
Owner

jackc commented Sep 25, 2021

That's very strange. I don't have convenient access to test this on Windows, but I don't see what could be different between *nix and Windows here.

@lucasgpalves
Copy link

It seems that the problem you are experiencing is due to the port setting in your tern configuration. The error indicates that the connection is trying to use the default port (5432), ignoring the specified port (5431). This can occur for various reasons, including the way environment variables are interpreted or how the connection string is configured.

[database]
user=test1 
password=root
conn_string = host=127.0.0.1 port=5431 dbname=test_dev_01  connect_timeout=10

@smainz
Copy link
Contributor

smainz commented Sep 5, 2024

I can confirm that bug (currently on Windows): If sslmode is not set, tern does not pick up port in tern.conf or command line
If you set sslmode = prefer tern picks up the port both in tern.conf or on the command line.

@smainz
Copy link
Contributor

smainz commented Sep 15, 2024

@jackc I think I found the root cause for this error.

The port is properly read from config file or cli argument, but when a connectionis created here:

tern/main.go

Lines 1062 to 1063 in 160bdb3

if connConfig, err := pgx.ParseConfig(""); err == nil {
config.ConnConfig = *connConfig

a fallback connection is created as well.

In the next steps some config arguments are overwritten with args from file or args from cli

tern/main.go

Line 1082 in 160bdb3

err := appendConfigFromFile(config, configFile)

and

tern/main.go

Line 1098 in 160bdb3

err := appendConfigFromCLIArgs(config)

The config is still correct, but the port is not set in Config.Fallbacks[0]:
image

If pgx.ConnectConfig() is not able to connect to the database with the config parameters (it looks like on Windows (did not test Linux) empty sslmode is invalid) it uses the fallback.
The result is an error message containing the port from the fallback, as the fallback points to the wrong port.

Incase c.SslMode is set this problem does not show up, as the PG_* env vars are set:

tern/main.go

Lines 155 to 175 in 160bdb3

switch c.SslMode {
case "disable", "allow", "prefer", "require", "verify-ca", "verify-full":
if err := os.Setenv("PGPORT", strconv.Itoa(int(c.ConnConfig.Port))); err != nil {
return nil, err
}
if err := os.Setenv("PGHOST", c.ConnConfig.Host); err != nil {
return nil, err
}
if err := os.Setenv("PGSSLMODE", c.SslMode); err != nil {
return nil, err
}
if err := os.Setenv("PGSSLROOTCERT", c.SslRootCert); err != nil {
return nil, err
}
if cc, err := pgx.ParseConfig(""); err == nil {
c.ConnConfig.TLSConfig = cc.TLSConfig
c.ConnConfig.Fallbacks = cc.Fallbacks
} else {
return nil, err
}

and the Fallbacks are copied to the config.

To solve this, I see these options:

  1. Set a default value for sslmode (e.g. prefered) in main.go 362

    tern/main.go

    Line 362 in 160bdb3

    cmd.Flags().StringVarP(&cliOptions.sslmode, "sslmode", "", "", "SSL mode")
  2. disallow empty sslmode here:

    tern/main.go

    Lines 131 to 132 in 160bdb3

    case "", "disable", "allow", "prefer", "require", "verify-ca", "verify-full":
    // okay

    to show an error message about invalid sslmode
  3. Add "" as valid sslmode here:

    tern/main.go

    Lines 155 to 156 in 160bdb3

    switch c.SslMode {
    case "disable", "allow", "prefer", "require", "verify-ca", "verify-full":
  4. Extract port and host from file, env and cli before calling pgx.Connect("") ans set env vars PG_*here:

    tern/main.go

    Line 1062 in 160bdb3

    if connConfig, err := pgx.ParseConfig(""); err == nil {
  5. copy port to the Falbacks before trying to connect

I consider a rewrite of LoadConfig to first load the arguments, merge them and then create the config the cleanest approach, but the one with the most changes.

Do you have an opinion about which options is best?

I would prepare the PR.

jackc added a commit that referenced this issue Sep 21, 2024
If the sslmode was blank then some of the fallback config was not being
set properly. This was because blank is treated as "prefer" by pgx
(which follows libpq's behavior). But tern parsing parsing the config
in several places and then trying to fix the fallback config. It missed
the default "prefer" case.

The solution was to avoid the multiple parsing and fallback config logic
entirely. Instead, we set the PG* environment variables from the CLI
options and config file before parsing the conn string.

#38
@jackc
Copy link
Owner

jackc commented Sep 21, 2024

Thanks for the in-depth analysis.

I consider a rewrite of LoadConfig to first load the arguments, merge them and then create the config the cleanest approach, but the one with the most changes.

I think this is the best approach. Take a look at #107.

@smainz
Copy link
Contributor

smainz commented Sep 25, 2024

Thanks a lot , #107 looks like it does what i need.

Do you need my help on anything?

@jackc
Copy link
Owner

jackc commented Sep 26, 2024

Thanks a lot , #107 looks like it does what i need.

Just merged it into master.

Do you need my help on anything?

If you can test master and make sure it works that would help.

@smainz
Copy link
Contributor

smainz commented Sep 28, 2024

I can confirm this works as expected:

  • with config file it picks up the port from the file
  • with cli argument it picks up the port from the command line
  • with config file and cli argument the cli argument overrides the port in the config file
  • with env var PGPORT set it picks up the port from PGPORT
  • with env var PGPORT set and cli argument the cli argument overrides the env var

I am unsure about this:

  • with PGPORT set and config file the config file overrides the env var.
    Usually I'd expect it the other way round, but I am not sure about conventions in postgres tooling.

@jackc
Copy link
Owner

jackc commented Sep 28, 2024

I'm not sure either. My expectation is that the priority is CLI arg, config file, then envvar. But I'm not sure where that idea came from. It may or may not be standard expectation.

@smainz
Copy link
Contributor

smainz commented Sep 28, 2024

My expectation is different (highest priority first):

  1. cli arg
  2. env var
  3. config file

The rational is: The more ad-hoc the parameter can be set / the more ephermal it is the higher the priority.
For one-off use cases I would not edit the config, but just use cli arg or if I need to call terntwice use an env var.

But expectations may be different. Let's keep it the way it is, usually I use config file and cli args,so env vars is an edge case for me.

Thanks for your effort.

@jackc
Copy link
Owner

jackc commented Sep 29, 2024

👍

Just tagged v2.2.2 with this fix.

@jackc jackc closed this as completed Sep 29, 2024
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

No branches or pull requests

4 participants