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

Integrate upstream hostaddr/load balancing support #22

Merged
merged 3 commits into from
Mar 25, 2024
Merged

Conversation

benesch
Copy link
Member

@benesch benesch commented Mar 25, 2024

This is more flexible than our previous approach to allowing the TLS verify host to be overridden. In particular, users can specify multiple IPs to try for the same host, e.g.:

Config::new()
  .host("my-pg-server")
  .hostaddr(1.2.3.1)
  .host("my-pg-server")
  .hostaddr(1.2.3.2)
  .host("my-pg-server")
  .hostaddr(1.2.3.3)

Copy link

@rjobanp rjobanp left a comment

Choose a reason for hiding this comment

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

I have some feedback on the API change but the rest of the code logic & tests LGTM

Comment on lines +47 to +59
/// * `hostaddr` - Numeric IP address of host to connect to. This should be in the standard IPv4 address format,
/// e.g., 172.28.40.9. If your machine supports IPv6, you can also use those addresses.
/// If this parameter is not specified, the value of `host` will be looked up to find the corresponding IP address,
/// - or if host specifies an IP address, that value will be used directly.
/// Using `hostaddr` allows the application to avoid a host name look-up, which might be important in applications
/// with time constraints. However, a host name is required for verify-full SSL certificate verification.
/// Specifically:
/// * If `hostaddr` is specified without `host`, the value for `hostaddr` gives the server network address.
/// The connection attempt will fail if the authentication method requires a host name;
/// * If `host` is specified without `hostaddr`, a host name lookup occurs;
/// * If both `host` and `hostaddr` are specified, the value for `hostaddr` gives the server network address.
/// The value for `host` is ignored unless the authentication method requires it,
/// in which case it will be used as the host name.
Copy link

Choose a reason for hiding this comment

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

this API change feels a little brittle/confusing to me and I'm wondering if we could rework it to be more straightforward somehow. It just seems awkward that there is in implicit order-based mapping between two separate parameters. Is there a reason not to change the semantics of the host parameter itself?

One idea would be to use : to specify host->IP mapping in the host parameter, so you could do something like this:

host=host1:127.0.0.1:127.0.0.2,host2:127.0.0.3,host3

specifying that host1 maps to the first two IPs, and host2 maps to the 3rd, and host3 should be resolved itself

Comment on lines 224 to +225
pub(crate) host: Vec<Host>,
pub(crate) hostaddr: Vec<IpAddr>,
Copy link

Choose a reason for hiding this comment

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

similar to the comment on the string parameters, it seems that it would be a lot cleaner to add a new enum type to the Host definition that tied together the mappings more clearly:

/// A host specification.
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum Host {
    /// A TCP hostname.
    Tcp(String),
    /// A resolved TCP hostname and associated IP addresses to use.
    /// The hostname is only used when the SSL mode requires host verification.
    ResolvedTcp { host: String, ips: Vec<String> },
    /// A path to a directory containing the server's Unix socket.
    ///
    /// This variant is only available on Unix platforms.
    #[cfg(unix)]
    Unix(PathBuf),
}

@benesch
Copy link
Member Author

benesch commented Mar 25, 2024

Oh, sorry, I should have mentioned that these API changes come from upstream: sfackler#945

I agree that the order dependency is confusing, but that comes from libpq (https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-MULTIPLE-HOSTS), and rust-postgres strives to be exactly compatible with libpq's connection strings.

Copy link

@rjobanp rjobanp left a comment

Choose a reason for hiding this comment

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

Ah that makes sense! Thanks for clarifying

@benesch benesch force-pushed the hostaddrs branch 3 times, most recently from f0877bf to a1f7b78 Compare March 25, 2024 15:08
@benesch benesch merged commit 91522e4 into master Mar 25, 2024
4 checks passed
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.

2 participants