External indexing socket with SSL support by var77 · Pull Request #327 · lanterndata/lantern

var77 · 2024-08-02T13:43:17Z

No description provided.

…nal indexing server

greptile-apps

PR Summary

The pull request introduces SSL support for external indexing sockets, enhancing security for data transmission.

CMakeLists.txt: Added OpenSSL checks and conditional compile-time macros for SSL support.
ci/scripts/build-linux.sh: Updated to install lantern-cli from varik/external-indexing-server-ssl branch.
scripts/integration_tests.py: Added SSL setup for external indexing server tests.
src/hnsw/build.c: Replaced external_client_fd with external_socket for SSL support.
src/hnsw/external_index_socket.c: Added SSL initialization, read, send, and close functions.
src/hnsw/external_index_socket_ssl.c: New file for SSL support functions.
src/hnsw/options.c: Added ldb_external_index_secure option for enabling SSL connections.

_{11 file(s) reviewed, 34 comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps · 2024-08-02T13:43:44Z

CMakeLists.txt

@@ -1,4 +1,5 @@
 cmake_minimum_required(VERSION 3.3)
+include(CheckSymbolExists)


info: Added include(CheckSymbolExists) to enable checking for the existence of specific symbols, which is necessary for the OpenSSL integration.

greptile-apps · 2024-08-02T13:43:45Z

CMakeLists.txt

+check_symbol_exists(USE_OPENSSL ${PostgreSQL_INCLUDE_DIRS}/pg_config.h PG_USE_OPENSSL)
+
+if(PG_USE_OPENSSL)
+  target_compile_definitions(lantern PRIVATE LANTERN_USE_OPENSSL)
+endif()


info: Added checks for OpenSSL symbols and conditionally defined LANTERN_USE_OPENSSL if OpenSSL is available. This is essential for enabling SSL support in the external indexing socket.

greptile-apps · 2024-08-02T13:44:00Z

ci/scripts/build-linux.sh

    then
      setup_rust
-      ORT_STRATEGY=system cargo install --git https://github.com/lanterndata/lantern_extras.git --branch main --debug --bin lantern-cli --root /tmp
+      ORT_STRATEGY=system cargo install --git https://github.com/lanterndata/lantern_extras.git --branch varik/external-indexing-server-ssl --debug --bin lantern-cli --root /tmp


info: Updated the branch for lantern-cli installation to varik/external-indexing-server-ssl.

greptile-apps · 2024-08-02T13:44:35Z

scripts/integration_tests.py

 @pytest.fixture
 def external_index(request):
    cli_path = os.getenv("LANTERN_CLI_PATH")
+    use_ssl = os.getenv("EXTERNAL_INDEX_SECURE")


info: Added support for SSL by checking the EXTERNAL_INDEX_SECURE environment variable.

greptile-apps · 2024-08-02T13:44:36Z

scripts/integration_tests.py

+                subprocess.run(["openssl", "req", "-x509", "-nodes", "-days", "365", "-newkey", "rsa:2048", "-keyout", "/tmp/key.pem", "-out", "/tmp/cert.pem", "-subj", "/C=US/ST=California/L=San Francisco/O=Lantern/CN=lantern.dev"])
+                ssl_args = ["--cert", "/tmp/cert.pem", "--key", "/tmp/key.pem"]


info: Generates SSL certificates and keys if EXTERNAL_INDEX_SECURE is set.

greptile-apps · 2024-08-02T13:47:20Z

src/hnsw/external_index_socket_ssl.c

+void close_ssl(external_index_socket_t *socket)
+{
+    if(socket->ssl != NULL) {
+        SSL_free(socket->ssl);
+        socket->ssl = NULL;
+    }
+
+    if(socket->ssl_ctx != NULL) {
+        SSL_CTX_free(socket->ssl_ctx);
+        socket->ssl_ctx = NULL;
+    }
+
+    close(socket->fd);
+}


info: Implemented close_ssl function to properly free SSL resources and close the socket.

greptile-apps · 2024-08-02T13:47:38Z

src/hnsw/external_index_socket_ssl.h

+#ifndef LDB_EXTERNAL_IDX_SOCKET_SSL_H
+#define LDB_EXTERNAL_IDX_SOCKET_SSL_H
+
+#ifdef LANTERN_USE_OPENSSL
+#include <openssl/err.h>
+#include <openssl/ssl.h>
+#else
+#define SSL_CTX void
+#define SSL     void
+#endif
+
+#endif  // LDB_EXTERNAL_IDX_SOCKET_SSL_H


info: Introduced conditional inclusion of OpenSSL headers for SSL support in external indexing sockets.

greptile-apps · 2024-08-02T13:48:06Z

src/hnsw/options.c

 int   ldb_hnsw_ef_search;
 int   ldb_external_index_port;
 char *ldb_external_index_host;
+bool  ldb_external_index_secure;


info: Added a new boolean variable ldb_external_index_secure to store the SSL configuration for external indexing.

greptile-apps · 2024-08-02T13:48:07Z

src/hnsw/options.c

+    DefineCustomBoolVariable("lantern.external_index_secure",
+                             "Use SSL connection when connecting to external index socket",
+                             "Set this to 0 to disable secure connection",
+                             &ldb_external_index_secure,
+                             true,
+                             PGC_USERSET,
+                             0,
+                             NULL,
+                             NULL,
+                             NULL);


info: Defined a new custom boolean variable lantern.external_index_secure to enable or disable SSL connections for the external index socket.

greptile-apps · 2024-08-02T13:48:29Z

src/hnsw/options.h

 extern bool  ldb_pgvector_compat;
 extern int   ldb_external_index_port;
 extern char* ldb_external_index_host;
+extern bool  ldb_external_index_secure;


info: Added extern bool ldb_external_index_secure; to support SSL for external indexing.

github-actions · 2024-08-02T14:55:14Z

Benchmarks

metric	old	new	pct change
recall (after create)	0.951	0.953	+0.21%
recall (after insert)	0.000	0.000	-
select tps	25374.002	25827.280	+1.79%
select bulk(100) tps	35.720	34.496	-3.43%
select latency (ms)	0.745 ± 1.560𝜎	0.726 ± 1.615𝜎	-2.55%
select bulk(100) latency (ms)	885.606 ± 99.034𝜎	884.818 ± 143.799𝜎	-0.09%
create latency (ms)	396793.805	393713.500	-0.78%
insert tps	431.859	447.078	+3.52%
insert bulk(100) tps	4.802	4.927	+2.60%
insert latency (ms)	73.505 ± 15.007𝜎	70.953 ± 15.966𝜎	-3.47%
insert bulk(100) latency (ms)	6513.105 ± 135.856𝜎	6365.889 ± 109.313𝜎	-2.26%
disk usage (bytes)	8192008192.000	8192008192.000	-

codecov · 2024-08-02T16:14:07Z

Codecov Report

Attention: Patch coverage is 73.39450% with 29 lines in your changes missing coverage. Please review.

Files	Patch %	Lines
src/hnsw/external_index_socket.c	72.72%	14 Missing and 4 partials ⚠️
src/hnsw/external_index_socket_ssl.c	68.57%	4 Missing and 7 partials ⚠️

📢 Thoughts on this report? Let us know!

Ngalstyan4 · 2024-08-06T12:57:48Z

ci/scripts/build.sh

  mkdir build
  cd build

+  if [ -n "$DISABLE_SSL" ]; then


let's use DISABLE_SSL or USE_SSL, not both

Ngalstyan4 · 2024-08-06T13:17:59Z

scripts/integration_tests.py

 @pytest.fixture
 def external_index(request):
    cli_path = os.getenv("LANTERN_CLI_PATH")
+    use_ssl = os.getenv("EXTERNAL_INDEX_SECURE") == "1"


Can this variable be called USE_SSL or whatever we decide is the uniform name of the variable?

Ngalstyan4 · 2024-08-06T13:25:37Z

src/hnsw/external_index_socket.c

+    while(total < len) {
+        n = socket_con->send(socket_con, buf + total, bytesleft, flags);
+        if(n == -1 || n == 0) {
+            return n;


why is this silently returning the number and not closing the socket and erroring out?

it seems we would want to error out from all the callsites anyway. If so, the API is consuming since the caller might not realize they have to check for these values and deal with closing/erroring out since otherwise the function is quite high level (it handles partial sends and retires)

Ngalstyan4 · 2024-08-06T13:50:07Z

src/hnsw/external_index_socket.c

    }

-    uint32 buf_size = read(client_fd, &init_response, EXTERNAL_INDEX_INIT_BUFFER_SIZE);
+    uint32 buf_size = socket_con->read(socket_con, (char *)&init_response, EXTERNAL_INDEX_INIT_BUFFER_SIZE);


buf_size type must must int32 since it might report -1 an later error handling function below compares it against -1

…overage info at the end of all tests

var77 added 2 commits July 31, 2024 20:07

properly handle socket send to make sure full packet is sent to exter…

c652537

…nal indexing server

add ssl support for external index sockets

d4cf9f6

greptile-apps bot reviewed Aug 2, 2024

View reviewed changes

var77 force-pushed the varik/external-indexing-ssl branch 2 times, most recently from 481e62a to bc57b42 Compare August 2, 2024 14:44

var77 force-pushed the varik/external-indexing-ssl branch 4 times, most recently from 73907d3 to 7968f95 Compare August 2, 2024 15:36

add tests for no-ssl builds

a7c98f8

var77 force-pushed the varik/external-indexing-ssl branch from 7968f95 to a7c98f8 Compare August 2, 2024 15:59

var77 added 2 commits August 5, 2024 13:03

use getaddrinfo to resolve ip address for external indexing socket

674a73a

add test for create/reindex index concurrently with external indexing

6a4cf73

Ngalstyan4 reviewed Aug 6, 2024

View reviewed changes

var77 force-pushed the varik/external-indexing-ssl branch 4 times, most recently from be42e69 to c5e52d6 Compare August 6, 2024 16:44

refactor sendall to check for error, fix int to uint casts, collect c…

d84c174

…overage info at the end of all tests

var77 force-pushed the varik/external-indexing-ssl branch from c5e52d6 to d84c174 Compare August 6, 2024 16:46

var77 added 2 commits August 7, 2024 10:36

change socket send to write as we are not setting any flags

3990f40

fix cast issues in external_index_socket files

9ecbbf7

var77 merged commit 3cd50df into main Aug 7, 2024

var77 deleted the varik/external-indexing-ssl branch August 7, 2024 08:46

		@@ -1,4 +1,5 @@
		cmake_minimum_required(VERSION 3.3)
		include(CheckSymbolExists)

		subprocess.run(["openssl", "req", "-x509", "-nodes", "-days", "365", "-newkey", "rsa:2048", "-keyout", "/tmp/key.pem", "-out", "/tmp/cert.pem", "-subj", "/C=US/ST=California/L=San Francisco/O=Lantern/CN=lantern.dev"])
		ssl_args = ["--cert", "/tmp/cert.pem", "--key", "/tmp/key.pem"]

Conversation

var77 commented Aug 2, 2024

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Uh oh!

greptile-apps bot Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Uh oh!

codecov bot commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Ngalstyan4 Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

Ngalstyan4 Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

Ngalstyan4 Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

Ngalstyan4 Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Aug 2, 2024 •

edited

Loading

codecov bot commented Aug 2, 2024 •

edited

Loading