Skip to content

Conversation

@tristan957
Copy link
Member

@tristan957 tristan957 commented May 12, 2025

In order to enable TLS connections between computes and safekeepers, we need to provide the control plane with a way to configure the various libpq keyword parameters, sslmode and sslrootcert. neon.safekeepers is a comma separated list of safekeepers formatted as host:port, so isn't available for extension in the same way that neon.pageserver_connstring is. This could be remedied in a future PR.

Part-of: https://github.com/neondatabase/cloud/issues/25823
Link: https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-PARAMKEYWORDS

@tristan957 tristan957 requested review from a team as code owners May 12, 2025 20:09
@tristan957 tristan957 requested review from knizhnik, myrrc and problame May 12, 2025 20:09
@github-actions
Copy link

If this PR added a GUC in the Postgres fork or neon extension,
please regenerate the Postgres settings in the cloud repo:

make NEON_WORKDIR=path/to/neon/checkout \
  -C goapp/internal/shareddomain/postgres generate

If you're an external contributor, a Neon employee will assist in
making sure this step is done.

@tristan957 tristan957 requested a review from DimasKovas May 12, 2025 20:09
@tristan957 tristan957 force-pushed the tristan957/extra branch 2 times, most recently from a4b0079 to e6eaa27 Compare May 12, 2025 20:33
@github-actions
Copy link

github-actions bot commented May 12, 2025

8492 tests run: 7944 passed, 0 failed, 548 skipped (full report)


Flaky tests (5)

Postgres 17

Postgres 16

Postgres 15

Code coverage* (full report)

  • functions: 32.3% (9053 of 27987 functions)
  • lines: 48.5% (79448 of 163662 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
4b188aa at 2025-05-27T02:31:58.294Z :recycle:

@tristan957 tristan957 force-pushed the tristan957/extra branch 2 times, most recently from 2210ca1 to 053ef6a Compare May 12, 2025 21:29
Copy link
Contributor

@problame problame left a comment

Choose a reason for hiding this comment

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

I don't have much context on this and the PR lacks description of how the new field is going to be used.

That said, I can't spot a problem wrt the changed Rust code that requires the CODEOWNERS approval from storage team.

Nit on naming: why not call it just safekeeper_extra_conninfo?
The _options suffix is confusing to me because there is already an options='-c timeline_id=...' piece in the libpq connstring that we assemble, and this new GUC here will not be part of that option string, but just a prefix to the conninfo.

An even more precise name would be safekeeper_conninfo_prepend.

@tristan957
Copy link
Member Author

tristan957 commented May 13, 2025

I don't have much context on this and the PR lacks description of how the new field is going to be used.

I will amend the commit and PR. Thanks!

That said, I can't spot a problem wrt the changed Rust code that requires the CODEOWNERS approval from storage team.

Nit on naming: why not call it just safekeeper_extra_conninfo? The _options suffix is confusing to me because there is already an options='-c timeline_id=...' piece in the libpq connstring that we assemble, and this new GUC here will not be part of that option string, but just a prefix to the conninfo.

libpq calls the structure a PQconninfoOption: https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-PQCONNINFO. I've just taken the name from that.

An even more precise name would be safekeeper_conninfo_prepend.

Hmm, I'm open to adding the prepend, but the reason I added it in the beginning of the connection strings was so that the extension always overwrites if host, port, or options is passed via the GUC. Eventually this GUC could turn into neon.safekeeper_connstrings. Let me know if you want me to change it on the next review.

@tristan957 tristan957 force-pushed the tristan957/extra branch 4 times, most recently from 5af9eef to 5ebb064 Compare May 14, 2025 02:59
@tristan957 tristan957 changed the title Add neon.safekeeper_extra_conninfo_options GUC Add neon.safekeeper_conninfo_options GUC May 14, 2025
@DimasKovas
Copy link
Contributor

LGTM, let's merge

In order to enable TLS connections between computes and safekeepers, we
need to provide the control plane with a way to configure the various
libpq keyword parameters, sslmode and sslrootcert. neon.safekeepers is a
comma separated list of safekeepers formatted as host:port, so isn't
available for extension in the same way that neon.pageserver_connstring
is. This could be remedied in a future PR.

Part-of: neondatabase/neon-archive-cloud#25823
Link: https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-PARAMKEYWORDS
Signed-off-by: Tristan Partin <tristan@neon.tech>
@tristan957 tristan957 enabled auto-merge May 27, 2025 01:21
@tristan957 tristan957 added this pull request to the merge queue May 27, 2025
Merged via the queue into main with commit fe1513c May 27, 2025
104 checks passed
@tristan957 tristan957 deleted the tristan957/extra branch May 27, 2025 02:31
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