From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 57209645BE for ; Mon, 20 Jul 2020 10:47:15 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4A966B951 for ; Mon, 20 Jul 2020 10:47:15 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 79028B947 for ; Mon, 20 Jul 2020 10:47:13 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 3DE5443124 for ; Mon, 20 Jul 2020 10:47:13 +0200 (CEST) To: Proxmox Backup Server development discussion , =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= References: <20200714091304.15254-1-s.reiter@proxmox.com> <1594819058.q8bha0npe8.astroid@nora.none> From: Stefan Reiter Message-ID: Date: Mon, 20 Jul 2020 10:47:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <1594819058.q8bha0npe8.astroid@nora.none> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.399 Adjusted score from AWL reputation of From: address KAM_ASCII_DIVIDERS 0.8 Spam that uses ascii formatting tricks KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup] Add .../apt/update API call X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 20 Jul 2020 08:47:15 -0000 thanks for taking a look! On 7/15/20 3:34 PM, Fabian Grünbichler wrote: > On July 14, 2020 11:13 am, Stefan Reiter wrote: >> Lists all available package updates via libapt-pkg. Output format is >> taken from PVE to enable JS component reuse in the future. >> >> Depends on apt-pkg-native-rs. Changelog-URL detection is inspired by PVE >> perl code (but modified). >> >> list_installed_apt_packages iterates all packages and creates an >> APTUpdateInfo with detailed information for every package matched by the >> given filter Fn. >> >> libapt-pkg has some questionable design choices regarding their use of >> 'iterators', which means quite a bit of nesting sadly... >> >> Signed-off-by: Stefan Reiter >> --- >> >> apt-pkg-native-rs requires some custom patches on top of the current upstream to >> be able to access all required information. I sent a PR to upstream, but it >> hasn't been included as of yet. >> >> The package is not mentioned in Cargo.toml, since we don't have an internal >> package for it, but for testing you can include my fork with the patches: >> >> apt-pkg-native = { git = "https://github.com/PiMaker/apt-pkg-native-rs" } >> >> The original is here: https://github.com/FauxFaux/apt-pkg-native-rs > > this is now available with your patches in devel, probably warrants a > comment that this is a special patched one when we add it to Cargo.toml > I'll send the v2 with the Cargo.toml updated and a comment >> Also, the changelog URL detection was initially just taken from Perl code, but >> it turns out we have some slightly different information there, so I did my best >> to rewrite it to be accurate. With this implementation I get a "200 OK" on the >> generated changelog URL of all default-installed packages on PBS, except for two >> with recent security updates (as mentioned in the code comment, they don't seem >> to have changelogs at all). >> >> I'll probably take a look at the perl code in the future to see if it can be >> improved as well, some Debian changelogs produce 404 URLs for me there. >> >> >> src/api2/node.rs | 2 + >> src/api2/node/apt.rs | 205 +++++++++++++++++++++++++++++++++++++++++++ >> src/api2/types.rs | 27 ++++++ >> 3 files changed, 234 insertions(+) >> create mode 100644 src/api2/node/apt.rs >> >> diff --git a/src/api2/node.rs b/src/api2/node.rs >> index 13ff282c..7e70bc04 100644 >> --- a/src/api2/node.rs >> +++ b/src/api2/node.rs >> @@ -11,8 +11,10 @@ mod services; >> mod status; >> pub(crate) mod rrd; >> pub mod disks; >> +mod apt; >> >> pub const SUBDIRS: SubdirMap = &[ >> + ("apt", &apt::ROUTER), >> ("disks", &disks::ROUTER), >> ("dns", &dns::ROUTER), >> ("journal", &journal::ROUTER), >> diff --git a/src/api2/node/apt.rs b/src/api2/node/apt.rs >> new file mode 100644 >> index 00000000..8b1f2ecf >> --- /dev/null >> +++ b/src/api2/node/apt.rs >> @@ -0,0 +1,205 @@ >> +use apt_pkg_native::Cache; >> +use anyhow::{Error, bail}; >> +use serde_json::{json, Value}; >> + >> +use proxmox::{list_subdirs_api_method, const_regex}; >> +use proxmox::api::{api, Router, Permission, SubdirMap}; >> + >> +use crate::config::acl::PRIV_SYS_AUDIT; >> +use crate::api2::types::{APTUpdateInfo, NODE_SCHEMA}; >> + >> +const_regex! { >> + VERSION_EPOCH_REGEX = r"^\d+:"; >> + FILENAME_EXTRACT_REGEX = r"^.*/.*?_(.*)_Packages$"; >> +} >> + >> +fn get_changelog_url( >> + package: &str, >> + filename: &str, >> + source_pkg: &str, >> + version: &str, >> + source_version: &str, >> + origin: &str, >> + component: &str, >> +) -> Result { >> + if origin == "" { >> + bail!("no origin available for package {}", package); >> + } > > maybe we could just switch this out in favor of 'apt changelog'? or see > what that does internally? > AFAICT it does exactly this... Haven't looked at the source, but running "apt changelog samba" (where the latest update was a security one, thus without a changelog) displays the same behaviour we do (a 404 with the "metadata.*" url). The issue with just running "apt changelog" here is that we want a URL and not the changelog itself (if we want to stay faithful to the PVE API anyway). Though I suppose we could implement the "changelog" API call here and put that into "ChangeLogURL"? >> + >> + if origin == "Debian" { >> + let source_version = (VERSION_EPOCH_REGEX.regex_obj)().replace_all(source_version, ""); >> + >> + let prefix = if source_pkg.starts_with("lib") { >> + source_pkg.get(0..4) >> + } else { >> + source_pkg.get(0..1) >> + }; >> + >> + let prefix = match prefix { >> + Some(p) => p, >> + None => bail!("cannot get starting characters of package name '{}'", package) >> + }; >> + >> + // note: security updates seem to not always upload a changelog for >> + // their package version, so this only works *most* of the time >> + return Ok(format!("https://metadata.ftp-master.debian.org/changelogs/main/{}/{}/{}_{}_changelog", >> + prefix, source_pkg, source_pkg, source_version)); >> + >> + } else if origin == "Proxmox" && component.starts_with("pbs") { > > what about co-located PBS & PVE? and probably less important, what about > the devel/Ceph repositories? > Would just leaving out the starts_with fix this? I'll test around. >> + let version = (VERSION_EPOCH_REGEX.regex_obj)().replace_all(version, ""); >> + >> + let base = match (FILENAME_EXTRACT_REGEX.regex_obj)().captures(filename) { >> + Some(captures) => { >> + let base_capture = captures.get(1); >> + match base_capture { >> + Some(base_underscore) => base_underscore.as_str().replace("_", "/"), >> + None => bail!("incompatible filename, cannot find regex group") >> + } >> + }, >> + None => bail!("incompatible filename, doesn't match regex") >> + }; >> + >> + return Ok(format!("http://download.proxmox.com/{}/{}_{}.changelog", >> + base, package, version)); >> + } >> + >> + bail!("unknown origin ({}) or component ({})", origin, component) >> +} >> + >> +fn list_installed_apt_packages bool>(filter: F) >> + -> Vec { >> + >> + let mut ret = Vec::new(); >> + >> + // note: this is not an 'apt update', it just re-reads the cache from disk >> + let mut cache = Cache::get_singleton(); >> + cache.reload(); >> + >> + let mut cache_iter = cache.iter(); >> + >> + loop { >> + let view = match cache_iter.next() { >> + Some(view) => view, >> + None => break >> + }; >> + >> + let current_version = match view.current_version() { >> + Some(vers) => vers, >> + None => continue >> + }; >> + let candidate_version = match view.candidate_version() { >> + Some(vers) => vers, >> + // if there's no candidate (i.e. no update) get info of currently >> + // installed version instead >> + None => current_version.clone() >> + }; >> + >> + let package = view.name(); >> + if filter(&package, ¤t_version, &candidate_version) { >> + let mut origin_res = "unknown".to_owned(); >> + let mut section_res = "unknown".to_owned(); >> + let mut priority_res = "unknown".to_owned(); >> + let mut change_log_url = "".to_owned(); >> + let mut short_desc = package.clone(); >> + let mut long_desc = "".to_owned(); >> + >> + // get additional information via nested APT 'iterators' >> + let mut view_iter = view.versions(); >> + while let Some(ver) = view_iter.next() { >> + if ver.version() == candidate_version { >> + if let Some(section) = ver.section() { >> + section_res = section; >> + } >> + >> + if let Some(prio) = ver.priority_type() { >> + priority_res = prio; >> + } >> + >> + // assume every package has only one origin and package file > > isn't this wrong though? e.g., we ship packages that are also shipped > with origin Debian.. > I'll have to check what happens with those. What is supposed to happen? Which package would that be for example? >> + let mut origin_iter = ver.origin_iter(); >> + let origin = origin_iter.next(); >> + if let Some(origin) = origin { >> + >> + if let Some(sd) = origin.short_desc() { >> + short_desc = sd; >> + } >> + >> + if let Some(ld) = origin.long_desc() { >> + long_desc = ld; >> + } >> + >> + let mut pkg_iter = origin.file(); >> + let pkg_file = pkg_iter.next(); >> + if let Some(pkg_file) = pkg_file { >> + if let Some(origin_name) = pkg_file.origin() { >> + origin_res = origin_name; >> + } >> + >> + let filename = pkg_file.file_name(); >> + let source_pkg = ver.source_package(); >> + let source_ver = ver.source_version(); >> + let component = pkg_file.component(); >> + >> + // build changelog URL from gathered information >> + // ignore errors, use empty changelog instead >> + let url = get_changelog_url(&package, &filename, &source_pkg, >> + &candidate_version, &source_ver, &origin_res, &component); >> + if let Ok(url) = url { >> + change_log_url = url; >> + } >> + } >> + } >> + >> + break; >> + } >> + } >> + >> + let info = APTUpdateInfo { >> + package, >> + title: short_desc, >> + arch: view.arch(), >> + description: long_desc, >> + change_log_url, >> + origin: origin_res, >> + version: candidate_version, >> + old_version: current_version, >> + priority: priority_res, >> + section: section_res, >> + }; >> + ret.push(info); >> + } >> + } >> + >> + return ret; >> +} >> + >> +#[api( >> + input: { >> + properties: { >> + node: { >> + schema: NODE_SCHEMA, >> + }, >> + }, >> + }, >> + returns: { >> + description: "A list of packages with available updates.", >> + type: Array, >> + items: { type: APTUpdateInfo }, >> + }, >> + access: { >> + permission: &Permission::Privilege(&[], PRIV_SYS_AUDIT, false), >> + }, >> +)] >> +/// List available APT updates >> +fn apt_update_available(_param: Value) -> Result { >> + let ret = list_installed_apt_packages(|_pkg, cur_ver, can_ver| cur_ver != can_ver); >> + Ok(json!(ret)) >> +} >> + >> +const SUBDIRS: SubdirMap = &[ >> + ("update", &Router::new().get(&API_METHOD_APT_UPDATE_AVAILABLE)), >> +]; >> + >> +pub const ROUTER: Router = Router::new() >> + .get(&list_subdirs_api_method!(SUBDIRS)) >> + .subdirs(SUBDIRS); >> diff --git a/src/api2/types.rs b/src/api2/types.rs >> index 0d0fab3b..da73f8db 100644 >> --- a/src/api2/types.rs >> +++ b/src/api2/types.rs >> @@ -962,3 +962,30 @@ pub enum RRDTimeFrameResolution { >> /// 1 week => last 490 days >> Year = 60*10080, >> } >> + >> +#[api()] >> +#[derive(Serialize, Deserialize)] >> +#[serde(rename_all = "kebab-case")] > > widget toolkit expects PascalCase, so either we fix it there to handle > both (if that's possible), or we return ugly here :-/ > Right, I wasn't sure how to handle this. I guess PascalCase here would be the easiest fix. Have we not encountered this issue anywhere else? >> +/// Describes a package for which an update is available. >> +pub struct APTUpdateInfo { >> + /// Package name >> + pub package: String, >> + /// Package title >> + pub title: String, >> + /// Package architecture >> + pub arch: String, >> + /// Human readable package description >> + pub description: String, >> + /// New version to be updated to >> + pub version: String, >> + /// Old version currently installed >> + pub old_version: String, >> + /// Package origin >> + pub origin: String, >> + /// Package priority in human-readable form >> + pub priority: String, >> + /// Package section >> + pub section: String, >> + /// URL under which the package's changelog can be retrieved >> + pub change_log_url: String, >> +} >> -- >> 2.20.1 >> >> >> >> _______________________________________________ >> pbs-devel mailing list >> pbs-devel@lists.proxmox.com >> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel >> >> >> > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > >