91 add endpoint for image proxying #100

mickvandijke · 2023-03-29T20:12:58Z

closes #91 .

With this PR we can let clients proxy torrent description images through the backend.

How it works

The following endpoint has been added to the backend /proxy/image/{original_image_url_urlencoded}

This endpoint accepts the optional header "Authentication": "Bearer TOKEN"

Authenticated users

When an authenticated user requests an image by its url from the backend proxy, the backend will first check whether the image is already in a local cache and then serve it to the user. If it is not in the local cache already, the backend will request the image from the supplied url and then store it in the local cache.

Unauthenticated users

Guests can only request images that are already in the cache.

Quota

Too mitigate spam, the proxy endpoint is limited by a quota per user. Whenever an authenticated user requests an image that is not already in the local cache, the size of the requested image will be deducted from the user's quota.

When the quota is met, the user will only be able to request images that are already in the cache.

The quota resets after a certain time period that can be set in the config.

Error feedback

In the case of an error, such as (but not limited to):

Invalid image url
Too big images
User quota met

The backend will return an image with the error text in red.

Configuration

[cache]
image_cache_max_request_timeout_ms = 1000 // max timeout when requesting an image from its external url
image_cache_capacity = 128_000_000 // max total cache size in bytes
image_cache_entry_size_limit = 4_000_000 // max size for a single image in the cache
image_cache_user_quota_period_seconds = 3600 // time between quota resets in seconds
image_cache_user_quota_bytes = 64_000_000 // image size quota in bytes per user

josecelano

Hey @WarmBeer looks good. I would extract some methods. I've added some suggestions but nothing critical to stopping it from merging.

josecelano · 2023-04-10T15:46:54Z

src/bin/main.rs

+    let image_cache_manager_config = ImageCacheManagerConfig {
+        max_image_request_timeout_ms,
+        max_image_size,
+        user_quota_period_seconds,
+        user_quota_bytes,
+    };


Hey @WarmBeer, why don't you use torrust_index_backend::config::Cache, something like:

let cache_config = cfg.settings.get_cache_configuration().await;

Hi @josecelano , I see the image cache as an isolated plugin to the backend. This is why I thought that it should own its own config struct.

OK, you mean it could have its crate without specifying how the configuration should be stored in a toml file, do you? I suppose that kind of extension should also provide a specification for a config file section. But maybe they could also use an independent config file. Anyway, for the time being, config.settings.get_cache_configuration could return an ImageCacheManagerConfig. I think that's easy to change in the future if you extract the extension.

src/cache/cache.rs

josecelano · 2023-04-10T15:53:43Z

src/cache/cache.rs

+    EntrySizeLimitExceedsTotalCapacity,
+    BytesExceedEntrySizeLimit,
+    CacheCapacityIsTooSmall,
+    CacheFull,


@WarmBeer It seems CacheFull is not being used. Anyway, it's full for ...? I assume it's full for adding a new entry.

josecelano · 2023-04-10T15:58:55Z

src/cache/cache.rs

+    }
+
+    // With a limit for individual entry sizes.
+    pub fn with_entry_size_limit(esl: usize) -> Self {


@WarmBeer Generally, I prefer to be explicit with entry_size_limit instead of esl. Unless the abbreviation is really common, like cfg, ...

josecelano · 2023-04-10T15:59:20Z

src/cache/cache.rs

+        new
+    }
+
+    // With bot a total capacity limit and an individual entry size limit.


both instead of bot

josecelano · 2023-04-10T16:28:52Z

src/cache/image/manager.rs

+    pub fn met(&self) -> bool {
+        self.usage >= self.max_usage
+    }
+}


@WarmBeer I think "reached" is more common than "met", but it's OK.

josecelano · 2023-04-10T16:31:29Z

src/cache/image/manager.rs

+
+    /// Get an image from the url and insert it into the cache if it isn't cached already.
+    /// Unauthenticated users can only get already cached images.
+    pub async fn get_image_by_url(&self, url: &str, opt_user: Option<UserCompact>) -> Result<Bytes, Error> {


@WarmBeer some people use "maybe_" prefix instead of "opt_". It makes more sense to me. "opt" sounds like a configuration option. However, "maybe_" is only used in Rust.

Hey @josecelano , I can't find a naming convention for this in Rust anywhere. I have come across code using the opt prefix some time ago and I just stuck with that I guess.

There is a Reddit thread that discusses this naming convention: https://www.reddit.com/r/rust/comments/9hpyam/question_what_names_do_you_use_for_option_and/ .

Both prefixes maybe and opt seem to be used.

I don't have a preference for any over the other.

Hey @josecelano ,

Thank you for the review! I have given my thoughts and put a thumbs up were I fully agree with you and will update things. I think however that we should merge this now and then I will make these changes later on as other tasks take priority currently. Is that ok?

Yes, it's OK.

josecelano · 2023-04-10T16:35:01Z

src/cache/image/manager.rs

+        // TODO: Update the cache on a separate thread, so that the client does not have to wait.
+        // Update image cache.


@WarmBeer, maybe you can split this function into two parts here. You can return the bytes, return the images to the client and later call another method with the rest of the tasks: update cache, quota, etcetera. I think that's very common in web frameworks.

I would also extract a method for each task: request a remote image, update cache, update quota, etcetera.

josecelano · 2023-04-10T16:37:44Z

src/config.rs

+    pub image_cache_user_quota_period_seconds: u64,
+    pub image_cache_user_quota_bytes: usize,
+}
+
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct TorrustConfig {


@WarmBeer rename to TorrustIndexBackendConfig? However, I think it's not configuration but a kind of AppContainer with tall the application service dependencies.

josecelano · 2023-04-11T08:56:24Z

src/routes/proxy.rs

+        Err(e) => unsafe {
+            // Handling status codes in the frontend other tan OK is quite a pain.
+            // Return OK for now.
+            let (_status_code, error_image_bytes): (StatusCode, Bytes) = match e {
+                Error::UrlIsUnreachable => (StatusCode::GATEWAY_TIMEOUT, ERROR_IMAGE_URL_IS_UNREACHABLE.clone()),
+                Error::UrlIsNotAnImage => (StatusCode::BAD_REQUEST, ERROR_IMAGE_URL_IS_NOT_AN_IMAGE.clone()),
+                Error::ImageTooBig => (StatusCode::BAD_REQUEST, ERROR_IMAGE_TOO_BIG.clone()),
+                Error::UserQuotaMet => (StatusCode::TOO_MANY_REQUESTS, ERROR_IMAGE_USER_QUOTA_MET.clone()),
+                Error::Unauthenticated => (StatusCode::UNAUTHORIZED, ERROR_IMAGE_UNAUTHENTICATED.clone()),
+            };


Hey @WarmBeer why is this unsafe?

Hey @josecelano ,

All the error image bytes are stored in mutable static variables. Reading these variables is unsafe in Rust, because you don't know what other threads do with them. Since the variables are instantiated in a once_cell, we can consider them safe as there are no other places where they would be mutated.

I do not like using static and unsafe code, so I will update this in the future.

mickvandijke · 2023-04-20T08:40:06Z

Hey @josecelano ,

Thank you for the review! I have given my thoughts and put a thumbs up were I fully agree with you and will update things. I think however that we should merge this now and then I will make these changes later on as other tasks take priority currently. Is that ok?

josecelano · 2023-04-11T09:15:56Z

src/cache/cache.rs

+    #[tokio::test]
+    async fn set_multiple_bytes_cache_with_capacity_and_entry_size_limit_should_have_len_of_two() {
+        let mut bytes_cache = BytesCache::with_capacity_and_entry_size_limit(12, 6).unwrap();
+        let bytes: Bytes = Bytes::from("abcdef");
+
+        assert!(bytes_cache.set("1".to_string(), bytes.clone()).await.is_ok());
+        assert!(bytes_cache.set("2".to_string(), bytes).await.is_ok());
+
+        assert_eq!(bytes_cache.len().await, 2)
+    }


Hi @WarmBeer, I'd refactor this test to:

#[tokio::test] async fn given_a_bytes_cache_with_a_capacity_and_entry_size_limit_it_should_allow_adding_new_entries_if_the_limit_is_not_exceeded() { let bytes: Bytes = Bytes::from("abcdef"); let mut bytes_cache = BytesCache::with_capacity_and_entry_size_limit(bytes.len() * 2, bytes.len()).unwrap(); // Add first entry (6 bytes) assert!(bytes_cache.set("key1".to_string(), bytes.clone()).await.is_ok()); // Add second entry (6 bytes) assert!(bytes_cache.set("key2".to_string(), bytes).await.is_ok()); // Both entries were added because we did not reach the limit assert_eq!(bytes_cache.len().await, 2) }

If what we are testing is the cache limit.

josecelano · 2023-04-20T11:19:42Z

src/bin/main.rs

+    let image_cache_manager_config = ImageCacheManagerConfig {
+        max_image_request_timeout_ms,
+        max_image_size,
+        user_quota_period_seconds,
+        user_quota_bytes,
+    };


OK, you mean it could have its crate without specifying how the configuration should be stored in a toml file, do you? I suppose that kind of extension should also provide a specification for a config file section. But maybe they could also use an independent config file. Anyway, for the time being, config.settings.get_cache_configuration could return an ImageCacheManagerConfig. I think that's easy to change in the future if you extract the extension.

josecelano · 2023-04-20T11:29:30Z

src/cache/image/manager.rs

+
+    /// Get an image from the url and insert it into the cache if it isn't cached already.
+    /// Unauthenticated users can only get already cached images.
+    pub async fn get_image_by_url(&self, url: &str, opt_user: Option<UserCompact>) -> Result<Bytes, Error> {


Hey @josecelano ,

Thank you for the review! I have given my thoughts and put a thumbs up were I fully agree with you and will update things. I think however that we should merge this now and then I will make these changes later on as other tasks take priority currently. Is that ok?

Yes, it's OK.

@josecelano

916d869 refactor: remove patch versions from added packages (Warm Beer) d5c5487 refactor: cargo fmt (Warm Beer) 005817f feat: [#91] added image proxy with cache (Warm Beer) Pull request description: Rebased and processed @josecelano 's feedback: #100

josecelano · 2023-05-06T07:08:10Z

@WarmBeer created a new PR for this feature and it was already merged.

mickvandijke linked an issue Mar 29, 2023 that may be closed by this pull request

Add endpoint for image proxying #91

Closed

mickvandijke force-pushed the 91-add-endpoint-for-image-proxying branch from ac26149 to 3e11c83 Compare April 6, 2023 20:45

mickvandijke requested review from da2ce7 and josecelano April 9, 2023 13:15

josecelano approved these changes Apr 11, 2023

View reviewed changes

josecelano approved these changes Apr 20, 2023

View reviewed changes

da2ce7 added the Needs Rebase Base Branch has Incompatibilities label Apr 20, 2023

da2ce7 force-pushed the 91-add-endpoint-for-image-proxying branch 2 times, most recently from 8dd997b to 83afc51 Compare April 25, 2023 15:32

da2ce7 removed the Needs Rebase Base Branch has Incompatibilities label Apr 25, 2023

mickvandijke added 4 commits April 25, 2023 17:35

feat: added image proxy cache

cb83347

fix: removed test setting

867d7f3

refactor: run cargo fmt

6ec547a

feat: generate error images from text

83afc51

mickvandijke mentioned this pull request May 3, 2023

feat: [#91] added image proxy with cache #134

Merged

josecelano closed this May 6, 2023

da2ce7 deleted the 91-add-endpoint-for-image-proxying branch September 21, 2023 11:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

91 add endpoint for image proxying #100

91 add endpoint for image proxying #100

mickvandijke commented Mar 29, 2023 •

edited

Loading

josecelano left a comment

josecelano Apr 10, 2023

mickvandijke Apr 20, 2023

josecelano Apr 20, 2023

josecelano Apr 10, 2023

josecelano Apr 10, 2023

josecelano Apr 10, 2023

josecelano Apr 10, 2023

josecelano Apr 10, 2023

mickvandijke Apr 20, 2023

josecelano Apr 20, 2023

josecelano Apr 10, 2023

josecelano Apr 10, 2023

josecelano Apr 10, 2023

josecelano Apr 11, 2023

mickvandijke Apr 20, 2023

mickvandijke commented Apr 20, 2023

josecelano Apr 11, 2023

josecelano Apr 20, 2023

josecelano Apr 20, 2023

josecelano commented May 6, 2023

		// TODO: Update the cache on a separate thread, so that the client does not have to wait.
		// Update image cache.

91 add endpoint for image proxying #100

91 add endpoint for image proxying #100

Conversation

mickvandijke commented Mar 29, 2023 • edited Loading

How it works

Authenticated users

Unauthenticated users

Quota

Error feedback

Configuration

josecelano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickvandijke commented Apr 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josecelano commented May 6, 2023

mickvandijke commented Mar 29, 2023 •

edited

Loading