From 193b7a14f2d8967c9180c71664cd9de6346a7757 Mon Sep 17 00:00:00 2001 From: Yan Song Date: Mon, 17 Jul 2023 09:12:00 +0000 Subject: [PATCH] storage: remove auth_through option for registry mirror The auth_through option adds user burden to configure the mirror and understand its meaning, and since we have optimized handling of concurrent token requests, this option can now be removed. Signed-off-by: Yan Song --- api/src/config.rs | 11 ------ docs/nydusd.md | 3 -- .../blob_cache_entry_configuration_v2.toml | 4 -- docs/samples/configuration_v2.toml | 4 -- storage/src/backend/connection.rs | 37 +++---------------- storage/src/backend/http_proxy.rs | 11 +----- storage/src/backend/object_storage.rs | 20 +--------- storage/src/backend/registry.rs | 30 +++------------ 8 files changed, 15 insertions(+), 105 deletions(-) diff --git a/api/src/config.rs b/api/src/config.rs index a58d72b00d3..c4cf24cc09d 100644 --- a/api/src/config.rs +++ b/api/src/config.rs @@ -854,12 +854,6 @@ pub struct MirrorConfig { /// HTTP request headers to be passed to mirror server. #[serde(default)] pub headers: HashMap, - /// Whether the authorization process is through mirror, default to false. - /// true: authorization through mirror, e.g. Using normal registry as mirror. - /// false: authorization through original registry, - /// e.g. when using Dragonfly server as mirror, authorization through it may affect performance. - #[serde(default)] - pub auth_through: bool, /// Interval for mirror health checking, in seconds. #[serde(default = "default_check_interval")] pub health_check_interval: u64, @@ -873,7 +867,6 @@ impl Default for MirrorConfig { Self { host: String::new(), headers: HashMap::new(), - auth_through: false, health_check_interval: 5, failure_limit: 5, ping_url: String::new(), @@ -1597,7 +1590,6 @@ mod tests { [[backend.oss.mirrors]] host = "http://127.0.0.1:65001" ping_url = "http://127.0.0.1:65001/ping" - auth_through = true health_check_interval = 10 failure_limit = 10 "#; @@ -1631,7 +1623,6 @@ mod tests { let mirror = &oss.mirrors[0]; assert_eq!(mirror.host, "http://127.0.0.1:65001"); assert_eq!(mirror.ping_url, "http://127.0.0.1:65001/ping"); - assert!(mirror.auth_through); assert!(mirror.headers.is_empty()); assert_eq!(mirror.health_check_interval, 10); assert_eq!(mirror.failure_limit, 10); @@ -1663,7 +1654,6 @@ mod tests { [[backend.registry.mirrors]] host = "http://127.0.0.1:65001" ping_url = "http://127.0.0.1:65001/ping" - auth_through = true health_check_interval = 10 failure_limit = 10 "#; @@ -1699,7 +1689,6 @@ mod tests { let mirror = ®istry.mirrors[0]; assert_eq!(mirror.host, "http://127.0.0.1:65001"); assert_eq!(mirror.ping_url, "http://127.0.0.1:65001/ping"); - assert!(mirror.auth_through); assert!(mirror.headers.is_empty()); assert_eq!(mirror.health_check_interval, 10); assert_eq!(mirror.failure_limit, 10); diff --git a/docs/nydusd.md b/docs/nydusd.md index 8180dc1ceb1..52ffc568d62 100644 --- a/docs/nydusd.md +++ b/docs/nydusd.md @@ -286,9 +286,6 @@ Currently, the mirror mode is only tested in the registry backend, and in theory { // Mirror server URL (including scheme), e.g. Dragonfly dfdaemon server URL "host": "http://dragonfly1.io:65001", - // true: Send the authorization request to the mirror e.g. another docker registry. - // false: Authorization request won't be relayed by the mirror e.g. Dragonfly. - "auth_through": false, // Headers for mirror server "headers": { // For Dragonfly dfdaemon server URL, we need to specify "X-Dragonfly-Registry" (including scheme). diff --git a/docs/samples/blob_cache_entry_configuration_v2.toml b/docs/samples/blob_cache_entry_configuration_v2.toml index 2e424f125bf..06002cc19fb 100644 --- a/docs/samples/blob_cache_entry_configuration_v2.toml +++ b/docs/samples/blob_cache_entry_configuration_v2.toml @@ -57,8 +57,6 @@ host = "http://127.0.0.1:65001" ping_url = "http://127.0.0.1:65001/ping" # HTTP request headers to be passed to mirror server. # headers = -# Whether the authorization process is through mirror, default to false. -auth_through = true # Interval for mirror health checking, in seconds. health_check_interval = 5 # Maximum number of failures before marking a mirror as unusable. @@ -108,8 +106,6 @@ host = "http://127.0.0.1:65001" ping_url = "http://127.0.0.1:65001/ping" # HTTP request headers to be passed to mirror server. # headers = -# Whether the authorization process is through mirror, default to false. -auth_through = true # Interval for mirror health checking, in seconds. health_check_interval = 5 # Maximum number of failures before marking a mirror as unusable. diff --git a/docs/samples/configuration_v2.toml b/docs/samples/configuration_v2.toml index b1bc865e3c7..4e0e4db9f1f 100644 --- a/docs/samples/configuration_v2.toml +++ b/docs/samples/configuration_v2.toml @@ -55,8 +55,6 @@ host = "http://127.0.0.1:65001" ping_url = "http://127.0.0.1:65001/ping" # HTTP request headers to be passed to mirror server. # headers = -# Whether the authorization process is through mirror, default to false. -auth_through = true # Interval for mirror health checking, in seconds. health_check_interval = 5 # Maximum number of failures before marking a mirror as unusable. @@ -106,8 +104,6 @@ host = "http://127.0.0.1:65001" ping_url = "http://127.0.0.1:65001/ping" # HTTP request headers to be passed to mirror server. # headers = -# Whether the authorization process is through mirror, default to false. -auth_through = true # Interval for mirror health checking, in seconds. health_check_interval = 5 # Maximum number of failures before marking a mirror as unusable. diff --git a/storage/src/backend/connection.rs b/storage/src/backend/connection.rs index e324e4f4aa2..f5f889e0bcf 100644 --- a/storage/src/backend/connection.rs +++ b/storage/src/backend/connection.rs @@ -448,13 +448,6 @@ impl Connection { self.shutdown.store(true, Ordering::Release); } - /// If the auth_through is enable, all requests are send to the mirror server. - /// If the auth_through disabled, e.g. P2P/Dragonfly, we try to avoid sending - /// non-authorization request to the mirror server, which causes performance loss. - /// requesting_auth means this request is to get authorization from a server, - /// which must be a non-authorization request. - /// IOW, only the requesting_auth is false and the headers contain authorization token, - /// we send this request to mirror. #[allow(clippy::too_many_arguments)] pub fn call( &self, @@ -464,8 +457,6 @@ impl Connection { data: Option>, headers: &mut HeaderMap, catch_status: bool, - // This means the request is dedicated to authorization. - requesting_auth: bool, ) -> ConnectionResult { if self.shutdown.load(Ordering::Acquire) { return Err(ConnectionError::Disconnected); @@ -524,27 +515,10 @@ impl Connection { } } + let mut mirror_enabled = false; if !self.mirrors.is_empty() { - let mut fallback_due_auth = false; + mirror_enabled = true; for mirror in self.mirrors.iter() { - // With configuration `auth_through` disabled, we should not intend to send authentication - // request to mirror. Mainly because mirrors like P2P/Dragonfly has a poor performance when - // relaying non-data requests. But it's still possible that ever returned token is expired. - // So mirror might still respond us with status code UNAUTHORIZED, which should be handle - // by sending authentication request to the original registry. - // - // - For non-authentication request with token in request header, handle is as usual requests to registry. - // This request should already take token in header. - // - For authentication request - // 1. auth_through is disabled(false): directly pass below mirror translations and jump to original registry handler. - // 2. auth_through is enabled(true): try to get authenticated from mirror and should also handle status code UNAUTHORIZED. - if !mirror.config.auth_through - && (!headers.contains_key(HEADER_AUTHORIZATION) || requesting_auth) - { - fallback_due_auth = true; - break; - } - if mirror.status.load(Ordering::Relaxed) { let data_cloned = data.as_ref().cloned(); @@ -598,9 +572,10 @@ impl Connection { headers.remove(HeaderName::from_str(key).unwrap()); } } - if !fallback_due_auth { - warn!("Request to all mirror server failed, fallback to original server."); - } + } + + if mirror_enabled { + warn!("Request to all mirror server failed, fallback to original server."); } self.call_inner( diff --git a/storage/src/backend/http_proxy.rs b/storage/src/backend/http_proxy.rs index 8fd31df77a5..c1324fbef78 100644 --- a/storage/src/backend/http_proxy.rs +++ b/storage/src/backend/http_proxy.rs @@ -214,7 +214,6 @@ impl BlobReader for HttpProxyReader { None, &mut HeaderMap::new(), true, - false, ) .map(|resp| resp.headers().to_owned()) .map_err(|e| HttpProxyError::RemoteRequest(e).into()) @@ -255,15 +254,7 @@ impl BlobReader for HttpProxyReader { .map_err(|e| HttpProxyError::ConstructHeader(format!("{}", e)))?, ); let mut resp = connection - .call::<&[u8]>( - Method::GET, - uri.as_str(), - None, - None, - &mut headers, - true, - false, - ) + .call::<&[u8]>(Method::GET, uri.as_str(), None, None, &mut headers, true) .map_err(HttpProxyError::RemoteRequest)?; Ok(resp diff --git a/storage/src/backend/object_storage.rs b/storage/src/backend/object_storage.rs index c7a617b2aaa..7c2b8ba655c 100644 --- a/storage/src/backend/object_storage.rs +++ b/storage/src/backend/object_storage.rs @@ -89,15 +89,7 @@ where let resp = self .connection - .call::<&[u8]>( - Method::HEAD, - url.as_str(), - None, - None, - &mut headers, - true, - false, - ) + .call::<&[u8]>(Method::HEAD, url.as_str(), None, None, &mut headers, true) .map_err(ObjectStorageError::Request)?; let content_length = resp .headers() @@ -136,15 +128,7 @@ where // Safe because the the call() is a synchronous operation. let mut resp = self .connection - .call::<&[u8]>( - Method::GET, - url.as_str(), - None, - None, - &mut headers, - true, - false, - ) + .call::<&[u8]>(Method::GET, url.as_str(), None, None, &mut headers, true) .map_err(ObjectStorageError::Request)?; Ok(resp .copy_to(&mut buf) diff --git a/storage/src/backend/registry.rs b/storage/src/backend/registry.rs index 987d2a0873b..b25fb624e0b 100644 --- a/storage/src/backend/registry.rs +++ b/storage/src/backend/registry.rs @@ -265,7 +265,6 @@ impl RegistryState { Some(ReqBody::Form(form)), &mut headers, true, - true, ) .map_err(|e| einval!(format!("registry auth server request failed {:?}", e)))?; let ret: TokenResponse = token_resp.json().map_err(|e| { @@ -484,22 +483,14 @@ impl RegistryReader { if let Some(data) = data { return self .connection - .call( - method, - url, - None, - Some(data), - &mut headers, - catch_status, - false, - ) + .call(method, url, None, Some(data), &mut headers, catch_status) .map_err(RegistryError::Request); } // Try to request registry server with `authorization` header let mut resp = self .connection - .call::<&[u8]>(method.clone(), url, None, None, &mut headers, false, false) + .call::<&[u8]>(method.clone(), url, None, None, &mut headers, false) .map_err(RegistryError::Request)?; if resp.status() == StatusCode::UNAUTHORIZED { if headers.contains_key(HEADER_AUTHORIZATION) { @@ -514,7 +505,7 @@ impl RegistryReader { resp = self .connection - .call::<&[u8]>(method.clone(), url, None, None, &mut headers, false, false) + .call::<&[u8]>(method.clone(), url, None, None, &mut headers, false) .map_err(RegistryError::Request)?; }; @@ -534,7 +525,7 @@ impl RegistryReader { // Try to request registry server with `authorization` header again let resp = self .connection - .call(method, url, None, data, &mut headers, catch_status, false) + .call(method, url, None, data, &mut headers, catch_status) .map_err(RegistryError::Request)?; let status = resp.status(); @@ -590,7 +581,6 @@ impl RegistryReader { None, &mut headers, false, - false, ) .map_err(RegistryError::Request)?; @@ -675,7 +665,6 @@ impl RegistryReader { None, &mut headers, true, - false, ) .map_err(RegistryError::Request); match resp_ret { @@ -819,8 +808,6 @@ impl Registry { cached_bearer_auth: ArcSwapOption::new(None), }); - let mirrors = connection.mirrors.clone(); - let registry = Registry { connection, state, @@ -828,13 +815,8 @@ impl Registry { first: First::new(), }; - for mirror in mirrors.iter() { - if !mirror.config.auth_through { - registry.start_refresh_token_thread(); - info!("Refresh token thread started."); - break; - } - } + registry.start_refresh_token_thread(); + info!("Refresh token thread started."); Ok(registry) }