real s3 tests should now pass. adding the better_download function is gross, hoping to remove it in a future commit

compute_tools/src/bin/compute_ctl.rs

@@ -58,7 +58,9 @@ use compute_tools::monitor::launch_monitor;
 use compute_tools::params::*;
 use compute_tools::spec::*;
 
-const BUILD_TAG_DEFAULT: &str = "111"; // TODO: change back to local; I need 111 for my test
+// this is an arbitrary build tag. Fine as a default / for testing purposes
+// in-case of not-set environment var
+const BUILD_TAG_DEFAULT: &str = "5648391853";
 
 fn main() -> Result<()> {
     init_tracing_and_logging(DEFAULT_LOG_LEVEL)?;

compute_tools/src/extension_server.rs

@@ -117,7 +117,7 @@ pub async fn get_available_extensions(
     let index_path = RemotePath::new(Path::new(&index_path)).context("error forming path")?;
     info!("download ext_index.json from: {:?}", &index_path);
 
-    let mut download = remote_storage.download(&index_path).await?;
+    let mut download = better_download(remote_storage, &index_path).await?;
     let mut ext_idx_buffer = Vec::new();
     download
         .download_stream
@@ -170,7 +170,7 @@ pub async fn download_extension(
     pgbin: &str,
 ) -> Result<()> {
     info!("Download extension {:?} from {:?}", ext_name, ext_path);
-    let mut download = remote_storage.download(ext_path).await?;
+    let mut download = better_download(remote_storage, ext_path).await?;
     let mut download_buffer = Vec::new();
     download
         .download_stream
@@ -189,11 +189,11 @@ pub async fn download_extension(
     info!("Download + unzip {:?} completed successfully", &ext_path);
 
     let sharedir_paths = (
-        format!("{unzip_dest}/{ext_name}/share/extension"),
+        unzip_dest.to_string() + "/share/extension",
         Path::new(&get_pg_config("--sharedir", pgbin)).join("extension"),
     );
     let libdir_paths = (
-        format!("{unzip_dest}/{ext_name}/lib"),
+        unzip_dest.to_string() + "/lib",
         Path::new(&get_pg_config("--libdir", pgbin)).join("postgresql"),
     );
     // move contents of the libdir / sharedir in unzipped archive to the correct local paths
@@ -221,17 +221,14 @@ pub fn init_remote_storage(remote_ext_config: &str) -> anyhow::Result<GenericRem
         .as_str()
         .context("config parse error")?;
     let remote_ext_endpoint = remote_ext_config["endpoint"].as_str();
-    let remote_ext_prefix = remote_ext_config["prefix"]
-        .as_str()
-        .unwrap_or_default()
-        .to_string();
+    let remote_ext_prefix = remote_ext_config["prefix"].as_str();
 
     // If needed, it is easy to allow modification of other parameters
     // however, default values should be fine for now
     let config = S3Config {
         bucket_name: remote_ext_bucket.to_string(),
         bucket_region: remote_ext_region.to_string(),
-        prefix_in_bucket: Some(remote_ext_prefix),
+        prefix_in_bucket: remote_ext_prefix.map(str::to_string),
         endpoint: remote_ext_endpoint.map(|x| x.to_string()),
         concurrency_limit: NonZeroUsize::new(100).expect("100 != 0"),
         max_keys_per_list_response: None,

control_plane/src/endpoint.rs

@@ -499,7 +499,7 @@ impl Endpoint {
             //
             // The proper way to implement this is to pass the custom extension
             // in spec, but we don't have a way to do that yet in the python tests.
-            custom_extensions: Some(vec!["embedding".into(), "anon".into()]),
+            custom_extensions: Some(vec!["anon".into(), "kq_imcx".into()]),
         };
         let spec_path = self.endpoint_path().join("spec.json");
         std::fs::write(spec_path, serde_json::to_string_pretty(&spec)?)?;

docs/rfcs/024-extension-loading.md

@@ -140,6 +140,7 @@ popular extensions.
 ## Extension Storage implementation
 
 The layout of the S3 bucket is as follows:
+```
 5615610098 // this is an extension build number
 ├── v14
 │   ├── extensions
@@ -169,6 +170,7 @@ The layout of the S3 bucket is as follows:
     ├── extensions
     │   └── embedding.tar.zst
     └── ext_index.json
+```
 
 Note that build number cannot be part of prefix because we might need extensions
 from other build numbers.
@@ -180,6 +182,7 @@ We only upload a new one if it is updated.
 *access* is controlled by spec
 
 More specifically, here is an example ext_index.json
+```
 {
   "embedding": {
     "control_data": {
@@ -194,6 +197,7 @@ More specifically, here is an example ext_index.json
     "archive_path": "5615261079/v15/extensions/anon.tar.zst"
   }
 }
+```
 
 ### How to add new extension to the Extension Storage?
 

libs/remote_storage/src/lib.rs

@@ -24,6 +24,7 @@ use tokio::io;
 use toml_edit::Item;
 use tracing::info;
 
+pub use self::s3_bucket::better_download;
 pub use self::{local_fs::LocalFs, s3_bucket::S3Bucket, simulate_failures::UnreliableWrapper};
 
 /// How many different timelines can be processed simultaneously when synchronizing layers with the remote storage.

libs/remote_storage/src/s3_bucket.rs

@@ -31,7 +31,8 @@ use tracing::debug;
 
 use super::StorageMetadata;
 use crate::{
-    Download, DownloadError, RemotePath, RemoteStorage, S3Config, REMOTE_STORAGE_PREFIX_SEPARATOR,
+    Download, DownloadError, GenericRemoteStorage, RemotePath, RemoteStorage, S3Config,
+    REMOTE_STORAGE_PREFIX_SEPARATOR,
 };
 
 const MAX_DELETE_OBJECTS_REQUEST_SIZE: usize = 1000;
@@ -131,6 +132,39 @@ struct GetObjectRequest {
     key: String,
     range: Option<String>,
 }
+
+use crate::GenericRemoteStorage::AwsS3;
+// the regular download function adds a "/" to the start of file names in the
+// case of prefix="None", which breaks everything. Thus, the following function is necessary
+pub async fn better_download(
+    bucket: &GenericRemoteStorage,
+    from: &RemotePath,
+) -> Result<Download, DownloadError> {
+    if let AwsS3(bucket) = bucket {
+        // this is more expected behavior.
+        // prefix="" should result in a trailing slash
+        // wheras prefix=None should **NOT** result in a trailing slash
+        let query_key = match &bucket.prefix_in_bucket {
+            Some(_) => bucket.relative_path_to_s3_object(from),
+            None => from
+                .get_path()
+                .to_str()
+                .expect("bad object name")
+                .to_string(),
+        };
+
+        bucket
+            .download_object(GetObjectRequest {
+                bucket: bucket.bucket_name.clone(),
+                key: query_key,
+                range: None,
+            })
+            .await
+    } else {
+        panic!("this isn't supposed to happen");
+    }
+}
+
 impl S3Bucket {
     /// Creates the S3 storage, errors if incorrect AWS S3 configuration provided.
     pub fn new(aws_config: &S3Config) -> anyhow::Result<Self> {

test_runner/fixtures/neon_fixtures.py

@@ -812,11 +812,11 @@ class NeonEnvBuilder:
 
         if enable_remote_extensions:
             self.ext_remote_storage = S3Storage(
-                bucket_name="neon-dev-extensions",
+                bucket_name="neon-dev-extensions-eu-central-1",
                 bucket_region="eu-central-1",
                 access_key=access_key,
                 secret_key=secret_key,
-                prefix_in_bucket="",
+                prefix_in_bucket=None,
             )
 
     def cleanup_local_storage(self):

test_runner/regress/data/extension_test/111/v14/ext_index.json

@@ -1,15 +0,0 @@
-{
-  "embedding": {
-    "control_data": {
-      "embedding.control": "comment = 'hnsw index' \ndefault_version = '0.1.0' \nmodule_pathname = '$libdir/embedding' \nrelocatable = true \ntrusted = true"
-    },
-    "archive_path": "111/v14/extensions/embedding.tar.zst"
-  },
-  "anon": {
-    "control_data": {
-      "anon.control": "# PostgreSQL Anonymizer (anon) extension \ncomment = 'Data anonymization tools' \ndefault_version = '1.1.0' \ndirectory='extension/anon' \nrelocatable = false \nrequires = 'pgcrypto' \nsuperuser = false \nmodule_pathname = '$libdir/anon' \ntrusted = true \n"
-    },
-    "archive_path": "111/v14/extensions/anon.tar.zst"
-  }
-}
-

test_runner/regress/data/extension_test/111/v15/ext_index.json

@@ -1,14 +0,0 @@
-{
-  "embedding": {
-    "control_data": {
-      "embedding.control": "comment = 'hnsw index' \ndefault_version = '0.1.0' \nmodule_pathname = '$libdir/embedding' \nrelocatable = true \ntrusted = true"
-    },
-    "archive_path": "111/v15/extensions/embedding.tar.zst"
-  },
-  "anon": {
-    "control_data": {
-      "anon.control": "# PostgreSQL Anonymizer (anon) extension \ncomment = 'Data anonymization tools' \ndefault_version = '1.1.0' \ndirectory='extension/anon' \nrelocatable = false \nrequires = 'pgcrypto' \nsuperuser = false \nmodule_pathname = '$libdir/anon' \ntrusted = true \n"
-    },
-    "archive_path": "111/v15/extensions/anon.tar.zst"
-  }
-}

test_runner/regress/data/extension_test/5648391853/v14/ext_index.json

@@ -0,0 +1 @@
+{"anon": {"control_data": {"anon.control": "# PostgreSQL Anonymizer (anon) extension\ncomment = 'Data anonymization tools'\ndefault_version = '1.1.0'\ndirectory='extension/anon'\nrelocatable = false\nrequires = 'pgcrypto'\nsuperuser = false\nmodule_pathname = '$libdir/anon'\ntrusted = true\n"}, "archive_path": "5648391853/v14/extensions/anon.tar.zstd"}, "kq_imcx": {"control_data": {"kq_imcx.control": "# This file is generated content from add_postgresql_extension.\n# No point in modifying it, it will be overwritten anyway.\n\n# Default version, always set\ndefault_version = '0.1'\n\n# Module pathname generated from target shared library name. Use\n# MODULE_PATHNAME in script file.\nmodule_pathname = '$libdir/kq_imcx.so'\n\n# Comment for extension. Set using COMMENT option. Can be set in\n# script file as well.\ncomment = 'ketteQ In-Memory Calendar Extension (IMCX)'\n\n# Encoding for script file. Set using ENCODING option.\n#encoding = ''\n\n# Required extensions. Set using REQUIRES option (multi-valued).\n#requires = ''\ntrusted = true\n"}, "archive_path": "5648391853/v14/extensions/kq_imcx.tar.zstd"}}

test_runner/regress/data/extension_test/5648391853/v15/ext_index.json

@@ -0,0 +1 @@
+{"kq_imcx": {"control_data": {"kq_imcx.control": "# This file is generated content from add_postgresql_extension.\n# No point in modifying it, it will be overwritten anyway.\n\n# Default version, always set\ndefault_version = '0.1'\n\n# Module pathname generated from target shared library name. Use\n# MODULE_PATHNAME in script file.\nmodule_pathname = '$libdir/kq_imcx.so'\n\n# Comment for extension. Set using COMMENT option. Can be set in\n# script file as well.\ncomment = 'ketteQ In-Memory Calendar Extension (IMCX)'\n\n# Encoding for script file. Set using ENCODING option.\n#encoding = ''\n\n# Required extensions. Set using REQUIRES option (multi-valued).\n#requires = ''\ntrusted = true\n"}, "archive_path": "5648391853/v15/extensions/kq_imcx.tar.zstd"}, "anon": {"control_data": {"anon.control": "# PostgreSQL Anonymizer (anon) extension\ncomment = 'Data anonymization tools'\ndefault_version = '1.1.0'\ndirectory='extension/anon'\nrelocatable = false\nrequires = 'pgcrypto'\nsuperuser = false\nmodule_pathname = '$libdir/anon'\ntrusted = true\n"}, "archive_path": "5648391853/v15/extensions/anon.tar.zstd"}}

test_runner/regress/test_download_extensions.py

@@ -66,13 +66,9 @@ def test_remote_extensions(
             all_extensions = [x[0] for x in cur.fetchall()]
             log.info(all_extensions)
             assert "anon" in all_extensions
-            assert "embedding" in all_extensions
-            # TODO: check that we cant't download custom extensions for other tenant ids
+            assert "kq_imcx" in all_extensions
 
-            # check that we can download public extension
-            cur.execute("CREATE EXTENSION embedding")
-            cur.execute("SELECT extname FROM pg_extension")
-            assert "embedding" in [x[0] for x in cur.fetchall()]
+            # TODO: check that we cant't download custom extensions for other tenant ids
 
             # check that we can download private extension
             try: