Skip to content

Commit

Permalink
storage: remove auth_through option for registry mirror
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
imeoer committed Jul 17, 2023
1 parent e137c01 commit c5c719b
Show file tree
Hide file tree
Showing 9 changed files with 15 additions and 109 deletions.
11 changes: 0 additions & 11 deletions api/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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(),
Expand Down Expand Up @@ -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
"#;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
"#;
Expand Down Expand Up @@ -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);
Expand Down
3 changes: 0 additions & 3 deletions docs/nydusd.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
4 changes: 0 additions & 4 deletions misc/configs/nydusd-blob-cache-entry-configuration-v2.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
4 changes: 0 additions & 4 deletions misc/configs/nydusd-blob-cache-entry.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
4 changes: 0 additions & 4 deletions misc/configs/nydusd-config-v2.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
37 changes: 6 additions & 31 deletions storage/src/backend/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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);
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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(
Expand Down
11 changes: 1 addition & 10 deletions storage/src/backend/http_proxy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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
Expand Down
20 changes: 2 additions & 18 deletions storage/src/backend/object_storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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)
Expand Down
30 changes: 6 additions & 24 deletions storage/src/backend/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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| {
Expand Down Expand Up @@ -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) {
Expand All @@ -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)?;
};

Expand All @@ -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();
Expand Down Expand Up @@ -529,7 +520,6 @@ impl RegistryReader {
None,
&mut headers,
false,
false,
)
.map_err(RegistryError::Request)?;

Expand Down Expand Up @@ -614,7 +604,6 @@ impl RegistryReader {
None,
&mut headers,
true,
false,
)
.map_err(RegistryError::Request);
match resp_ret {
Expand Down Expand Up @@ -784,22 +773,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)
}
Expand Down

0 comments on commit c5c719b

Please sign in to comment.