From 467cb43555738ad81c56a6d1d7641bd24826888a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Fri, 1 Oct 2021 21:47:19 +0200 Subject: [PATCH 1/7] Added improved error handling around downloading and installing packages. Agent now specifically requests 'application/gzip' as content type in the download request and fails the download if it receives anything else. --- CHANGELOG.md | 13 +-- src/provider/error.rs | 7 ++ .../repository/stackablerepository.rs | 98 +++++++++++++++++-- src/provider/states/pod/installing.rs | 23 ++++- 4 files changed, 126 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29d258c9..04b7b2bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,16 +7,17 @@ removed on startup ([#312]). ### Changed +- Changed the version reported by the Stackable Agent in `nodeInfo.kubeletVersion` of the `Node` object in Kubernetes + from the version of the Krustlet library to the Stackable Agent version ([#315]). - Restart agent on all crashes ([#318]). -[#312]: https://github.com/stackabletech/agent/pull/312 -[#318]: https://github.com/stackabletech/agent/pull/318 - -### Changed -- Changed the version reported by the Stackable Agent in `nodeInfo.kubeletVersion` of the `Node` object in Kubernetes - from the version of the Krustlet library to the Stackable Agent version ([#315]). +### Fixed +- Agent deletes directories from failed install attempts ([#319]) +[#312]: https://github.com/stackabletech/agent/pull/312 [#315]: https://github.com/stackabletech/agent/pull/315 +[#318]: https://github.com/stackabletech/agent/pull/318 +[#319]: https://github.com/stackabletech/agent/pull/319 ## [0.6.1] - 2021-09-14 diff --git a/src/provider/error.rs b/src/provider/error.rs index 8d1a50ea..2591de79 100644 --- a/src/provider/error.rs +++ b/src/provider/error.rs @@ -3,6 +3,7 @@ use k8s_openapi::url; use thiserror::Error; use crate::provider::repository::package::Package; +use reqwest::Url; use std::ffi::OsString; #[derive(Error, Debug)] @@ -22,6 +23,12 @@ pub enum StackableError { #[from] source: kube::Error, }, + #[error("An error has ocurred when trying to download [{package}] from [{download_link}]: {errormessage}")] + PackageDownloadError { + package: Package, + download_link: Url, + errormessage: String, + }, #[error(transparent)] TemplateRenderError(#[from] RenderError), #[error(transparent)] diff --git a/src/provider/repository/stackablerepository.rs b/src/provider/repository/stackablerepository.rs index e666dbd6..70db29df 100644 --- a/src/provider/repository/stackablerepository.rs +++ b/src/provider/repository/stackablerepository.rs @@ -3,16 +3,18 @@ use std::convert::TryFrom; use std::fmt; use std::fs::File; use std::hash::{Hash, Hasher}; -use std::io::{copy, Cursor}; +use std::io::{copy, Cursor, Write}; use std::path::PathBuf; use kube::api::Meta; -use log::{debug, trace}; +use log::{debug, trace, warn}; +use reqwest::header::{ACCEPT, CONTENT_TYPE}; +use reqwest::{Client, StatusCode}; use serde::{Deserialize, Serialize}; use url::Url; use crate::provider::error::StackableError; -use crate::provider::error::StackableError::PackageNotFound; +use crate::provider::error::StackableError::{PackageDownloadError, PackageNotFound}; use crate::provider::repository::package::Package; use crate::provider::repository::repository_spec::Repository; @@ -112,12 +114,76 @@ impl StackableRepoProvider { let stackable_package = self.get_package(package.clone()).await?; let download_link = Url::parse(&stackable_package.link)?; - let response = reqwest::get(download_link).await?; + let client = Client::builder() + .build() + .map_err(|error| PackageDownloadError { + package: package.clone(), + download_link: download_link.clone(), + errormessage: format!("Unable to create http client: [{}]", error), + })?; + + // We set the ACCEPT header field on our request which states that the only content type + // we are willing to accept is 'application/gzip' + // If the webserver is unable to provide this content type to us it _SHOULD_ respond with a + // 406 response code, but it seems we can't rely on that. + // For more details see: https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.1 + let response = match client + .get(download_link.clone()) + .header(ACCEPT, "application/gzip") + .send() + .await + { + Ok(response) if response.status().is_success() => { + // The request was successful, but just to be safe we'll still check the content_type + if let Some(content_type) = response.headers().get(CONTENT_TYPE) { + if content_type == "application/gzip" { + Ok(response) + } else { + // If we get a known wrong content type we'll abort + Err(PackageDownloadError { + package: package.clone(), + download_link, + errormessage: format!( + "Got wrong 'content_type' header [{:?}] in response from webserver.", + content_type + ), + }) + } + } else { + // If we get no content_type (not sure if this is even legal) we'll soldier on and hope for the best + debug!("Response had no 'content_type' header set, we'll give the sender the benefit of the doubt and try processing this anyway."); + Ok(response) + } + } + Ok(response) if response.status() == StatusCode::NOT_ACCEPTABLE => + Err(PackageDownloadError { + package: package.clone(), + download_link, + errormessage: "Got response code 406 from webserver - Unable to negotiate content type, this is probably due to content encoding settings on the webserver.".to_string(), + }), + Ok(response) => Err(PackageDownloadError { + package: package.clone(), + download_link, + errormessage: format!( + "Got non-success response [{}] from webserver!", + response.status() + ), + }), + Err(error) => Err(PackageDownloadError { + package: package.clone(), + download_link, + errormessage: format!("{}", error), + }), + }?; + + // All error cases return above, so we can safely assume that this is a valid download at + // this point let mut content = Cursor::new(response.bytes().await?); let mut out = File::create(target_path.join(package.get_file_name()))?; copy(&mut content, &mut out)?; + out.flush()?; Ok(()) } @@ -126,10 +192,26 @@ impl StackableRepoProvider { debug!("Retrieving repository metadata from {}", self.metadata_url); - let repo_data = reqwest::get(self.metadata_url.clone()) - .await? - .json::() - .await?; + let repo_data = match reqwest::get(self.metadata_url.clone()).await { + Ok(repo_data) => repo_data, + Err(error) => { + warn!( + "Failed to retrieve metadata from {} due to {:?}", + self.metadata_url, error + ); + return Err(error.into()); + } + }; + let repo_data = match repo_data.json::().await { + Ok(parsed_data) => parsed_data, + Err(error) => { + warn!( + "Error parsing metadata from repository {}: {:?}", + self.name, error + ); + return Err(error.into()); + } + }; debug!("Got repository metadata: {:?}", repo_data); diff --git a/src/provider/states/pod/installing.rs b/src/provider/states/pod/installing.rs index 6b23ab61..94c9c1f7 100644 --- a/src/provider/states/pod/installing.rs +++ b/src/provider/states/pod/installing.rs @@ -1,3 +1,4 @@ +use std::fs; use std::fs::File; use std::path::{Path, PathBuf}; @@ -76,7 +77,7 @@ impl State for Installing { ); } else { info!("Installing package {}", package); - match self.install_package(package) { + match self.install_package(package.clone()) { Ok(()) => Transition::next( self, CreatingConfig { @@ -88,6 +89,26 @@ impl State for Installing { "Failed to install package [{}] due to: [{:?}]", &package_name, e ); + // Clean up partially unpacked directory, to avoid later iterations + // assuming this install attempt was successfull because the + // target directory exists + let installation_directory = self + .parcel_directory + .join(package.get_directory_name()) + .to_string_lossy() + .to_string(); + debug!( + "Cleaning up partial installation by deleting directory [{}]", + installation_directory + ); + if let Err(error) = + fs::remove_dir_all(self.parcel_directory.join(package.get_directory_name())) + { + error!( + "Failed to clean up directory [{}] due to {}", + installation_directory, error + ); + }; Transition::next( self, SetupFailed { From a8eec58d0c4632e8ce133795da6021cb743ca222 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Tue, 12 Oct 2021 00:25:51 +0200 Subject: [PATCH 2/7] Added extra content types that Nexus sends. --- .../repository/stackablerepository.rs | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/provider/repository/stackablerepository.rs b/src/provider/repository/stackablerepository.rs index 70db29df..923aaca8 100644 --- a/src/provider/repository/stackablerepository.rs +++ b/src/provider/repository/stackablerepository.rs @@ -6,17 +6,22 @@ use std::hash::{Hash, Hasher}; use std::io::{copy, Cursor, Write}; use std::path::PathBuf; +use crate::provider::error::StackableError; +use crate::provider::error::StackableError::{PackageDownloadError, PackageNotFound}; +use crate::provider::repository::package::Package; +use crate::provider::repository::repository_spec::Repository; use kube::api::Meta; +use lazy_static::lazy_static; use log::{debug, trace, warn}; use reqwest::header::{ACCEPT, CONTENT_TYPE}; use reqwest::{Client, StatusCode}; use serde::{Deserialize, Serialize}; use url::Url; -use crate::provider::error::StackableError; -use crate::provider::error::StackableError::{PackageDownloadError, PackageNotFound}; -use crate::provider::repository::package::Package; -use crate::provider::repository::repository_spec::Repository; +lazy_static! { + static ref DEFAULT_ALLOWED_CONTENT_TYPES: Vec<&'static str> = + vec!["application/gzip", "application/tgz", "application/x-gzip"]; +} #[derive(Debug, Clone)] pub struct StackableRepoProvider { @@ -116,6 +121,7 @@ impl StackableRepoProvider { let download_link = Url::parse(&stackable_package.link)?; let client = Client::builder() + .danger_accept_invalid_certs(true) .build() .map_err(|error| PackageDownloadError { package: package.clone(), @@ -137,7 +143,13 @@ impl StackableRepoProvider { Ok(response) if response.status().is_success() => { // The request was successful, but just to be safe we'll still check the content_type if let Some(content_type) = response.headers().get(CONTENT_TYPE) { - if content_type == "application/gzip" { + let content_type = content_type.to_str().map_err(|error| PackageDownloadError { + package: package.clone(), + download_link: download_link.clone(), + errormessage: format!("Got content_type with non-ascii characters from webserver: [{}]", error), + })?; + + if DEFAULT_ALLOWED_CONTENT_TYPES.contains(&content_type) { Ok(response) } else { // If we get a known wrong content type we'll abort From 9442e88cda8fbdad7488dd6f5ee0511bd03b284f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Tue, 12 Oct 2021 11:55:10 +0200 Subject: [PATCH 3/7] Added changed download behavior to CHANGELOG.md --- CHANGELOG.md | 2 ++ src/provider/repository/stackablerepository.rs | 7 ++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 04b7b2bb..ef14ec76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ - Changed the version reported by the Stackable Agent in `nodeInfo.kubeletVersion` of the `Node` object in Kubernetes from the version of the Krustlet library to the Stackable Agent version ([#315]). - Restart agent on all crashes ([#318]). +- Agent will now request content_type "application/gzip" in package downloads and reject responses with content_type + that is not one of either "application/gzip", "application/tgz" or "application/x-gzip" ([#319]) ### Fixed - Agent deletes directories from failed install attempts ([#319]) diff --git a/src/provider/repository/stackablerepository.rs b/src/provider/repository/stackablerepository.rs index 923aaca8..94cc7cd8 100644 --- a/src/provider/repository/stackablerepository.rs +++ b/src/provider/repository/stackablerepository.rs @@ -18,6 +18,10 @@ use reqwest::{Client, StatusCode}; use serde::{Deserialize, Serialize}; use url::Url; +// These are the default content_types that we have seen in the wild +// of these only 'application/gzip' is valid according to +// https://www.iana.org/assignments/media-types/media-types.xhtml but our own +// Nexus uses the other two, so we cannot really complain lazy_static! { static ref DEFAULT_ALLOWED_CONTENT_TYPES: Vec<&'static str> = vec!["application/gzip", "application/tgz", "application/x-gzip"]; @@ -141,7 +145,8 @@ impl StackableRepoProvider { .await { Ok(response) if response.status().is_success() => { - // The request was successful, but just to be safe we'll still check the content_type + // The request was successful, but just to be safe we'll still check the content_type, + // since the webserver is free to ignore the requested content_type if let Some(content_type) = response.headers().get(CONTENT_TYPE) { let content_type = content_type.to_str().map_err(|error| PackageDownloadError { package: package.clone(), From 2d727b824d0a317e24cf9425fbd701643f28d9dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Thu, 14 Oct 2021 08:47:03 +0200 Subject: [PATCH 4/7] Removed accidentally committed line. --- src/provider/repository/stackablerepository.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/provider/repository/stackablerepository.rs b/src/provider/repository/stackablerepository.rs index 94cc7cd8..26f71288 100644 --- a/src/provider/repository/stackablerepository.rs +++ b/src/provider/repository/stackablerepository.rs @@ -125,7 +125,6 @@ impl StackableRepoProvider { let download_link = Url::parse(&stackable_package.link)?; let client = Client::builder() - .danger_accept_invalid_certs(true) .build() .map_err(|error| PackageDownloadError { package: package.clone(), From 84488a85a2b552880913b64e6d4eaef0662c6221 Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Mon, 18 Oct 2021 12:54:32 +0200 Subject: [PATCH 5/7] Fix pull request number in changelog --- CHANGELOG.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ef14ec76..e0f8469d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,16 +10,16 @@ - Changed the version reported by the Stackable Agent in `nodeInfo.kubeletVersion` of the `Node` object in Kubernetes from the version of the Krustlet library to the Stackable Agent version ([#315]). - Restart agent on all crashes ([#318]). -- Agent will now request content_type "application/gzip" in package downloads and reject responses with content_type - that is not one of either "application/gzip", "application/tgz" or "application/x-gzip" ([#319]) +- Agent will now request content type "application/gzip" in package downloads and reject responses with content type + that is not one of either "application/gzip", "application/tgz" or "application/x-gzip" ([#326]) ### Fixed -- Agent deletes directories from failed install attempts ([#319]) +- Agent deletes directories from failed install attempts ([#326]) [#312]: https://github.com/stackabletech/agent/pull/312 [#315]: https://github.com/stackabletech/agent/pull/315 [#318]: https://github.com/stackabletech/agent/pull/318 -[#319]: https://github.com/stackabletech/agent/pull/319 +[#326]: https://github.com/stackabletech/agent/pull/326 ## [0.6.1] - 2021-09-14 From ff4503b84b26bf00c3e5409838190e084270dc8c Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Mon, 18 Oct 2021 13:43:20 +0200 Subject: [PATCH 6/7] Use single source of target directory calculation --- src/provider/states/pod/installing.rs | 34 +++++++++++---------------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/src/provider/states/pod/installing.rs b/src/provider/states/pod/installing.rs index 94c9c1f7..0bbba300 100644 --- a/src/provider/states/pod/installing.rs +++ b/src/provider/states/pod/installing.rs @@ -1,6 +1,6 @@ use std::fs; use std::fs::File; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; use flate2::read::GzDecoder; use kubelet::pod::state::prelude::*; @@ -26,15 +26,15 @@ impl Installing { fn package_installed>(&self, package: T) -> bool { let package = package.into(); - let package_file_name = self.parcel_directory.join(package.get_directory_name()); + let target_directory = self.get_target_directory(&package); debug!( "Checking if package {:?} has already been installed to {:?}", - package, package_file_name + package, target_directory ); - Path::new(&package_file_name).exists() + target_directory.exists() } - fn get_target_directory(&self, package: Package) -> PathBuf { + fn get_target_directory(&self, package: &Package) -> PathBuf { self.parcel_directory.join(package.get_directory_name()) } @@ -46,13 +46,13 @@ impl Installing { let tar = GzDecoder::new(tar_gz); let mut archive = Archive::new(tar); - let target_directory = self.get_target_directory(package.clone()); + let target_directory = self.get_target_directory(&package); info!( "Installing package: {:?} from {:?} into {:?}", package, archive_path, target_directory ); - archive.unpack(self.parcel_directory.join(package.get_directory_name()))?; + archive.unpack(target_directory)?; Ok(()) } } @@ -89,24 +89,18 @@ impl State for Installing { "Failed to install package [{}] due to: [{:?}]", &package_name, e ); - // Clean up partially unpacked directory, to avoid later iterations - // assuming this install attempt was successfull because the - // target directory exists - let installation_directory = self - .parcel_directory - .join(package.get_directory_name()) - .to_string_lossy() - .to_string(); + // Clean up partially unpacked directory to avoid later iterations assuming + // this install attempt was successful because the target directory exists. + let installation_directory = self.get_target_directory(&package); debug!( "Cleaning up partial installation by deleting directory [{}]", - installation_directory + installation_directory.to_string_lossy() ); - if let Err(error) = - fs::remove_dir_all(self.parcel_directory.join(package.get_directory_name())) - { + if let Err(error) = fs::remove_dir_all(&installation_directory) { error!( "Failed to clean up directory [{}] due to {}", - installation_directory, error + installation_directory.to_string_lossy(), + error ); }; Transition::next( From 7698966e86c547c6225bd5ffb4c8eba9fcfba690 Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Mon, 18 Oct 2021 14:02:25 +0200 Subject: [PATCH 7/7] Remove unnecessary usage of lazy_static --- src/provider/repository/stackablerepository.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/provider/repository/stackablerepository.rs b/src/provider/repository/stackablerepository.rs index 26f71288..32e8074e 100644 --- a/src/provider/repository/stackablerepository.rs +++ b/src/provider/repository/stackablerepository.rs @@ -11,7 +11,6 @@ use crate::provider::error::StackableError::{PackageDownloadError, PackageNotFou use crate::provider::repository::package::Package; use crate::provider::repository::repository_spec::Repository; use kube::api::Meta; -use lazy_static::lazy_static; use log::{debug, trace, warn}; use reqwest::header::{ACCEPT, CONTENT_TYPE}; use reqwest::{Client, StatusCode}; @@ -22,10 +21,8 @@ use url::Url; // of these only 'application/gzip' is valid according to // https://www.iana.org/assignments/media-types/media-types.xhtml but our own // Nexus uses the other two, so we cannot really complain -lazy_static! { - static ref DEFAULT_ALLOWED_CONTENT_TYPES: Vec<&'static str> = - vec!["application/gzip", "application/tgz", "application/x-gzip"]; -} +const DEFAULT_ALLOWED_CONTENT_TYPES: &[&str] = + &["application/gzip", "application/tgz", "application/x-gzip"]; #[derive(Debug, Clone)] pub struct StackableRepoProvider {