Re: [pbs-devel] [PATCH proxmox-backup] Add .../apt/update API call

[Stefan Reiter <s.reiter@proxmox.com>]

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 <s.reiter@proxmox.com>
>> ---
>>
>> 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<String, Error> {
>> +    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<F: Fn(&str, &str, &str) -> bool>(filter: F)
>> +    -> Vec<APTUpdateInfo> {
>> +
>> +    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, &current_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<Value, Error> {
>> +    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
> 
>