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 <[email protected]>
dragonflyoss · Jul 18, 2023 · 0775ec1 · 0775ec1
1 parent 73f5703
commit 0775ec1
Show file tree

Hide file tree

Showing 9 changed files with 15 additions and 109 deletions.
diff --git a/api/src/config.rs b/api/src/config.rs
@@ -914,12 +914,6 @@ pub struct MirrorConfig {
     /// HTTP request headers to be passed to mirror server.
     #[serde(default)]
     pub headers: HashMap<String, String>,
-    /// 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,
@@ -933,7 +927,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(),
@@ -1822,7 +1815,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
         "#;
@@ -1856,7 +1848,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);
@@ -1888,7 +1879,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
         "#;
@@ -1924,7 +1914,6 @@ mod tests {
         let mirror = &registry.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
@@ -297,9 +297,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/misc/configs/nydusd-blob-cache-entry-configuration-v2.toml b/misc/configs/nydusd-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/misc/configs/nydusd-blob-cache-entry.toml b/misc/configs/nydusd-blob-cache-entry.toml
@@ -62,8 +62,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.
@@ -113,8 +111,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/misc/configs/nydusd-config-v2.toml b/misc/configs/nydusd-config-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
@@ -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<R: Read + Clone + Send + 'static>(
         &self,
@@ -464,8 +457,6 @@ impl Connection {
         data: Option<ReqBody<R>>,
         headers: &mut HeaderMap,
         catch_status: bool,
-        // This means the request is dedicated to authorization.
-        requesting_auth: bool,
     ) -> ConnectionResult<Response> {
         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
@@ -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
@@ -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
@@ -267,7 +267,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| {
@@ -423,22 +422,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) {
@@ -453,7 +444,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)?;
             };
 
@@ -473,7 +464,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();
@@ -529,7 +520,6 @@ impl RegistryReader {
                     None,
                     &mut headers,
                     false,
-                    false,
                 )
                 .map_err(RegistryError::Request)?;
 
@@ -614,7 +604,6 @@ impl RegistryReader {
                             None,
                             &mut headers,
                             true,
-                            false,
                         )
                         .map_err(RegistryError::Request);
                     match resp_ret {
@@ -786,22 +775,15 @@ impl Registry {
             cached_bearer_auth: ArcSwapOption::new(None),
         });
 
-        let mirrors = connection.mirrors.clone();
-
         let registry = Registry {
             connection,
             state,
             metrics: BackendMetrics::new(id, "registry"),
             first: Arc::new(Once::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)
     }