* [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI
@ 2021-03-22 11:59 Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-apt 01/10] initial commit Fabian Ebner
` (10 more replies)
0 siblings, 11 replies; 16+ messages in thread
From: Fabian Ebner @ 2021-03-22 11:59 UTC (permalink / raw)
To: pbs-devel
List the configured repositories and have some basic checks for them.
The plan is to use perlmod to make the Rust implementation available for PVE+PMG
as well.
There's still the question if introducing a digest is worth it. At the moment,
the warnings returned by the checkrepositories call might not match up with the
repositories returned previously, but that's a rather minor issue as editing
repositories is a rare operation.
Should a digest be added now to be future-proof? Should it live in the
proxmox-apt crate and be file-level, or would it be enough to hash the result in
the PBS API call and use that? The latter seems like the more pragmatic approach
and avoids cluttering the APT backend.
Changes from v2:
* incorporate Wolfgang's feedback
* improve main warning's UI
Still missing:
* Upgrade suite/distribuiton button to be used before major release
upgrades (but it's really simply to add that now).
* perlmod magic and integration in PVE and PMG.
Changes v1 -> v2:
* Perl -> Rust
* PVE -> PBS
* Don't rely on regexes for parsing.
* Add writer and tests.
* UI: pin warnings to the repository they're for.
* Keep order of options consistent with configuration.
* Smaller things noted on the individual patches.
proxmox-apt:
Fabian Ebner (4):
initial commit
add files for Debian packaging
add functions to check for Proxmox repositories
add check_repositories function
proxmox-backup:
Fabian Ebner (4):
depend on new proxmox-apt crate
api: apt: add repositories call
ui: add repositories
add check_repositories_call
Cargo.toml | 1 +
src/api2/node/apt.rs | 78 +++++++++++++++++++++++++++++++++++++
www/ServerAdministration.js | 7 ++++
3 files changed, 86 insertions(+)
widget-toolkit:
Fabian Ebner (2):
add UI for APT repositories
APT repositories: add warnings
src/Makefile | 1 +
src/node/APTRepositories.js | 295 ++++++++++++++++++++++++++++++++++++
2 files changed, 296 insertions(+)
create mode 100644 src/node/APTRepositories.js
--
2.20.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [pbs-devel] [PATCH v3 proxmox-apt 01/10] initial commit
2021-03-22 11:59 [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI Fabian Ebner
@ 2021-03-22 11:59 ` Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-apt 02/10] add files for Debian packaging Fabian Ebner
` (9 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Fabian Ebner @ 2021-03-22 11:59 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
Changes from v2:
* incorporate Wolfgang's feedback:
* improve warning order/structure in a few places
* make parsing the tail for the one-line format shorter/more readable
* use std::mem::take instead of cloning the comment
* add write_repositories_ref_vec and repositories_from_file helpers for
monomorphization
* have parse_repositories take a &mut Vec and push onto it to avoid
the need to append the result
* improve sorting by matching with std::cmp::Ordering
* improve offset handling for the .sources parser's main loop
* use try_for_each and write directly to avoid collect+join
* implement Display on types instead of From on String
* add comment to explain why AsRef is used
.cargo/config | 5 +
.gitignore | 3 +
Cargo.toml | 22 ++
rustfmt.toml | 1 +
src/lib.rs | 3 +
src/repositories/check.rs | 47 ++++
src/repositories/list_parser.rs | 176 ++++++++++++
src/repositories/mod.rs | 256 ++++++++++++++++++
src/repositories/sources_parser.rs | 214 +++++++++++++++
src/repositories/writer.rs | 85 ++++++
src/types.rs | 211 +++++++++++++++
tests/repositories.rs | 73 +++++
.../sources.list.d.expected/multiline.sources | 8 +
| 3 +
.../pbs-enterprise.list | 2 +
tests/sources.list.d.expected/pve.list | 13 +
tests/sources.list.d.expected/standard.list | 7 +
.../sources.list.d.expected/standard.sources | 11 +
tests/sources.list.d/multiline.sources | 9 +
| 2 +
tests/sources.list.d/pbs-enterprise.list | 1 +
tests/sources.list.d/pve.list | 10 +
tests/sources.list.d/standard.list | 6 +
tests/sources.list.d/standard.sources | 10 +
24 files changed, 1178 insertions(+)
create mode 100644 .cargo/config
create mode 100644 .gitignore
create mode 100644 Cargo.toml
create mode 100644 rustfmt.toml
create mode 100644 src/lib.rs
create mode 100644 src/repositories/check.rs
create mode 100644 src/repositories/list_parser.rs
create mode 100644 src/repositories/mod.rs
create mode 100644 src/repositories/sources_parser.rs
create mode 100644 src/repositories/writer.rs
create mode 100644 src/types.rs
create mode 100644 tests/repositories.rs
create mode 100644 tests/sources.list.d.expected/multiline.sources
create mode 100644 tests/sources.list.d.expected/options_comment.list
create mode 100644 tests/sources.list.d.expected/pbs-enterprise.list
create mode 100644 tests/sources.list.d.expected/pve.list
create mode 100644 tests/sources.list.d.expected/standard.list
create mode 100644 tests/sources.list.d.expected/standard.sources
create mode 100644 tests/sources.list.d/multiline.sources
create mode 100644 tests/sources.list.d/options_comment.list
create mode 100644 tests/sources.list.d/pbs-enterprise.list
create mode 100644 tests/sources.list.d/pve.list
create mode 100644 tests/sources.list.d/standard.list
create mode 100644 tests/sources.list.d/standard.sources
diff --git a/.cargo/config b/.cargo/config
new file mode 100644
index 0000000..3b5b6e4
--- /dev/null
+++ b/.cargo/config
@@ -0,0 +1,5 @@
+[source]
+[source.debian-packages]
+directory = "/usr/share/cargo/registry"
+[source.crates-io]
+replace-with = "debian-packages"
diff --git a/.gitignore b/.gitignore
new file mode 100644
index 0000000..0d00c41
--- /dev/null
+++ b/.gitignore
@@ -0,0 +1,3 @@
+Cargo.lock
+target/
+tests/sources.list.d.actual
diff --git a/Cargo.toml b/Cargo.toml
new file mode 100644
index 0000000..a0ecc26
--- /dev/null
+++ b/Cargo.toml
@@ -0,0 +1,22 @@
+[package]
+name = "proxmox-apt"
+version = "0.1.0"
+authors = [
+ "Fabian Ebner <f.ebner@proxmox.com>",
+ "Proxmox Support Team <support@proxmox.com>",
+]
+edition = "2018"
+license = "AGPL-3"
+description = "Proxmox library for APT"
+homepage = "https://www.proxmox.com"
+
+exclude = [ "debian" ]
+
+[lib]
+name = "proxmox_apt"
+path = "src/lib.rs"
+
+[dependencies]
+anyhow = "1.0"
+proxmox = { version = "0.11.0", features = [ "api-macro" ] }
+serde = { version = "1.0", features = ["derive"] }
diff --git a/rustfmt.toml b/rustfmt.toml
new file mode 100644
index 0000000..32a9786
--- /dev/null
+++ b/rustfmt.toml
@@ -0,0 +1 @@
+edition = "2018"
diff --git a/src/lib.rs b/src/lib.rs
new file mode 100644
index 0000000..b065c0f
--- /dev/null
+++ b/src/lib.rs
@@ -0,0 +1,3 @@
+pub mod types;
+
+pub mod repositories;
diff --git a/src/repositories/check.rs b/src/repositories/check.rs
new file mode 100644
index 0000000..87fbbac
--- /dev/null
+++ b/src/repositories/check.rs
@@ -0,0 +1,47 @@
+use anyhow::{bail, Error};
+
+use crate::types::{APTRepository, APTRepositoryFileType};
+
+impl APTRepository {
+ /// Makes sure that all basic properties of a repository are present and
+ /// not obviously invalid.
+ pub fn basic_check(&self) -> Result<(), Error> {
+ if self.types.is_empty() {
+ bail!("missing package type(s)");
+ }
+ if self.uris.is_empty() {
+ bail!("missing URI(s)");
+ }
+ if self.suites.is_empty() {
+ bail!("missing suite(s)");
+ }
+
+ for uri in self.uris.iter() {
+ if !uri.contains(':') || uri.len() < 3 {
+ bail!("invalid URI: '{}'", uri);
+ }
+ }
+
+ for suite in self.suites.iter() {
+ if !suite.ends_with('/') && self.components.is_empty() {
+ bail!("missing component(s)");
+ } else if suite.ends_with('/') && !self.components.is_empty() {
+ bail!("absolute suite '{}' does not allow component(s)", suite);
+ }
+ }
+
+ if self.file_type == APTRepositoryFileType::List {
+ if self.types.len() > 1 {
+ bail!("more than one package type");
+ }
+ if self.uris.len() > 1 {
+ bail!("more than one URI");
+ }
+ if self.suites.len() > 1 {
+ bail!("more than one suite");
+ }
+ }
+
+ Ok(())
+ }
+}
diff --git a/src/repositories/list_parser.rs b/src/repositories/list_parser.rs
new file mode 100644
index 0000000..06bb7c2
--- /dev/null
+++ b/src/repositories/list_parser.rs
@@ -0,0 +1,176 @@
+use std::convert::TryInto;
+use std::io::BufRead;
+use std::iter::{Iterator, Peekable};
+use std::str::SplitAsciiWhitespace;
+
+use anyhow::{bail, format_err, Error};
+
+use super::APTRepositoryParser;
+use crate::types::{APTRepository, APTRepositoryFileType, APTRepositoryOption};
+
+pub struct APTListFileParser<R: BufRead> {
+ path: String,
+ input: R,
+ line_nr: usize,
+ comment: String,
+}
+
+impl<R: BufRead> APTListFileParser<R> {
+ pub fn new(path: String, reader: R) -> Self {
+ Self {
+ path,
+ input: reader,
+ line_nr: 0,
+ comment: String::new(),
+ }
+ }
+
+ /// Helper to parse options from the existing token stream.
+ /// Also returns `Ok(())` if there are no options.
+ /// Errors when options are invalid or not closed by `']'`.
+ fn parse_options(
+ options: &mut Vec<APTRepositoryOption>,
+ tokens: &mut Peekable<SplitAsciiWhitespace>,
+ ) -> Result<(), Error> {
+ let mut option = match tokens.peek() {
+ Some(token) => {
+ match token.strip_prefix('[') {
+ Some(option) => option,
+ None => return Ok(()), // doesn't look like options
+ }
+ }
+ None => return Ok(()),
+ };
+
+ tokens.next(); // avoid reading the beginning twice
+
+ let mut finished = false;
+ loop {
+ if let Some(stripped) = option.strip_suffix(']') {
+ option = stripped;
+ if option.is_empty() {
+ break;
+ }
+ finished = true; // but still need to handle the last one
+ };
+
+ if let Some(mid) = option.find('=') {
+ let (key, mut value_str) = option.split_at(mid);
+ value_str = &value_str[1..];
+
+ if key.is_empty() {
+ bail!("option has no key: '{}'", option);
+ }
+
+ if value_str.is_empty() {
+ bail!("option has no value: '{}'", option);
+ }
+
+ let values: Vec<String> = value_str
+ .split(',')
+ .map(|value| value.to_string())
+ .collect();
+
+ options.push(APTRepositoryOption {
+ key: key.to_string(),
+ values,
+ });
+ } else if !option.is_empty() {
+ bail!("got invalid option - '{}'", option);
+ }
+
+ if finished {
+ break;
+ }
+
+ option = match tokens.next() {
+ Some(option) => option,
+ None => bail!("options not closed by ']'"),
+ }
+ }
+
+ Ok(())
+ }
+
+ /// Parse a repository or comment in one-line format.
+ /// Commented out repositories are also detected and returned with the
+ /// `enabled` property set to `false`.
+ /// If the line contains a repository, `self.comment` is added to the
+ /// `comment` property.
+ /// If the line contains a comment, it is added to `self.comment`.
+ fn parse_one_line(&mut self, mut line: &str) -> Result<Option<APTRepository>, Error> {
+ line = line.trim_matches(|c| char::is_ascii_whitespace(&c));
+
+ // check for commented out repository first
+ if let Some(commented_out) = line.strip_prefix('#') {
+ if let Ok(Some(mut repo)) = self.parse_one_line(commented_out) {
+ repo.set_enabled(false);
+ return Ok(Some(repo));
+ }
+ }
+
+ let mut repo =
+ APTRepository::new(self.path.clone(), self.line_nr, APTRepositoryFileType::List);
+
+ // now handle "real" comment
+ if let Some(comment_start) = line.find('#') {
+ let (line_start, comment) = line.split_at(comment_start);
+ self.comment = format!("{}{}\n", self.comment, &comment[1..]);
+ line = line_start;
+ }
+
+ let mut tokens = line.split_ascii_whitespace().peekable();
+
+ match tokens.next() {
+ Some(package_type) => {
+ repo.types.push(package_type.try_into()?);
+ }
+ None => return Ok(None), // empty line
+ }
+
+ Self::parse_options(&mut repo.options, &mut tokens)?;
+
+ // the rest of the line is just '<uri> <suite> [<components>...]'
+ let mut tokens = tokens.map(str::to_string);
+ repo.uris
+ .push(tokens.next().ok_or_else(|| format_err!("missing URI"))?);
+ repo.suites
+ .push(tokens.next().ok_or_else(|| format_err!("missing suite"))?);
+ repo.components.extend(tokens);
+
+ repo.comment = std::mem::take(&mut self.comment);
+
+ Ok(Some(repo))
+ }
+}
+
+impl<R: BufRead> APTRepositoryParser for APTListFileParser<R> {
+ fn parse_repositories(&mut self, repos: &mut Vec<APTRepository>) -> Result<(), Error> {
+ let mut line = String::new();
+
+ loop {
+ self.line_nr += 1;
+ line.clear();
+
+ match self.input.read_line(&mut line) {
+ Err(err) => bail!("input error for '{}' - {}", self.path, err),
+ Ok(0) => break,
+ Ok(_) => match self.parse_one_line(&line) {
+ Ok(Some(repo)) => {
+ repos.push(repo);
+ self.comment.clear();
+ }
+ Ok(None) => continue,
+ Err(err) => bail!(
+ "malformed entry in '{}' line {} - {}",
+ self.path,
+ self.line_nr,
+ err,
+ ),
+ },
+ }
+ }
+
+ Ok(())
+ }
+}
diff --git a/src/repositories/mod.rs b/src/repositories/mod.rs
new file mode 100644
index 0000000..5c446af
--- /dev/null
+++ b/src/repositories/mod.rs
@@ -0,0 +1,256 @@
+use std::cmp::Ordering;
+use std::collections::BTreeMap;
+use std::convert::TryFrom;
+use std::ffi::OsString;
+use std::path::{Path, PathBuf};
+
+use anyhow::{bail, format_err, Error};
+
+use crate::types::{APTRepository, APTRepositoryFileType, APTRepositoryOption};
+
+mod list_parser;
+use list_parser::APTListFileParser;
+
+mod sources_parser;
+use sources_parser::APTSourcesFileParser;
+
+mod check;
+mod writer;
+
+const APT_SOURCES_LIST_FILENAME: &str = "/etc/apt/sources.list";
+const APT_SOURCES_LIST_DIRECTORY: &str = "/etc/apt/sources.list.d/";
+
+impl APTRepository {
+ /// Crates an empty repository with infomration that is known before parsing.
+ fn new(path: String, number: usize, file_type: APTRepositoryFileType) -> Self {
+ Self {
+ types: vec![],
+ uris: vec![],
+ suites: vec![],
+ components: vec![],
+ options: vec![],
+ comment: String::new(),
+ path,
+ number,
+ file_type,
+ enabled: true,
+ }
+ }
+
+ /// Changes the `enabled` flag and makes sure the `Enabled` option for
+ /// `APTRepositoryPackageType::Sources` repositories is updated too.
+ fn set_enabled(&mut self, enabled: bool) {
+ self.enabled = enabled;
+
+ if self.file_type == APTRepositoryFileType::Sources {
+ let enabled_string = match enabled {
+ true => "true".to_string(),
+ false => "false".to_string(),
+ };
+ for option in self.options.iter_mut() {
+ if option.key == "Enabled" {
+ option.values = vec![enabled_string];
+ return;
+ }
+ }
+ self.options.push(APTRepositoryOption {
+ key: "Enabled".to_string(),
+ values: vec![enabled_string],
+ });
+ }
+ }
+}
+
+trait APTRepositoryParser {
+ /// Parse all repositories including the disabled ones and push them onto
+ /// the provided vector.
+ fn parse_repositories(&mut self, repos: &mut Vec<APTRepository>) -> Result<(), Error>;
+}
+
+/// Helper to decide whether a file name is considered valid by APT and to
+/// extract its file type and the path as a string.
+/// Hidden files yield `Ok(None)`, while invalid file names yield an error.
+fn check_filename<P: AsRef<Path>>(
+ path: P,
+) -> Result<Option<(APTRepositoryFileType, String)>, OsString> {
+ let path: PathBuf = path.as_ref().to_path_buf();
+ let path_string = path.clone().into_os_string().into_string()?;
+
+ let file_name = match path.file_name() {
+ Some(file_name) => file_name.to_os_string().into_string()?,
+ None => return Err(OsString::from(path_string)),
+ };
+
+ // APT silently ignores hidden files
+ if file_name.starts_with('.') {
+ return Ok(None);
+ }
+
+ let extension = match path.extension() {
+ Some(extension) => extension.to_os_string().into_string()?,
+ None => return Err(OsString::from(path_string)),
+ };
+
+ let file_type = match APTRepositoryFileType::try_from(&extension[..]) {
+ Ok(file_type) => file_type,
+ _ => return Err(OsString::from(path_string)),
+ };
+
+ // APT ignores such files but issues a warning
+ if !file_name
+ .chars()
+ .all(|x| x.is_ascii_alphanumeric() || x == '_' || x == '-' || x == '.')
+ {
+ return Err(OsString::from(path_string));
+ }
+
+ Ok(Some((file_type, path_string)))
+}
+
+/// Similar to [`repositories_from_files`], but for a single file, and adds the
+/// parsed repositories onto the provided vector instead. Another difference is
+/// that it doesn't call [`basic_check`](check::basic_check).
+fn repositories_from_file(path: &Path, repos: &mut Vec<APTRepository>) -> Result<(), Error> {
+ if !path.is_file() {
+ eprintln!("Ignoring {:?} - not a file", path);
+ return Ok(());
+ }
+
+ let file_type;
+ let path_string;
+
+ match check_filename(path) {
+ Ok(Some(res)) => {
+ file_type = res.0;
+ path_string = res.1;
+ }
+ Ok(None) => return Ok(()),
+ Err(path) => {
+ eprintln!("Ignoring {:?} - invalid file name", path);
+ return Ok(());
+ }
+ }
+
+ let contents =
+ std::fs::read(path).map_err(|err| format_err!("unable to read {:?} - {}", path, err))?;
+
+ let mut parser: Box<dyn APTRepositoryParser> = match file_type {
+ APTRepositoryFileType::List => Box::new(APTListFileParser::new(path_string, &contents[..])),
+ APTRepositoryFileType::Sources => {
+ Box::new(APTSourcesFileParser::new(path_string, &contents[..]))
+ }
+ };
+
+ parser.parse_repositories(repos)?;
+
+ Ok(())
+}
+
+/// Returns all APT repositories configured in the specified files, including
+/// disabled ones.
+/// Warns about invalid file names and some format violations, while other
+/// format violations result in an error.
+pub fn repositories_from_files<P: AsRef<Path>>(paths: &[P]) -> Result<Vec<APTRepository>, Error> {
+ let mut repos = Vec::<APTRepository>::new();
+
+ for path in paths.iter() {
+ repositories_from_file(path.as_ref(), &mut repos)?;
+ }
+
+ for repo in repos.iter() {
+ repo.basic_check().map_err(|err| {
+ format_err!("check for {}:{} failed - {}", repo.path, repo.number, err)
+ })?;
+ }
+
+ Ok(repos)
+}
+
+/// Returns all APT repositories configured in `/etc/apt/sources.list` and
+/// in `/etc/apt/sources.list.d` including disabled repositories.
+/// Warns about invalid file names and some format violations, while other
+/// format violations result in an error.
+pub fn repositories() -> Result<Vec<APTRepository>, Error> {
+ let mut paths = Vec::<PathBuf>::new();
+
+ let sources_list_path = PathBuf::from(APT_SOURCES_LIST_FILENAME);
+ if sources_list_path.is_file() {
+ paths.push(sources_list_path)
+ };
+
+ let sources_list_d_path = PathBuf::from(APT_SOURCES_LIST_DIRECTORY);
+ if sources_list_d_path.is_dir() {
+ for entry in std::fs::read_dir(sources_list_d_path)? {
+ paths.push(entry?.path());
+ }
+ }
+
+ let repos = repositories_from_files(&paths)?;
+
+ Ok(repos)
+}
+
+/// See [`write_repositories`]. Will sort the vector by the repository's file
+/// name and number.
+fn write_repositories_ref_vec(repos: &mut Vec<&APTRepository>) -> Result<(), Error> {
+ // check before writing
+ for repo in repos.iter() {
+ repo.basic_check().map_err(|err| {
+ format_err!("check for {}:{} failed - {}", repo.path, repo.number, err)
+ })?;
+ }
+
+ repos.sort_by(|a, b| match a.path.cmp(&b.path) {
+ Ordering::Equal => a.number.cmp(&b.number),
+ ord => ord,
+ });
+
+ let mut files = BTreeMap::<String, Vec<u8>>::new();
+
+ for repo in repos.iter() {
+ let raw = match files.get_mut(&repo.path) {
+ Some(raw) => raw,
+ None => {
+ files.insert(repo.path.clone(), vec![]);
+ files.get_mut(&repo.path).unwrap()
+ }
+ };
+
+ repo.write(&mut *raw)
+ .map_err(|err| format_err!("writing {}:{} failed - {}", repo.path, repo.number, err))?;
+ }
+
+ for (path, content) in files.iter() {
+ let path = PathBuf::from(path);
+ let dir = path.parent().unwrap();
+
+ std::fs::create_dir_all(dir)
+ .map_err(|err| format_err!("unable to create dir {:?} - {}", dir, err))?;
+
+ let pid = std::process::id();
+ let mut tmp_path = path.clone();
+ tmp_path.set_extension("tmp");
+ tmp_path.set_extension(format!("{}", pid));
+
+ if let Err(err) = std::fs::write(&tmp_path, &content) {
+ let _ = std::fs::remove_file(&tmp_path);
+ bail!("write failed: {}", err);
+ }
+
+ if let Err(err) = std::fs::rename(&tmp_path, &path) {
+ let _ = std::fs::remove_file(&tmp_path);
+ bail!("rename failed for {:?} - {}", path, err);
+ }
+ }
+
+ Ok(())
+}
+
+/// Write the repositories to the respective files specified by their
+/// `path` property and in the order determined by their `number` property.
+/// Does a `check::basic_check(repository)` for each repository first.
+pub fn write_repositories<A: AsRef<APTRepository>>(repos: &[A]) -> Result<(), Error> {
+ let mut repos: Vec<&APTRepository> = repos.iter().map(|repo| repo.as_ref()).collect();
+
+ write_repositories_ref_vec(&mut repos)
+}
diff --git a/src/repositories/sources_parser.rs b/src/repositories/sources_parser.rs
new file mode 100644
index 0000000..5f25d33
--- /dev/null
+++ b/src/repositories/sources_parser.rs
@@ -0,0 +1,214 @@
+use std::convert::TryInto;
+use std::io::BufRead;
+use std::iter::Iterator;
+
+use anyhow::{bail, Error};
+
+use crate::types::{
+ APTRepository, APTRepositoryFileType, APTRepositoryOption, APTRepositoryPackageType,
+};
+
+use super::APTRepositoryParser;
+
+pub struct APTSourcesFileParser<R: BufRead> {
+ path: String,
+ input: R,
+ stanza_nr: usize,
+ comment: String,
+}
+
+/// See `man sources.list` and `man deb822` for the format specification.
+impl<R: BufRead> APTSourcesFileParser<R> {
+ pub fn new(path: String, reader: R) -> Self {
+ Self {
+ path,
+ input: reader,
+ stanza_nr: 1,
+ comment: String::new(),
+ }
+ }
+
+ /// Based on APT's `StringToBool` in `strutl.cc`
+ fn string_to_bool(string: &str, default: bool) -> bool {
+ let string = string.trim_matches(|c| char::is_ascii_whitespace(&c));
+ let string = string.to_lowercase();
+
+ match &string[..] {
+ "1" | "yes" | "true" | "with" | "on" | "enable" => true,
+ "0" | "no" | "false" | "without" | "off" | "disable" => false,
+ _ => default,
+ }
+ }
+
+ /// Checks if `key` is valid according to deb822
+ fn valid_key(key: &str) -> bool {
+ if key.starts_with('-') {
+ return false;
+ };
+ return key.chars().all(|c| matches!(c, '!'..='9' | ';'..='~'));
+ }
+
+ /// Try parsing a repository in stanza format from `lines`.
+ /// Returns `Ok(None)` when no stanza can be found.
+ /// Comments are added to `self.comments`. If a stanza can be found,
+ /// `self.comment` is added to the repository's `comment` property.
+ /// Fully commented out stanzas are treated as comments.
+ fn parse_stanza(&mut self, lines: &str) -> Result<Option<APTRepository>, Error> {
+ let mut repo = APTRepository::new(
+ self.path.clone(),
+ self.stanza_nr,
+ APTRepositoryFileType::Sources,
+ );
+
+ // Values may be folded into multiple lines.
+ // Those lines have to start with a space or a tab.
+ let lines = lines.replace("\n ", " ");
+ let lines = lines.replace("\n\t", " ");
+
+ let mut got_something = false;
+
+ for line in lines.lines() {
+ let line = line.trim_matches(|c| char::is_ascii_whitespace(&c));
+ if line.is_empty() {
+ continue;
+ }
+
+ if let Some(commented_out) = line.strip_prefix('#') {
+ self.comment = format!("{}{}\n", self.comment, commented_out);
+ continue;
+ }
+
+ if let Some(mid) = line.find(':') {
+ let (key, value_str) = line.split_at(mid);
+ let value_str = &value_str[1..];
+ let key = key.trim_matches(|c| char::is_ascii_whitespace(&c));
+
+ if key.is_empty() {
+ bail!("option has no key: '{}'", line);
+ }
+
+ if value_str.is_empty() {
+ // ignored by APT
+ eprintln!("option has no value: '{}'", line);
+ continue;
+ }
+
+ if !Self::valid_key(key) {
+ // ignored by APT
+ eprintln!("option with invalid key '{}'", key);
+ continue;
+ }
+
+ let values: Vec<String> = value_str
+ .split_ascii_whitespace()
+ .map(|value| value.to_string())
+ .collect();
+
+ match key {
+ "Types" => {
+ if !repo.types.is_empty() {
+ eprintln!("key 'Types' was defined twice");
+ }
+ let mut types = Vec::<APTRepositoryPackageType>::new();
+ for package_type in values {
+ types.push((&package_type[..]).try_into()?);
+ }
+ repo.types = types;
+ }
+ "URIs" => {
+ if !repo.uris.is_empty() {
+ eprintln!("key 'URIs' was defined twice");
+ }
+ repo.uris = values;
+ }
+ "Suites" => {
+ if !repo.suites.is_empty() {
+ eprintln!("key 'Suites' was defined twice");
+ }
+ repo.suites = values;
+ }
+ "Components" => {
+ if !repo.components.is_empty() {
+ eprintln!("key 'Components' was defined twice");
+ }
+ repo.components = values;
+ }
+ "Enabled" => {
+ repo.set_enabled(Self::string_to_bool(value_str, true));
+ }
+ _ => repo.options.push(APTRepositoryOption {
+ key: key.to_string(),
+ values,
+ }),
+ }
+ } else {
+ bail!("got invalid line - '{:?}'", line);
+ }
+
+ got_something = true;
+ }
+
+ if !got_something {
+ return Ok(None);
+ }
+
+ repo.comment = std::mem::take(&mut self.comment);
+
+ Ok(Some(repo))
+ }
+
+ /// Helper function for `parse_repositories`.
+ fn try_parse_stanza(
+ &mut self,
+ lines: &str,
+ repos: &mut Vec<APTRepository>,
+ ) -> Result<(), Error> {
+ match self.parse_stanza(&lines[..]) {
+ Ok(Some(repo)) => {
+ repos.push(repo);
+ self.comment.clear();
+ self.stanza_nr += 1;
+ }
+ Ok(None) => (),
+ Err(err) => {
+ bail!(
+ "malformed entry in '{}' stanza {} - {}",
+ self.path,
+ self.stanza_nr,
+ err,
+ );
+ }
+ }
+
+ Ok(())
+ }
+}
+
+impl<R: BufRead> APTRepositoryParser for APTSourcesFileParser<R> {
+ fn parse_repositories(&mut self, repos: &mut Vec<APTRepository>) -> Result<(), Error> {
+ let mut lines = String::new();
+
+ loop {
+ let old_length = lines.len();
+ match self.input.read_line(&mut lines) {
+ Err(err) => bail!("input error for '{}' - {}", self.path, err),
+ Ok(0) => {
+ self.try_parse_stanza(&lines[..], repos)?;
+ break;
+ }
+ Ok(_) => {
+ if (&lines[old_length..])
+ .trim_matches(|c| char::is_ascii_whitespace(&c))
+ .is_empty()
+ {
+ // detected end of stanza
+ self.try_parse_stanza(&lines[..], repos)?;
+ lines.clear();
+ }
+ }
+ }
+ }
+
+ Ok(())
+ }
+}
diff --git a/src/repositories/writer.rs b/src/repositories/writer.rs
new file mode 100644
index 0000000..76ea6ea
--- /dev/null
+++ b/src/repositories/writer.rs
@@ -0,0 +1,85 @@
+use std::io::Write;
+
+use anyhow::{bail, Error};
+
+use crate::types::{APTRepository, APTRepositoryFileType};
+
+impl APTRepository {
+ /// Writes a repository in the corresponding format followed by a blank.
+ /// Expects that `basic_check()` for the repository was successful.
+ pub fn write(&self, w: &mut dyn Write) -> Result<(), Error> {
+ match self.file_type {
+ APTRepositoryFileType::List => write_one_line(self, w),
+ APTRepositoryFileType::Sources => write_stanza(self, w),
+ }
+ }
+}
+
+/// Writes a repository in one-line format followed by a blank line.
+/// Expects that `repo.file_type == APTRepositoryFileType::List`.
+/// Expects that `basic_check()` for the repository was successful.
+fn write_one_line(repo: &APTRepository, w: &mut dyn Write) -> Result<(), Error> {
+ if repo.file_type != APTRepositoryFileType::List {
+ bail!("not a .list repository");
+ }
+
+ if !repo.comment.is_empty() {
+ for line in repo.comment.lines() {
+ writeln!(w, "#{}", line)?;
+ }
+ }
+
+ if !repo.enabled {
+ write!(w, "# ")?;
+ }
+
+ write!(w, "{} ", repo.types[0])?;
+
+ if !repo.options.is_empty() {
+ write!(w, "[ ")?;
+ repo.options
+ .iter()
+ .try_for_each(|option| write!(w, "{}={} ", option.key, option.values.join(",")))?;
+ write!(w, "] ")?;
+ };
+
+ write!(w, "{} ", repo.uris[0])?;
+ write!(w, "{} ", repo.suites[0])?;
+ writeln!(w, "{}", repo.components.join(" "))?;
+
+ writeln!(w)?;
+
+ Ok(())
+}
+
+/// Writes a single stanza followed by a blank line.
+/// Expects that `repo.file_type == APTRepositoryFileType::Sources`.
+fn write_stanza(repo: &APTRepository, w: &mut dyn Write) -> Result<(), Error> {
+ if repo.file_type != APTRepositoryFileType::Sources {
+ bail!("not a .sources repository");
+ }
+
+ if !repo.comment.is_empty() {
+ for line in repo.comment.lines() {
+ writeln!(w, "#{}", line)?;
+ }
+ }
+
+ write!(w, "Types:")?;
+ repo.types
+ .iter()
+ .try_for_each(|package_type| write!(w, " {}", package_type))?;
+ writeln!(w)?;
+
+ writeln!(w, "URIs: {}", repo.uris.join(" "))?;
+ writeln!(w, "Suites: {}", repo.suites.join(" "))?;
+ writeln!(w, "Components: {}", repo.components.join(" "))?;
+
+ for option in repo.options.iter() {
+ writeln!(w, "{}: {}", option.key, option.values.join(" "))?;
+ }
+
+ writeln!(w)?;
+
+ Ok(())
+}
diff --git a/src/types.rs b/src/types.rs
new file mode 100644
index 0000000..be69652
--- /dev/null
+++ b/src/types.rs
@@ -0,0 +1,211 @@
+use std::convert::TryFrom;
+use std::fmt::Display;
+
+use anyhow::{bail, Error};
+use serde::{Deserialize, Serialize};
+
+use proxmox::api::api;
+
+#[api]
+#[derive(Debug, Copy, Clone, Serialize, Deserialize, PartialEq)]
+#[serde(rename_all = "lowercase")]
+pub enum APTRepositoryFileType {
+ /// One-line-style format
+ List,
+ /// DEB822-style format
+ Sources,
+}
+
+impl TryFrom<&str> for APTRepositoryFileType {
+ type Error = Error;
+
+ fn try_from(string: &str) -> Result<Self, Error> {
+ match &string[..] {
+ "list" => Ok(APTRepositoryFileType::List),
+ "sources" => Ok(APTRepositoryFileType::Sources),
+ _ => bail!("invalid file type '{}'", string),
+ }
+ }
+}
+
+impl Display for APTRepositoryFileType {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ APTRepositoryFileType::List => write!(f, "list"),
+ APTRepositoryFileType::Sources => write!(f, "sources"),
+ }
+ }
+}
+
+#[api]
+#[derive(Debug, Copy, Clone, Serialize, Deserialize, PartialEq)]
+#[serde(rename_all = "kebab-case")]
+pub enum APTRepositoryPackageType {
+ /// Debian package
+ Deb,
+ /// Debian source package
+ DebSrc,
+}
+
+impl TryFrom<&str> for APTRepositoryPackageType {
+ type Error = Error;
+
+ fn try_from(string: &str) -> Result<Self, Error> {
+ match &string[..] {
+ "deb" => Ok(APTRepositoryPackageType::Deb),
+ "deb-src" => Ok(APTRepositoryPackageType::DebSrc),
+ _ => bail!("invalid file type '{}'", string),
+ }
+ }
+}
+
+impl Display for APTRepositoryPackageType {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ APTRepositoryPackageType::Deb => write!(f, "deb"),
+ APTRepositoryPackageType::DebSrc => write!(f, "deb-src"),
+ }
+ }
+}
+
+#[api(
+ properties: {
+ Key: {
+ description: "Option key.",
+ type: String,
+ },
+ Values: {
+ description: "Option values.",
+ type: Array,
+ items: {
+ description: "Value.",
+ type: String,
+ },
+ },
+ },
+)]
+#[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(rename_all = "PascalCase")] // for consistency
+/// Additional options for an APT repository.
+/// Used for both single- and mutli-value options.
+pub struct APTRepositoryOption {
+ /// Option key.
+ pub key: String,
+ /// Option value(s).
+ pub values: Vec<String>,
+}
+
+#[api(
+ properties: {
+ Types: {
+ description: "List of package types.",
+ type: Array,
+ items: {
+ type: APTRepositoryPackageType,
+ },
+ },
+ URIs: {
+ description: "List of repository URIs.",
+ type: Array,
+ items: {
+ description: "Repository URI.",
+ type: String,
+ },
+ },
+ Suites: {
+ description: "List of distributions.",
+ type: Array,
+ items: {
+ description: "Package distribution.",
+ type: String,
+ },
+ },
+ Components: {
+ description: "List of repository components.",
+ type: Array,
+ items: {
+ description: "Repository component.",
+ type: String,
+ },
+ },
+ Options: {
+ type: Array,
+ optional: true,
+ items: {
+ type: APTRepositoryOption,
+ },
+ },
+ Comment: {
+ description: "Associated comment.",
+ type: String,
+ optional: true,
+ },
+ Path: {
+ description: "Path to the defining file.",
+ type: String,
+ },
+ Number: {
+ description: "Line or stanza number.",
+ type: Integer,
+ },
+ FileType: {
+ type: APTRepositoryFileType,
+ },
+ Enabled: {
+ description: "Whether the repository is enabled or not.",
+ type: Boolean,
+ },
+ },
+)]
+#[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(rename_all = "PascalCase")]
+/// Describes an APT repository.
+pub struct APTRepository {
+ /// List of package types.
+ #[serde(skip_serializing_if = "Vec::is_empty")]
+ pub types: Vec<APTRepositoryPackageType>,
+
+ /// List of repository URIs.
+ #[serde(skip_serializing_if = "Vec::is_empty")]
+ #[serde(rename = "URIs")]
+ pub uris: Vec<String>,
+
+ /// List of package distributions.
+ #[serde(skip_serializing_if = "Vec::is_empty")]
+ pub suites: Vec<String>,
+
+ /// List of repository components.
+ #[serde(skip_serializing_if = "Vec::is_empty")]
+ pub components: Vec<String>,
+
+ /// Additional options.
+ #[serde(skip_serializing_if = "Vec::is_empty")]
+ pub options: Vec<APTRepositoryOption>,
+
+ /// Associated comment.
+ #[serde(skip_serializing_if = "String::is_empty")]
+ pub comment: String,
+
+ /// Path to the defining file.
+ #[serde(skip_serializing_if = "String::is_empty")]
+ pub path: String,
+
+ /// Line or stanza number.
+ pub number: usize,
+
+ /// Format of the defining file.
+ pub file_type: APTRepositoryFileType,
+
+ /// Whether the repository is enabled or not.
+ pub enabled: bool,
+}
+
+/// Some functions like write_repositiories can be called with either a slice of
+/// [`APTRepository`]s or a slice of references thereof. Thus, users of the
+/// crate are more flexibility in working with collections of repositories. See
+/// the test_parse_write test for an example.
+impl AsRef<APTRepository> for APTRepository {
+ fn as_ref(&self) -> &APTRepository {
+ &self
+ }
+}
diff --git a/tests/repositories.rs b/tests/repositories.rs
new file mode 100644
index 0000000..020e133
--- /dev/null
+++ b/tests/repositories.rs
@@ -0,0 +1,73 @@
+use std::collections::HashMap;
+use std::path::PathBuf;
+
+use anyhow::{bail, format_err, Error};
+
+use proxmox_apt::repositories::{repositories_from_files, write_repositories};
+use proxmox_apt::types::APTRepository;
+
+#[test]
+fn test_parse_write() -> Result<(), Error> {
+ let test_dir = std::env::current_dir()?.join("tests");
+ let read_dir = test_dir.join("sources.list.d");
+ let write_dir = test_dir.join("sources.list.d.actual");
+ let expected_dir = test_dir.join("sources.list.d.expected");
+
+ let mut paths = Vec::<PathBuf>::new();
+ for entry in std::fs::read_dir(read_dir)? {
+ paths.push(entry?.path());
+ }
+
+ let repos = repositories_from_files(&paths)?;
+
+ // used to mess up the order from parsing and to check that each repo has a
+ // unique path:number
+ let mut repo_hash = HashMap::<String, APTRepository>::new();
+
+ for mut repo in repos {
+ let path = PathBuf::from(repo.path);
+ let new_path = write_dir.join(path.file_name().unwrap());
+
+ repo.path = new_path.into_os_string().into_string().unwrap();
+
+ let key = format!("{}:{}", repo.path, repo.number);
+
+ if repo_hash.insert(key.clone(), repo).is_some() {
+ bail!("key '{}' is not unique!", key);
+ }
+ }
+
+ if write_dir.is_dir() {
+ std::fs::remove_dir_all(&write_dir)
+ .map_err(|err| format_err!("unable to remove dir {:?} - {}", write_dir, err))?;
+ }
+
+ std::fs::create_dir_all(&write_dir)
+ .map_err(|err| format_err!("unable to create dir {:?} - {}", write_dir, err))?;
+
+ let repos_vec: Vec<&APTRepository> = repo_hash.values().collect();
+ write_repositories(&repos_vec)?;
+
+ let mut expected_count = 0;
+
+ for entry in std::fs::read_dir(expected_dir)? {
+ expected_count += 1;
+
+ let expected_path = entry?.path();
+ let actual_path = write_dir.join(expected_path.file_name().unwrap());
+
+ let expected_contents = std::fs::read(&expected_path)
+ .map_err(|err| format_err!("unable to read {:?} - {}", expected_path, err))?;
+
+ let actual_contents = std::fs::read(&actual_path)
+ .map_err(|err| format_err!("unable to read {:?} - {}", actual_path, err))?;
+
+ assert_eq!(expected_contents, actual_contents);
+ }
+
+ let actual_count = std::fs::read_dir(write_dir)?.count();
+
+ assert_eq!(expected_count, actual_count);
+
+ Ok(())
+}
diff --git a/tests/sources.list.d.expected/multiline.sources b/tests/sources.list.d.expected/multiline.sources
new file mode 100644
index 0000000..91f53c2
--- /dev/null
+++ b/tests/sources.list.d.expected/multiline.sources
@@ -0,0 +1,8 @@
+# comment in here
+Types: deb deb-src
+URIs: http://ftp.at.debian.org/debian
+Suites: buster buster-updates
+Components: main contrib
+Languages: it de fr
+Enabled: false
+
--git a/tests/sources.list.d.expected/options_comment.list b/tests/sources.list.d.expected/options_comment.list
new file mode 100644
index 0000000..f0952e4
--- /dev/null
+++ b/tests/sources.list.d.expected/options_comment.list
@@ -0,0 +1,3 @@
+# comment
+deb [ lang=it,de arch=amd64 ] http://ftp.at.debian.org/debian buster main contrib
+
diff --git a/tests/sources.list.d.expected/pbs-enterprise.list b/tests/sources.list.d.expected/pbs-enterprise.list
new file mode 100644
index 0000000..acb2990
--- /dev/null
+++ b/tests/sources.list.d.expected/pbs-enterprise.list
@@ -0,0 +1,2 @@
+deb https://enterprise.proxmox.com/debian/pbs buster pbs-enterprise
+
diff --git a/tests/sources.list.d.expected/pve.list b/tests/sources.list.d.expected/pve.list
new file mode 100644
index 0000000..127a49a
--- /dev/null
+++ b/tests/sources.list.d.expected/pve.list
@@ -0,0 +1,13 @@
+deb http://ftp.debian.org/debian buster main contrib
+
+deb http://ftp.debian.org/debian buster-updates main contrib
+
+# PVE pve-no-subscription repository provided by proxmox.com,
+# NOT recommended for production use
+deb http://download.proxmox.com/debian/pve buster pve-no-subscription
+
+# deb https://enterprise.proxmox.com/debian/pve buster pve-enterprise
+
+# security updates
+deb http://security.debian.org/debian-security buster/updates main contrib
+
diff --git a/tests/sources.list.d.expected/standard.list b/tests/sources.list.d.expected/standard.list
new file mode 100644
index 0000000..63c1b60
--- /dev/null
+++ b/tests/sources.list.d.expected/standard.list
@@ -0,0 +1,7 @@
+deb http://ftp.at.debian.org/debian buster main contrib
+
+deb http://ftp.at.debian.org/debian buster-updates main contrib
+
+# security updates
+deb http://security.debian.org buster/updates main contrib
+
diff --git a/tests/sources.list.d.expected/standard.sources b/tests/sources.list.d.expected/standard.sources
new file mode 100644
index 0000000..56ce280
--- /dev/null
+++ b/tests/sources.list.d.expected/standard.sources
@@ -0,0 +1,11 @@
+Types: deb
+URIs: http://ftp.at.debian.org/debian
+Suites: buster buster-updates
+Components: main contrib
+
+# security updates
+Types: deb
+URIs: http://security.debian.org
+Suites: buster/updates
+Components: main contrib
+
diff --git a/tests/sources.list.d/multiline.sources b/tests/sources.list.d/multiline.sources
new file mode 100644
index 0000000..c3a1ff0
--- /dev/null
+++ b/tests/sources.list.d/multiline.sources
@@ -0,0 +1,9 @@
+Types: deb deb-src
+URIs: http://ftp.at.debian.org/debian
+Suites: buster buster-updates
+# comment in here
+Components: main contrib
+Languages: it
+ de
+ fr
+Enabled: off
--git a/tests/sources.list.d/options_comment.list b/tests/sources.list.d/options_comment.list
new file mode 100644
index 0000000..e3f4112
--- /dev/null
+++ b/tests/sources.list.d/options_comment.list
@@ -0,0 +1,2 @@
+deb [ lang=it,de arch=amd64 ] http://ftp.at.debian.org/debian buster main contrib # comment
+
diff --git a/tests/sources.list.d/pbs-enterprise.list b/tests/sources.list.d/pbs-enterprise.list
new file mode 100644
index 0000000..5f8763c
--- /dev/null
+++ b/tests/sources.list.d/pbs-enterprise.list
@@ -0,0 +1 @@
+deb https://enterprise.proxmox.com/debian/pbs buster pbs-enterprise
diff --git a/tests/sources.list.d/pve.list b/tests/sources.list.d/pve.list
new file mode 100644
index 0000000..6213f72
--- /dev/null
+++ b/tests/sources.list.d/pve.list
@@ -0,0 +1,10 @@
+deb http://ftp.debian.org/debian buster main contrib
+deb http://ftp.debian.org/debian buster-updates main contrib
+
+# PVE pve-no-subscription repository provided by proxmox.com,
+# NOT recommended for production use
+deb http://download.proxmox.com/debian/pve buster pve-no-subscription
+# deb https://enterprise.proxmox.com/debian/pve buster pve-enterprise
+
+# security updates
+deb http://security.debian.org/debian-security buster/updates main contrib
diff --git a/tests/sources.list.d/standard.list b/tests/sources.list.d/standard.list
new file mode 100644
index 0000000..26db887
--- /dev/null
+++ b/tests/sources.list.d/standard.list
@@ -0,0 +1,6 @@
+deb http://ftp.at.debian.org/debian buster main contrib
+
+deb http://ftp.at.debian.org/debian buster-updates main contrib
+
+# security updates
+deb http://security.debian.org buster/updates main contrib
diff --git a/tests/sources.list.d/standard.sources b/tests/sources.list.d/standard.sources
new file mode 100644
index 0000000..605202e
--- /dev/null
+++ b/tests/sources.list.d/standard.sources
@@ -0,0 +1,10 @@
+Types: deb
+URIs: http://ftp.at.debian.org/debian
+Suites: buster buster-updates
+Components: main contrib
+
+# security updates
+Types: deb
+URIs: http://security.debian.org
+Suites: buster/updates
+Components: main contrib
--
2.20.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [pbs-devel] [PATCH v3 proxmox-apt 02/10] add files for Debian packaging
2021-03-22 11:59 [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-apt 01/10] initial commit Fabian Ebner
@ 2021-03-22 11:59 ` Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-apt 03/10] add functions to check for Proxmox repositories Fabian Ebner
` (8 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Fabian Ebner @ 2021-03-22 11:59 UTC (permalink / raw)
To: pbs-devel
The Makefile is based on the one from Mira's conntrack series, as it already got
some review.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
No changes from v2.
.gitignore | 5 ++++
Makefile | 64 ++++++++++++++++++++++++++++++++++++++++++++
debian/changelog | 5 ++++
debian/copyright | 16 +++++++++++
debian/debcargo.toml | 7 +++++
5 files changed, 97 insertions(+)
create mode 100644 Makefile
create mode 100644 debian/changelog
create mode 100644 debian/copyright
create mode 100644 debian/debcargo.toml
diff --git a/.gitignore b/.gitignore
index 0d00c41..6c46431 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,3 +1,8 @@
Cargo.lock
target/
tests/sources.list.d.actual
+proxmox-apt-*/
+*proxmox-apt*.buildinfo
+*proxmox-apt*.tar.?z
+*proxmox-apt*.changes
+*proxmox-apt*.deb
diff --git a/Makefile b/Makefile
new file mode 100644
index 0000000..de3d620
--- /dev/null
+++ b/Makefile
@@ -0,0 +1,64 @@
+include /usr/share/dpkg/pkg-info.mk
+include /usr/share/dpkg/architecture.mk
+
+PACKAGE=proxmox-apt
+BUILDDIR ?= $(PACKAGE)-$(DEB_VERSION_UPSTREAM)
+BUILDDIR_TMP ?= $(BUILDDIR).tmp
+
+DEB=librust-$(PACKAGE)-dev_$(DEB_VERSION_UPSTREAM_REVISION)_$(DEB_BUILD_ARCH).deb
+DSC=rust-$(PACKAGE)_$(DEB_VERSION_UPSTREAM_REVISION).dsc
+
+ifeq ($(BUILD_MODE), release)
+CARGO_BUILD_ARGS += --release
+COMPILEDIR := target/release
+else
+COMPILEDIR := target/debug
+endif
+
+all: cargo-build $(SUBDIRS)
+
+.PHONY: cargo-build
+cargo-build:
+ cargo build $(CARGO_BUILD_ARGS)
+
+.PHONY: build
+build:
+ rm -rf $(BUILDDIR) $(BUILDDIR_TMP); mkdir $(BUILDDIR_TMP)
+ debcargo package \
+ --config debian/debcargo.toml \
+ --changelog-ready \
+ --no-overlay-write-back \
+ --directory $(BUILDDIR_TMP) \
+ $(PACKAGE) \
+ $(shell dpkg-parsechangelog -l debian/changelog -SVersion | sed -e 's/-.*//')
+ rm -f $(BUILDDIR_TMP)/Cargo.lock
+ find $(BUILDDIR_TMP)/debian -name "*.hint" -delete
+ mv $(BUILDDIR_TMP) $(BUILDDIR)
+
+.PHONY: deb
+deb: $(DEB)
+$(DEB): build
+ cd $(BUILDDIR); dpkg-buildpackage -b -us -uc --no-pre-clean
+ lintian $(DEB)
+
+.PHONY: dsc
+dsc: $(DSC)
+$(DSC): build
+ cd $(BUILDDIR); dpkg-buildpackage -S -us -uc -d -nc
+ lintian $(DSC)
+
+.PHONY: dinstall
+dinstall: $(DEB)
+ dpkg -i $(DEB)
+
+.PHONY: upload
+upload: $(DEB) $(DBG_DEB)
+ tar cf - $(DEB) $(DBG_DEB) | ssh -X repoman@repo.proxmox.com -- upload --product pbs,pmg,pve --dist buster --arch $(DEB_BUILD_ARCH)
+
+.PHONY: distclean
+distclean: clean
+
+.PHONY: clean
+clean:
+ rm -rf *.deb *.buildinfo *.changes *.dsc rust-$(PACKAGE)_*.tar.?z $(BUILDDIR) $(BUILDDIR_TMP)
+ find . -name '*~' -exec rm {} ';'
diff --git a/debian/changelog b/debian/changelog
new file mode 100644
index 0000000..11e26ed
--- /dev/null
+++ b/debian/changelog
@@ -0,0 +1,5 @@
+rust-proxmox-apt (0.1.0-1) unstable; urgency=medium
+
+ * Initial release.
+
+ -- Proxmox Support Team <support@proxmox.com> Thu, 18 Feb 2021 10:20:44 +0100
diff --git a/debian/copyright b/debian/copyright
new file mode 100644
index 0000000..5661ef6
--- /dev/null
+++ b/debian/copyright
@@ -0,0 +1,16 @@
+Copyright (C) 2021 Proxmox Server Solutions GmbH
+
+This software is written by Proxmox Server Solutions GmbH <support@proxmox.com>
+
+This program is free software: you can redistribute it and/or modify
+it under the terms of the GNU Affero General Public License as published by
+the Free Software Foundation, either version 3 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+GNU Affero General Public License for more details.
+
+You should have received a copy of the GNU Affero General Public License
+along with this program. If not, see <http://www.gnu.org/licenses/>.
diff --git a/debian/debcargo.toml b/debian/debcargo.toml
new file mode 100644
index 0000000..74e3854
--- /dev/null
+++ b/debian/debcargo.toml
@@ -0,0 +1,7 @@
+overlay = "."
+crate_src_path = ".."
+maintainer = "Proxmox Support Team <support@proxmox.com>"
+
+[source]
+vcs_git = "git://git.proxmox.com/git/proxmox-apt.git"
+vcs_browser = "https://git.proxmox.com/?p=proxmox-apt.git"
--
2.20.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [pbs-devel] [PATCH v3 proxmox-apt 03/10] add functions to check for Proxmox repositories
2021-03-22 11:59 [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-apt 01/10] initial commit Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-apt 02/10] add files for Debian packaging Fabian Ebner
@ 2021-03-22 11:59 ` Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-apt 04/10] add check_repositories function Fabian Ebner
` (7 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Fabian Ebner @ 2021-03-22 11:59 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
No changes from v2.
src/repositories/check.rs | 42 +++++++++++++++++++++++++++++++++++++++
src/repositories/mod.rs | 21 ++++++++++++++++++++
tests/repositories.rs | 29 ++++++++++++++++++++++++++-
3 files changed, 91 insertions(+), 1 deletion(-)
diff --git a/src/repositories/check.rs b/src/repositories/check.rs
index 87fbbac..d0656cd 100644
--- a/src/repositories/check.rs
+++ b/src/repositories/check.rs
@@ -44,4 +44,46 @@ impl APTRepository {
Ok(())
}
+
+ /// Checks if the repository is the no-subscription repository of the specified
+ /// Proxmox product.
+ pub fn is_no_subscription(&self, product: &str) -> bool {
+ let base_uri = "http://download.proxmox.com/debian";
+ let no_subscription_uri = format!("{}/{}", base_uri, product);
+ let no_subscription_component = format!("{}-no-subscription", product);
+
+ if self
+ .uris
+ .iter()
+ .any(|uri| uri.trim_end_matches('/') == no_subscription_uri)
+ {
+ return self
+ .components
+ .iter()
+ .any(|comp| *comp == no_subscription_component);
+ } else {
+ false
+ }
+ }
+
+ /// Checks if the repository is the enterprise repository of the specified
+ /// Proxmox product.
+ pub fn is_enterprise(&self, product: &str) -> bool {
+ let base_uri = "https://enterprise.proxmox.com/debian";
+ let enterprise_uri = format!("{}/{}", base_uri, product);
+ let enterprise_component = format!("{}-enterprise", product);
+
+ if self
+ .uris
+ .iter()
+ .any(|uri| uri.trim_end_matches('/') == enterprise_uri)
+ {
+ return self
+ .components
+ .iter()
+ .any(|comp| *comp == enterprise_component);
+ } else {
+ false
+ }
+ }
}
diff --git a/src/repositories/mod.rs b/src/repositories/mod.rs
index 5c446af..bd0af1d 100644
--- a/src/repositories/mod.rs
+++ b/src/repositories/mod.rs
@@ -107,6 +107,27 @@ fn check_filename<P: AsRef<Path>>(
Ok(Some((file_type, path_string)))
}
+/// Checks if the enterprise repository for the specified Proxmox product is
+/// configured and enabled.
+pub fn enterprise_repository_enabled<A: AsRef<APTRepository>>(repos: &[A], product: &str) -> bool {
+ repos.iter().any(|repo| {
+ let repo = repo.as_ref();
+ repo.enabled && repo.is_enterprise(product)
+ })
+}
+
+/// Checks if the no-subscription repository for the specified Proxmox product
+/// is configured and enabled.
+pub fn no_subscription_repository_enabled<A: AsRef<APTRepository>>(
+ repos: &[A],
+ product: &str,
+) -> bool {
+ repos.iter().any(|repo| {
+ let repo = repo.as_ref();
+ repo.enabled && repo.is_no_subscription(product)
+ })
+}
+
/// Similar to [`repositories_from_files`], but for a single file, and adds the
/// parsed repositories onto the provided vector instead. Another difference is
/// that it doesn't call [`basic_check`](check::basic_check).
diff --git a/tests/repositories.rs b/tests/repositories.rs
index 020e133..deb2f6a 100644
--- a/tests/repositories.rs
+++ b/tests/repositories.rs
@@ -3,7 +3,10 @@ use std::path::PathBuf;
use anyhow::{bail, format_err, Error};
-use proxmox_apt::repositories::{repositories_from_files, write_repositories};
+use proxmox_apt::repositories::{
+ enterprise_repository_enabled, no_subscription_repository_enabled, repositories_from_files,
+ write_repositories,
+};
use proxmox_apt::types::APTRepository;
#[test]
@@ -71,3 +74,27 @@ fn test_parse_write() -> Result<(), Error> {
Ok(())
}
+
+#[test]
+fn test_proxmox_repositories() -> Result<(), Error> {
+ let test_dir = std::env::current_dir()?.join("tests");
+ let read_dir = test_dir.join("sources.list.d");
+
+ let pve_list = read_dir.join("pve.list");
+ let repos = repositories_from_files(&vec![pve_list])?;
+
+ assert_eq!(false, enterprise_repository_enabled(&repos, "pbs"));
+ assert_eq!(false, enterprise_repository_enabled(&repos, "pve"));
+ assert_eq!(false, no_subscription_repository_enabled(&repos, "pmg"));
+ assert_eq!(true, no_subscription_repository_enabled(&repos, "pve"));
+
+ let pbs_list = read_dir.join("pbs-enterprise.list");
+ let repos = repositories_from_files(&vec![pbs_list])?;
+
+ assert_eq!(true, enterprise_repository_enabled(&repos, "pbs"));
+ assert_eq!(false, enterprise_repository_enabled(&repos, "pve"));
+ assert_eq!(false, no_subscription_repository_enabled(&repos, "pmg"));
+ assert_eq!(false, no_subscription_repository_enabled(&repos, "pve"));
+
+ Ok(())
+}
--
2.20.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [pbs-devel] [PATCH v3 proxmox-apt 04/10] add check_repositories function
2021-03-22 11:59 [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI Fabian Ebner
` (2 preceding siblings ...)
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-apt 03/10] add functions to check for Proxmox repositories Fabian Ebner
@ 2021-03-22 11:59 ` Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-backup 05/10] depend on new proxmox-apt crate Fabian Ebner
` (6 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Fabian Ebner @ 2021-03-22 11:59 UTC (permalink / raw)
To: pbs-devel
which, for now, checks the suites.
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
Changes from v2:
* incorporate Wolfgang's feedback:
* improve suite_is_variant by using strip_prefix and matches!(), which
as a bonus allows to gets rid of the one clippy exception I had added
for readability reasons
src/repositories/check.rs | 76 ++++++++++++++++++++++-
src/repositories/mod.rs | 16 ++++-
src/types.rs | 30 +++++++++
tests/repositories.rs | 60 +++++++++++++++++-
tests/sources.list.d.expected/bad.sources | 20 ++++++
tests/sources.list.d/bad.sources | 19 ++++++
6 files changed, 217 insertions(+), 4 deletions(-)
create mode 100644 tests/sources.list.d.expected/bad.sources
create mode 100644 tests/sources.list.d/bad.sources
diff --git a/src/repositories/check.rs b/src/repositories/check.rs
index d0656cd..d63eb3c 100644
--- a/src/repositories/check.rs
+++ b/src/repositories/check.rs
@@ -1,6 +1,21 @@
use anyhow::{bail, Error};
-use crate::types::{APTRepository, APTRepositoryFileType};
+use crate::types::{
+ APTRepository, APTRepositoryFileType, APTRepositoryPackageType, APTRepositoryWarning,
+};
+
+/// Checks if `suite` is some variant of `base_suite`, e.g. `buster-backports`
+/// is a variant of `buster`.
+fn suite_is_variant(suite: &str, base_suite: &str) -> bool {
+ matches!(
+ suite.strip_prefix(base_suite),
+ Some("")
+ | Some("-backports")
+ | Some("-backports-sloppy")
+ | Some("-updates")
+ | Some("/updates")
+ )
+}
impl APTRepository {
/// Makes sure that all basic properties of a repository are present and
@@ -45,6 +60,65 @@ impl APTRepository {
Ok(())
}
+ /// Checks if old or unstable suites are configured and also that the `stable`
+ /// keyword is not used.
+ pub fn check_suites(&self) -> Vec<APTRepositoryWarning> {
+ let old_suites = vec![
+ "lenny",
+ "squeeze",
+ "wheezy",
+ "jessie",
+ "stretch",
+ "oldoldstable",
+ "oldstable",
+ ];
+
+ let new_suites = vec!["unstable", "sid", "experimental"];
+
+ let mut warnings = vec![];
+
+ let mut add_warning = |warning| {
+ warnings.push(APTRepositoryWarning {
+ path: self.path.clone(),
+ number: self.number,
+ warning,
+ });
+ };
+
+ if self
+ .types
+ .iter()
+ .any(|package_type| *package_type == APTRepositoryPackageType::Deb)
+ {
+ for suite in self.suites.iter() {
+ if old_suites
+ .iter()
+ .any(|base_suite| suite_is_variant(suite, base_suite))
+ {
+ add_warning(format!("old suite '{}' configured!", suite));
+ }
+
+ if new_suites
+ .iter()
+ .any(|base_suite| suite_is_variant(suite, base_suite))
+ {
+ add_warning(format!(
+ "suite '{}' should not be used in production!",
+ suite,
+ ));
+ }
+
+ if suite_is_variant(suite, "stable") {
+ add_warning(
+ "use the name of the stable distribution instead of 'stable'!".to_string(),
+ );
+ }
+ }
+ }
+
+ warnings
+ }
+
/// Checks if the repository is the no-subscription repository of the specified
/// Proxmox product.
pub fn is_no_subscription(&self, product: &str) -> bool {
diff --git a/src/repositories/mod.rs b/src/repositories/mod.rs
index bd0af1d..cc7c666 100644
--- a/src/repositories/mod.rs
+++ b/src/repositories/mod.rs
@@ -6,7 +6,9 @@ use std::path::{Path, PathBuf};
use anyhow::{bail, format_err, Error};
-use crate::types::{APTRepository, APTRepositoryFileType, APTRepositoryOption};
+use crate::types::{
+ APTRepository, APTRepositoryFileType, APTRepositoryOption, APTRepositoryWarning,
+};
mod list_parser;
use list_parser::APTListFileParser;
@@ -107,6 +109,18 @@ fn check_filename<P: AsRef<Path>>(
Ok(Some((file_type, path_string)))
}
+/// Produces warnings if there are problems withe the repositories.
+/// Currently only checks the suites.
+pub fn check_repositories<A: AsRef<APTRepository>>(repos: &[A]) -> Vec<APTRepositoryWarning> {
+ let mut warnings = vec![];
+
+ for repo in repos.iter() {
+ warnings.append(&mut repo.as_ref().check_suites());
+ }
+
+ warnings
+}
+
/// Checks if the enterprise repository for the specified Proxmox product is
/// configured and enabled.
pub fn enterprise_repository_enabled<A: AsRef<APTRepository>>(repos: &[A], product: &str) -> bool {
diff --git a/src/types.rs b/src/types.rs
index be69652..8bfafd2 100644
--- a/src/types.rs
+++ b/src/types.rs
@@ -209,3 +209,33 @@ impl AsRef<APTRepository> for APTRepository {
&self
}
}
+
+#[api(
+ properties: {
+ Path: {
+ description: "Path to the defining file.",
+ type: String,
+ },
+ Number: {
+ description: "Line or stanza number.",
+ type: Integer,
+ },
+ Warning: {
+ description: "Warning message.",
+ type: String,
+ },
+ },
+)]
+#[derive(Debug, Clone, Serialize, Deserialize)]
+#[serde(rename_all = "PascalCase")]
+/// Warning for an APT repository.
+pub struct APTRepositoryWarning {
+ /// Path to the defining file.
+ #[serde(skip_serializing_if = "String::is_empty")]
+ pub path: String,
+ /// Line or stanza number.
+ pub number: usize,
+ /// Warning
+ #[serde(skip_serializing_if = "String::is_empty")]
+ pub warning: String,
+}
diff --git a/tests/repositories.rs b/tests/repositories.rs
index deb2f6a..1b548bd 100644
--- a/tests/repositories.rs
+++ b/tests/repositories.rs
@@ -4,8 +4,8 @@ use std::path::PathBuf;
use anyhow::{bail, format_err, Error};
use proxmox_apt::repositories::{
- enterprise_repository_enabled, no_subscription_repository_enabled, repositories_from_files,
- write_repositories,
+ check_repositories, enterprise_repository_enabled, no_subscription_repository_enabled,
+ repositories_from_files, write_repositories,
};
use proxmox_apt::types::APTRepository;
@@ -98,3 +98,59 @@ fn test_proxmox_repositories() -> Result<(), Error> {
Ok(())
}
+
+#[test]
+fn test_check_repositories() -> Result<(), Error> {
+ let test_dir = std::env::current_dir()?.join("tests");
+ let read_dir = test_dir.join("sources.list.d");
+
+ let pve_list = read_dir.join("pve.list");
+ let repos = repositories_from_files(&vec![pve_list])?;
+
+ let warnings = check_repositories(&repos);
+
+ assert_eq!(warnings.is_empty(), true);
+
+ let bad_sources = read_dir.join("bad.sources");
+ let repos = repositories_from_files(&vec![bad_sources])?;
+
+ let mut expected_warnings = HashMap::<String, String>::new();
+ expected_warnings.insert(
+ "bad.sources:1".to_string(),
+ "suite 'sid' should not be used in production!".to_string(),
+ );
+ expected_warnings.insert(
+ "bad.sources:2".to_string(),
+ "old suite 'lenny-backports' configured!".to_string(),
+ );
+ expected_warnings.insert(
+ "bad.sources:3".to_string(),
+ "old suite 'stretch/updates' configured!".to_string(),
+ );
+ expected_warnings.insert(
+ "bad.sources:4".to_string(),
+ "use the name of the stable distribution instead of 'stable'!".to_string(),
+ );
+
+ let warnings = check_repositories(&repos);
+
+ assert_eq!(warnings.len(), expected_warnings.len());
+
+ for warning in warnings.iter() {
+ let path = PathBuf::from(warning.path.clone());
+ let file_name = path
+ .file_name()
+ .unwrap()
+ .to_os_string()
+ .into_string()
+ .unwrap();
+
+ let key = format!("{}:{}", file_name, warning.number);
+ let message = expected_warnings.get(&key);
+
+ assert_eq!(message.is_some(), true);
+ assert_eq!(*message.unwrap(), warning.warning);
+ }
+
+ Ok(())
+}
diff --git a/tests/sources.list.d.expected/bad.sources b/tests/sources.list.d.expected/bad.sources
new file mode 100644
index 0000000..7601263
--- /dev/null
+++ b/tests/sources.list.d.expected/bad.sources
@@ -0,0 +1,20 @@
+Types: deb
+URIs: http://ftp.at.debian.org/debian
+Suites: sid
+Components: main contrib
+
+Types: deb
+URIs: http://ftp.at.debian.org/debian
+Suites: lenny-backports
+Components: contrib
+
+Types: deb
+URIs: http://security.debian.org
+Suites: stretch/updates
+Components: main contrib
+
+Types: deb
+URIs: http://ftp.at.debian.org/debian
+Suites: stable
+Components: main
+
diff --git a/tests/sources.list.d/bad.sources b/tests/sources.list.d/bad.sources
new file mode 100644
index 0000000..963f527
--- /dev/null
+++ b/tests/sources.list.d/bad.sources
@@ -0,0 +1,19 @@
+Types: deb
+URIs: http://ftp.at.debian.org/debian
+Suites: sid
+Components: main contrib
+
+Types: deb
+URIs: http://ftp.at.debian.org/debian
+Suites: lenny-backports
+Components: contrib
+
+Types: deb
+URIs: http://security.debian.org
+Suites: stretch/updates
+Components: main contrib
+
+Suites: stable
+URIs: http://ftp.at.debian.org/debian
+Components: main
+Types: deb
--
2.20.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [pbs-devel] [PATCH v3 proxmox-backup 05/10] depend on new proxmox-apt crate
2021-03-22 11:59 [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI Fabian Ebner
` (3 preceding siblings ...)
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-apt 04/10] add check_repositories function Fabian Ebner
@ 2021-03-22 11:59 ` Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-backup 06/10] api: apt: add repositories call Fabian Ebner
` (5 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Fabian Ebner @ 2021-03-22 11:59 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
No changes from v2.
Cargo.toml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Cargo.toml b/Cargo.toml
index 9483831c..76e1cde6 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -51,6 +51,7 @@ pathpatterns = "0.1.2"
proxmox = { version = "0.11.0", features = [ "sortable-macro", "api-macro", "websocket" ] }
#proxmox = { git = "git://git.proxmox.com/git/proxmox", version = "0.1.2", features = [ "sortable-macro", "api-macro" ] }
#proxmox = { path = "../proxmox/proxmox", features = [ "sortable-macro", "api-macro", "websocket" ] }
+proxmox-apt = "0.1.0"
proxmox-fuse = "0.1.1"
pxar = { version = "0.10.0", features = [ "tokio-io" ] }
#pxar = { path = "../pxar", features = [ "tokio-io" ] }
--
2.20.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [pbs-devel] [PATCH v3 proxmox-backup 06/10] api: apt: add repositories call
2021-03-22 11:59 [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI Fabian Ebner
` (4 preceding siblings ...)
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-backup 05/10] depend on new proxmox-apt crate Fabian Ebner
@ 2021-03-22 11:59 ` Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-backup 07/10] ui: add repositories Fabian Ebner
` (4 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Fabian Ebner @ 2021-03-22 11:59 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
No changes from v2.
src/api2/node/apt.rs | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/src/api2/node/apt.rs b/src/api2/node/apt.rs
index e77b89fa..4dad2e73 100644
--- a/src/api2/node/apt.rs
+++ b/src/api2/node/apt.rs
@@ -6,6 +6,8 @@ use proxmox::list_subdirs_api_method;
use proxmox::api::{api, RpcEnvironment, RpcEnvironmentType, Permission};
use proxmox::api::router::{Router, SubdirMap};
+use proxmox_apt::types::APTRepository;
+
use crate::server::WorkerTask;
use crate::tools::{apt, http, subscription};
@@ -350,8 +352,33 @@ pub fn get_versions() -> Result<Vec<APTUpdateInfo>, Error> {
Ok(packages)
}
+#[api(
+ input: {
+ properties: {
+ node: {
+ schema: NODE_SCHEMA,
+ },
+ },
+ },
+ returns: {
+ description: "List of configured APT repositories",
+ type: Array,
+ items: {
+ type: APTRepository,
+ },
+ },
+ access: {
+ permission: &Permission::Privilege(&[], PRIV_SYS_AUDIT, false),
+ },
+)]
+/// Get APT repository information
+pub fn get_repositories() -> Result<Vec<APTRepository>, Error> {
+ proxmox_apt::repositories::repositories()
+}
+
const SUBDIRS: SubdirMap = &[
("changelog", &Router::new().get(&API_METHOD_APT_GET_CHANGELOG)),
+ ("repositories", &Router::new().get(&API_METHOD_GET_REPOSITORIES)),
("update", &Router::new()
.get(&API_METHOD_APT_UPDATE_AVAILABLE)
.post(&API_METHOD_APT_UPDATE_DATABASE)
--
2.20.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [pbs-devel] [PATCH v3 proxmox-backup 07/10] ui: add repositories
2021-03-22 11:59 [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI Fabian Ebner
` (5 preceding siblings ...)
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-backup 06/10] api: apt: add repositories call Fabian Ebner
@ 2021-03-22 11:59 ` Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-backup 08/10] add check_repositories_call Fabian Ebner
` (3 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Fabian Ebner @ 2021-03-22 11:59 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
No changes from v2.
www/ServerAdministration.js | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/www/ServerAdministration.js b/www/ServerAdministration.js
index 0d803ac4..027a13b9 100644
--- a/www/ServerAdministration.js
+++ b/www/ServerAdministration.js
@@ -53,6 +53,13 @@ Ext.define('PBS.ServerAdministration', {
itemId: 'updates',
nodename: 'localhost',
},
+ {
+ xtype: 'proxmoxNodeAPTRepositories',
+ title: gettext('Repositories'),
+ iconCls: 'fa fa-files-o',
+ itemId: 'aptrepositories',
+ nodename: 'localhost',
+ },
{
xtype: 'proxmoxJournalView',
itemId: 'logs',
--
2.20.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [pbs-devel] [PATCH v3 proxmox-backup 08/10] add check_repositories_call
2021-03-22 11:59 [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI Fabian Ebner
` (6 preceding siblings ...)
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-backup 07/10] ui: add repositories Fabian Ebner
@ 2021-03-22 11:59 ` Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 widget-toolkit 09/10] add UI for APT repositories Fabian Ebner
` (2 subsequent siblings)
10 siblings, 0 replies; 16+ messages in thread
From: Fabian Ebner @ 2021-03-22 11:59 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
No changes from v2.
src/api2/node/apt.rs | 53 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 52 insertions(+), 1 deletion(-)
diff --git a/src/api2/node/apt.rs b/src/api2/node/apt.rs
index 4dad2e73..22e43340 100644
--- a/src/api2/node/apt.rs
+++ b/src/api2/node/apt.rs
@@ -6,7 +6,7 @@ use proxmox::list_subdirs_api_method;
use proxmox::api::{api, RpcEnvironment, RpcEnvironmentType, Permission};
use proxmox::api::router::{Router, SubdirMap};
-use proxmox_apt::types::APTRepository;
+use proxmox_apt::types::{APTRepository, APTRepositoryWarning};
use crate::server::WorkerTask;
use crate::tools::{apt, http, subscription};
@@ -352,6 +352,56 @@ pub fn get_versions() -> Result<Vec<APTUpdateInfo>, Error> {
Ok(packages)
}
+#[api(
+ input: {
+ properties: {
+ node: {
+ schema: NODE_SCHEMA,
+ },
+ },
+ },
+ returns: {
+ description: "Warnings for APT repositories and status of Proxmox repositories.",
+ properties: {
+ warnings: {
+ description: "Warnings for APT repositories.",
+ type: Array,
+ items: {
+ type: APTRepositoryWarning,
+ },
+ },
+ enterprise: {
+ description: "Whether the enterprise repository is enabled.",
+ type: Boolean,
+ },
+ nosubscription: {
+ description: "Whether the no-subscription repository is enabled.",
+ type: Boolean,
+ },
+ },
+ },
+ access: {
+ permission: &Permission::Privilege(&[], PRIV_SYS_AUDIT, false),
+ },
+)]
+/// Check APT repository configuration.
+pub fn check_repositories() -> Result<Value, Error> {
+ let repos = proxmox_apt::repositories::repositories()?;
+
+ let warnings = proxmox_apt::repositories::check_repositories(&repos);
+
+ let enterprise_enabled =
+ proxmox_apt::repositories::enterprise_repository_enabled(&repos, "pbs");
+ let no_subscription_enabled =
+ proxmox_apt::repositories::no_subscription_repository_enabled(&repos, "pbs");
+
+ Ok(json!({
+ "warnings": warnings,
+ "enterprise": enterprise_enabled,
+ "nosubscription": no_subscription_enabled
+ }))
+}
+
#[api(
input: {
properties: {
@@ -378,6 +428,7 @@ pub fn get_repositories() -> Result<Vec<APTRepository>, Error> {
const SUBDIRS: SubdirMap = &[
("changelog", &Router::new().get(&API_METHOD_APT_GET_CHANGELOG)),
+ ("checkrepositories", &Router::new().get(&API_METHOD_CHECK_REPOSITORIES)),
("repositories", &Router::new().get(&API_METHOD_GET_REPOSITORIES)),
("update", &Router::new()
.get(&API_METHOD_APT_UPDATE_AVAILABLE)
--
2.20.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [pbs-devel] [PATCH v3 widget-toolkit 09/10] add UI for APT repositories
2021-03-22 11:59 [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI Fabian Ebner
` (7 preceding siblings ...)
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-backup 08/10] add check_repositories_call Fabian Ebner
@ 2021-03-22 11:59 ` Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 widget-toolkit 10/10] APT repositories: add warnings Fabian Ebner
2021-03-23 10:29 ` [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI Fabian Grünbichler
10 siblings, 0 replies; 16+ messages in thread
From: Fabian Ebner @ 2021-03-22 11:59 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
No changes from v2.
src/Makefile | 1 +
src/node/APTRepositories.js | 145 ++++++++++++++++++++++++++++++++++++
2 files changed, 146 insertions(+)
create mode 100644 src/node/APTRepositories.js
diff --git a/src/Makefile b/src/Makefile
index 44c11ea..dd3f1f9 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -65,6 +65,7 @@ JSSRC= \
window/ACMEPluginEdit.js \
window/ACMEDomains.js \
node/APT.js \
+ node/APTRepositories.js \
node/NetworkEdit.js \
node/NetworkView.js \
node/DNSEdit.js \
diff --git a/src/node/APTRepositories.js b/src/node/APTRepositories.js
new file mode 100644
index 0000000..0b21a8a
--- /dev/null
+++ b/src/node/APTRepositories.js
@@ -0,0 +1,145 @@
+Ext.define('apt-repolist', {
+ extend: 'Ext.data.Model',
+ fields: [
+ 'Path',
+ 'Number',
+ 'FileType',
+ 'Enabled',
+ 'Comment',
+ 'Types',
+ 'URIs',
+ 'Suites',
+ 'Components',
+ 'Options',
+ ],
+});
+
+Ext.define('Proxmox.node.APTRepositories', {
+ extend: 'Ext.grid.GridPanel',
+
+ xtype: 'proxmoxNodeAPTRepositories',
+
+ sortableColumns: false,
+
+ columns: [
+ {
+ header: gettext('Enabled'),
+ dataIndex: 'Enabled',
+ renderer: Proxmox.Utils.format_enabled_toggle,
+ width: 90,
+ },
+ {
+ header: gettext('Types'),
+ dataIndex: 'Types',
+ renderer: function(options, cell, record) {
+ return record.data.Types.join(' ');
+ },
+ width: 100,
+ },
+ {
+ header: gettext('URIs'),
+ dataIndex: 'URIs',
+ renderer: function(options, cell, record) {
+ return record.data.URIs.join(' ');
+ },
+ width: 350,
+ },
+ {
+ header: gettext('Suites'),
+ dataIndex: 'Suites',
+ renderer: function(options, cell, record) {
+ return record.data.Suites.join(' ');
+ },
+ width: 130,
+ },
+ {
+ header: gettext('Components'),
+ dataIndex: 'Components',
+ renderer: function(options, cell, record) {
+ return record.data.Components.join(' ');
+ },
+ width: 170,
+ },
+ {
+ header: gettext('Options'),
+ dataIndex: 'Options',
+ renderer: function(options, cell, record) {
+ if (!options) {
+ return '';
+ }
+
+ let filetype = record.data.FileType;
+ let text = '';
+
+ options.forEach(function(option) {
+ let key = option.Key;
+ if (filetype === 'list') {
+ let values = option.Values.join(',');
+ text += `${key}=${values} `;
+ } else if (filetype === 'sources') {
+ let values = option.Values.join(' ');
+ text += `${key}: ${values}\n`;
+ } else {
+ throw "unkown file type";
+ }
+ });
+ return text;
+ },
+ flex: 1,
+ },
+ {
+ header: gettext('Comment'),
+ dataIndex: 'Comment',
+ flex: 1,
+ },
+ ],
+
+ initComponent: function() {
+ let me = this;
+
+ if (!me.nodename) {
+ throw "no node name specified";
+ }
+
+ let store = Ext.create('Ext.data.Store', {
+ model: 'apt-repolist',
+ groupField: 'Path',
+ proxy: {
+ type: 'proxmox',
+ url: `/api2/json/nodes/${me.nodename}/apt/repositories`,
+ },
+ sorters: [
+ {
+ property: 'Number',
+ direction: 'ASC',
+ },
+ ],
+ });
+
+ let groupingFeature = Ext.create('Ext.grid.feature.Grouping', {
+ groupHeaderTpl: '{[ "File: " + values.name ]} ({rows.length} ' +
+ 'repositor{[values.rows.length > 1 ? "ies" : "y"]})',
+ enableGroupingMenu: false,
+ });
+
+ let reload = function() {
+ store.load();
+ };
+
+ let sm = Ext.create('Ext.selection.RowModel', {});
+
+ Ext.apply(me, {
+ store: store,
+ selModel: sm,
+ features: [groupingFeature],
+ listeners: {
+ activate: reload,
+ },
+ });
+
+ Proxmox.Utils.monStoreErrors(me, store, true);
+ reload();
+
+ me.callParent();
+ },
+});
--
2.20.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [pbs-devel] [PATCH v3 widget-toolkit 10/10] APT repositories: add warnings
2021-03-22 11:59 [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI Fabian Ebner
` (8 preceding siblings ...)
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 widget-toolkit 09/10] add UI for APT repositories Fabian Ebner
@ 2021-03-22 11:59 ` Fabian Ebner
2021-03-23 10:29 ` [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI Fabian Grünbichler
10 siblings, 0 replies; 16+ messages in thread
From: Fabian Ebner @ 2021-03-22 11:59 UTC (permalink / raw)
To: pbs-devel
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
Changes from v2:
* rename globalWarning to mainWarning
* use gettext for main warnings
* use a stand-alone label component for the main warning instead of misusing
the title bar. This made it necessary to adapt the view model and the
reload functionionality, including handling of the rowBodyFeature.
* reduce capslock, since the warning is clearly visible even without.
src/node/APTRepositories.js | 172 +++++++++++++++++++++++++++++++++---
1 file changed, 161 insertions(+), 11 deletions(-)
diff --git a/src/node/APTRepositories.js b/src/node/APTRepositories.js
index 0b21a8a..d9b70be 100644
--- a/src/node/APTRepositories.js
+++ b/src/node/APTRepositories.js
@@ -14,13 +14,15 @@ Ext.define('apt-repolist', {
],
});
-Ext.define('Proxmox.node.APTRepositories', {
+Ext.define('Proxmox.node.APTRepositoriesGrid', {
extend: 'Ext.grid.GridPanel',
- xtype: 'proxmoxNodeAPTRepositories',
+ xtype: 'proxmoxNodeAPTRepositoriesGrid',
sortableColumns: false,
+ rowBodyFeature: undefined,
+
columns: [
{
header: gettext('Enabled'),
@@ -94,6 +96,78 @@ Ext.define('Proxmox.node.APTRepositories', {
},
],
+ check_repositories: function() {
+ let me = this;
+ let vm = me.up('proxmoxNodeAPTRepositories').getViewModel();
+
+ Proxmox.Utils.API2Request({
+ url: `/nodes/${me.nodename}/apt/checkrepositories`,
+ method: 'GET',
+ failure: function(response, opts) {
+ Ext.Msg.alert(gettext('Error'), response.htmlStatus);
+ },
+ success: function(response, opts) {
+ const data = response.result.data;
+
+ vm.set('enterpriseRepo', data.enterprise);
+ vm.set('noSubscriptionRepo', data.nosubscription);
+
+ let warnings = {};
+ for (const warning of data.warnings) {
+ const key = `${warning.Path}:${warning.Number}`;
+ if (warnings[key]) {
+ warnings[key] += "\n";
+ warnings[key] += "Warning: " + warning.Warning;
+ } else {
+ warnings[key] = "Warning: " + warning.Warning;
+ }
+ }
+
+ me.rowBodyFeature.getAdditionalData = function(innerData, rowIndex, record, orig) {
+ let headerCt = this.view.headerCt;
+ let colspan = headerCt.getColumnCount();
+
+ const key = `${innerData.Path}:${innerData.Number}`;
+ const warning_text = warnings[key];
+
+ return {
+ rowBody: '<div style="color: red; white-space: pre-line">' +
+ Ext.String.htmlEncode(warning_text) +
+ '</div>',
+ rowBodyCls: warning_text ? '' : Ext.baseCSSPrefix + 'grid-row-body-hidden',
+ rowBodyColspan: colspan,
+ };
+ };
+ },
+ });
+ },
+
+ check_subscription: function() {
+ let me = this;
+ let vm = me.up('proxmoxNodeAPTRepositories').getViewModel();
+
+ Proxmox.Utils.API2Request({
+ url: `/nodes/${me.nodename}/subscription`,
+ method: 'GET',
+ failure: function(response, opts) {
+ Ext.Msg.alert(gettext('Error'), response.htmlStatus);
+ },
+ success: function(response, opts) {
+ const res = response.result;
+ const subscription = !(res === null || res === undefined ||
+ !res || res.data.status.toLowerCase() !== 'active');
+ vm.set('subscriptionActive', subscription);
+ },
+ });
+ },
+
+ reload: function() {
+ let me = this;
+ me.store.load();
+ me.check_repositories();
+ me.check_subscription();
+ },
+
initComponent: function() {
let me = this;
@@ -116,29 +190,105 @@ Ext.define('Proxmox.node.APTRepositories', {
],
});
+ let rowBodyFeature = Ext.create('Ext.grid.feature.RowBody', {});
+ me.rowBodyFeature = rowBodyFeature;
+
let groupingFeature = Ext.create('Ext.grid.feature.Grouping', {
groupHeaderTpl: '{[ "File: " + values.name ]} ({rows.length} ' +
'repositor{[values.rows.length > 1 ? "ies" : "y"]})',
enableGroupingMenu: false,
});
- let reload = function() {
- store.load();
- };
-
let sm = Ext.create('Ext.selection.RowModel', {});
Ext.apply(me, {
store: store,
selModel: sm,
- features: [groupingFeature],
- listeners: {
- activate: reload,
- },
+ features: [groupingFeature, me.rowBodyFeature],
});
Proxmox.Utils.monStoreErrors(me, store, true);
- reload();
+ me.reload();
+
+ me.callParent();
+ },
+});
+
+Ext.define('Proxmox.node.APTRepositories', {
+ extend: 'Ext.panel.Panel',
+
+ xtype: 'proxmoxNodeAPTRepositories',
+ mixins: ['Proxmox.Mixin.CBind'],
+
+ viewModel: {
+ data: {
+ subscriptionActive: '',
+ noSubscriptionRepo: '',
+ enterpriseRepo: '',
+ },
+ formulas: {
+ mainWarning: function(get) {
+ if (get('subscriptionActive') === '' ||
+ get('noSubscriptionRepo') === '' ||
+ get('enterpriseRepo') === '') {
+ return '';
+ }
+
+ const warn = gettext('Warning') + ': ';
+
+ if (!get('subscriptionActive') && get('enterpriseRepo')) {
+ return warn + gettext('The enterprise repository is ' +
+ 'configured, but there is no active subscription!');
+ }
+
+ if (get('noSubscriptionRepo')) {
+ return warn + gettext('The no-subscription repository is ' +
+ 'not recommended for production use!');
+ }
+
+ if (!get('enterpriseRepo') && !get('noSubscriptionRepo')) {
+ return warn + gettext('Neither the enterprise repository ' +
+ 'nor the no-subscription repository is configured!');
+ }
+
+ return '';
+ },
+ },
+ },
+
+ items: [
+ {
+ xtype: 'label',
+ name: 'mainWarning',
+ cls: 'fa fa-exclamation-triangle',
+ style: 'color:red;',
+ bind: {
+ text: '{mainWarning}',
+ hidden: '{!mainWarning}',
+ },
+ },
+ {
+ xtype: 'proxmoxNodeAPTRepositoriesGrid',
+ name: 'repoGrid',
+ cbind: {
+ nodename: '{nodename}',
+ },
+ },
+ ],
+
+ listeners: {
+ activate: function() {
+ let me = this;
+ me.down('proxmoxNodeAPTRepositoriesGrid').reload();
+ },
+ },
+
+ initComponent: function() {
+ let me = this;
+
+ if (!me.nodename) {
+ throw "no node name specified";
+ }
me.callParent();
},
--
2.20.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI
2021-03-22 11:59 [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI Fabian Ebner
` (9 preceding siblings ...)
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 widget-toolkit 10/10] APT repositories: add warnings Fabian Ebner
@ 2021-03-23 10:29 ` Fabian Grünbichler
2021-03-24 9:40 ` Fabian Ebner
10 siblings, 1 reply; 16+ messages in thread
From: Fabian Grünbichler @ 2021-03-23 10:29 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On March 22, 2021 12:59 pm, Fabian Ebner wrote:
> List the configured repositories and have some basic checks for them.
>
> The plan is to use perlmod to make the Rust implementation available for PVE+PMG
> as well.
>
> There's still the question if introducing a digest is worth it. At the moment,
> the warnings returned by the checkrepositories call might not match up with the
> repositories returned previously, but that's a rather minor issue as editing
> repositories is a rare operation.
> Should a digest be added now to be future-proof? Should it live in the
> proxmox-apt crate and be file-level, or would it be enough to hash the result in
> the PBS API call and use that? The latter seems like the more pragmatic approach
> and avoids cluttering the APT backend.
gave this a spin, looks mostly good to me, some things I noticed:
"man deb822" states:
Field names are not case-sensitive, but it is usual to capitalize the
field names using mixed case as shown below. Field values are
case-sensitive unless the description of the field says otherwise.
the parser here is case-sensitive and chokes on .sources files which APT
happily parses & uses.
the "options" part is missing support for the following "feature" ("man
sources.list"):
Multivalue options also have -= and += as separators, which instead of
replacing the default with the given value(s) modify the default
value(s) to remove or include the given values.
I haven't tested that one though ;)
if a file cannot be parsed or is malformed (e.g. because I put "Uris"
instead of "URIs" in a .sources file ;)), the whole API call fails with
400. it might be more user-friendly to mark indiviual .list/.sources
files as containing invalid entries which are not displayed, and still
return the rest? might make the result less actionable since we don't
have the complete picture, but it still might be better than a single
error message for one of X files..
the GUI already makes the API request when opening the 'Administration'
part, which then displays an error modal if any of the files is wrong -
even though we are not on the repositories tab at that moment. this is
confusing. also, opening the repositories tab triggers a (re?)load
anyhow..
we have a warning for Debian unstable, but none for Debian testing which
should also never be enabled on a production machine.
we might want to match known-official repo URIs (ftp.*.debian.org,
deb.debian.org, download.proxmox.com, enterprise.proxmox.com) to mark
"potentially dangerous external repositories" (or to give the official
ones a "official mirror" badge or something like that)
a digest based on raw file contents or some other stable means seems
sensible to me. hashing just the results of the parser is usually wrong
(digest mismatch with different parser versions, parser bugs causing
indeterminism with rare setups, ...).
>
>
> Changes from v2:
> * incorporate Wolfgang's feedback
> * improve main warning's UI
>
> Still missing:
> * Upgrade suite/distribuiton button to be used before major release
> upgrades (but it's really simply to add that now).
> * perlmod magic and integration in PVE and PMG.
>
> Changes v1 -> v2:
> * Perl -> Rust
> * PVE -> PBS
> * Don't rely on regexes for parsing.
> * Add writer and tests.
> * UI: pin warnings to the repository they're for.
> * Keep order of options consistent with configuration.
> * Smaller things noted on the individual patches.
>
>
> proxmox-apt:
>
> Fabian Ebner (4):
> initial commit
> add files for Debian packaging
> add functions to check for Proxmox repositories
> add check_repositories function
>
>
> proxmox-backup:
>
> Fabian Ebner (4):
> depend on new proxmox-apt crate
> api: apt: add repositories call
> ui: add repositories
> add check_repositories_call
>
> Cargo.toml | 1 +
> src/api2/node/apt.rs | 78 +++++++++++++++++++++++++++++++++++++
> www/ServerAdministration.js | 7 ++++
> 3 files changed, 86 insertions(+)
>
>
> widget-toolkit:
>
> Fabian Ebner (2):
> add UI for APT repositories
> APT repositories: add warnings
>
> src/Makefile | 1 +
> src/node/APTRepositories.js | 295 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 296 insertions(+)
> create mode 100644 src/node/APTRepositories.js
>
> --
> 2.20.1
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI
2021-03-23 10:29 ` [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI Fabian Grünbichler
@ 2021-03-24 9:40 ` Fabian Ebner
2021-03-24 10:06 ` Fabian Grünbichler
0 siblings, 1 reply; 16+ messages in thread
From: Fabian Ebner @ 2021-03-24 9:40 UTC (permalink / raw)
To: pbs-devel, Fabian Grünbichler
Am 23.03.21 um 11:29 schrieb Fabian Grünbichler:
> On March 22, 2021 12:59 pm, Fabian Ebner wrote:
>> List the configured repositories and have some basic checks for them.
>>
>> The plan is to use perlmod to make the Rust implementation available for PVE+PMG
>> as well.
>>
>> There's still the question if introducing a digest is worth it. At the moment,
>> the warnings returned by the checkrepositories call might not match up with the
>> repositories returned previously, but that's a rather minor issue as editing
>> repositories is a rare operation.
>> Should a digest be added now to be future-proof? Should it live in the
>> proxmox-apt crate and be file-level, or would it be enough to hash the result in
>> the PBS API call and use that? The latter seems like the more pragmatic approach
>> and avoids cluttering the APT backend.
>
> gave this a spin, looks mostly good to me, some things I noticed:
>
> "man deb822" states:
>
> Field names are not case-sensitive, but it is usual to capitalize the
> field names using mixed case as shown below. Field values are
> case-sensitive unless the description of the field says otherwise.
>
> the parser here is case-sensitive and chokes on .sources files which APT
> happily parses & uses.
>
Thanks for catching that, I'll fix this in the next version.
> the "options" part is missing support for the following "feature" ("man
> sources.list"):
>
> Multivalue options also have -= and += as separators, which instead of
> replacing the default with the given value(s) modify the default
> value(s) to remove or include the given values.
>
> I haven't tested that one though ;)
>
The parser just uses "option+" and "option-" as the option keys then. It
/could/ be interpreted on parsing, but if the user chose to use this
notation, there's probably a reason and it should be written out the
same way it came in again. I'll add a test though.
> if a file cannot be parsed or is malformed (e.g. because I put "Uris"
> instead of "URIs" in a .sources file ;)), the whole API call fails with
> 400. it might be more user-friendly to mark indiviual .list/.sources
> files as containing invalid entries which are not displayed, and still
> return the rest? might make the result less actionable since we don't
> have the complete picture, but it still might be better than a single
> error message for one of X files..
>
I did consider something like this for a bit, but not sure how to
cleanly organize the API then. The call should still error out in my
opinion and syntactic errors should be rather rare anyways (apt also
just complains when it cannot parse). We could continue parsing and
collect all the errors at once at least, but not sure if that's worth it?
> the GUI already makes the API request when opening the 'Administration'
> part, which then displays an error modal if any of the files is wrong -
> even though we are not on the repositories tab at that moment. this is
> confusing. also, opening the repositories tab triggers a (re?)load
> anyhow..
>
Surely not optimal. Hopefully easily fixed by removing the 'reload' call
in 'initComponent'. Too used to having active on init...
> we have a warning for Debian unstable, but none for Debian testing which
> should also never be enabled on a production machine.
>
If there is an "upgrade suites" button/API call, then there would be
warnings after using that. But since enabling that button/API call needs
to happen anyways before each major release, I guess removing the e.g.
'bullseye' warnings then is just one more place to touch.
Or maybe add a 'before_major_release' parameter to check_repositories
and also to the UI? Then only the product specific code needs to be
touched before each major release. Of course there still needs to be a
new version of the library for after each major release.
> we might want to match known-official repo URIs (ftp.*.debian.org,
> deb.debian.org, download.proxmox.com, enterprise.proxmox.com) to mark
> "potentially dangerous external repositories" (or to give the official
> ones a "official mirror" badge or something like that)
>
Might be done as part of the check_repositories call if we go for
warnings. If we go for badges, we'd need something new or change the
interface for check_repositories. I feel like badges would be preferable
though...
> a digest based on raw file contents or some other stable means seems
> sensible to me. hashing just the results of the parser is usually wrong
> (digest mismatch with different parser versions, parser bugs causing
> indeterminism with rare setups, ...).
>
I'll try and add a digest that's returned when parsing from the raw
contents and then the API calls can re-compare against that before doing
any further calls to the library.
>>
>>
>> Changes from v2:
>> * incorporate Wolfgang's feedback
>> * improve main warning's UI
>>
>> Still missing:
>> * Upgrade suite/distribuiton button to be used before major release
>> upgrades (but it's really simply to add that now).
>> * perlmod magic and integration in PVE and PMG.
>>
>> Changes v1 -> v2:
>> * Perl -> Rust
>> * PVE -> PBS
>> * Don't rely on regexes for parsing.
>> * Add writer and tests.
>> * UI: pin warnings to the repository they're for.
>> * Keep order of options consistent with configuration.
>> * Smaller things noted on the individual patches.
>>
>>
>> proxmox-apt:
>>
>> Fabian Ebner (4):
>> initial commit
>> add files for Debian packaging
>> add functions to check for Proxmox repositories
>> add check_repositories function
>>
>>
>> proxmox-backup:
>>
>> Fabian Ebner (4):
>> depend on new proxmox-apt crate
>> api: apt: add repositories call
>> ui: add repositories
>> add check_repositories_call
>>
>> Cargo.toml | 1 +
>> src/api2/node/apt.rs | 78 +++++++++++++++++++++++++++++++++++++
>> www/ServerAdministration.js | 7 ++++
>> 3 files changed, 86 insertions(+)
>>
>>
>> widget-toolkit:
>>
>> Fabian Ebner (2):
>> add UI for APT repositories
>> APT repositories: add warnings
>>
>> src/Makefile | 1 +
>> src/node/APTRepositories.js | 295 ++++++++++++++++++++++++++++++++++++
>> 2 files changed, 296 insertions(+)
>> create mode 100644 src/node/APTRepositories.js
>>
>> --
>> 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
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI
2021-03-24 9:40 ` Fabian Ebner
@ 2021-03-24 10:06 ` Fabian Grünbichler
2021-03-24 12:08 ` Fabian Ebner
0 siblings, 1 reply; 16+ messages in thread
From: Fabian Grünbichler @ 2021-03-24 10:06 UTC (permalink / raw)
To: Fabian Ebner, pbs-devel
snipped ;)
On March 24, 2021 10:40 am, Fabian Ebner wrote:
> Am 23.03.21 um 11:29 schrieb Fabian Grünbichler:
>> the "options" part is missing support for the following "feature" ("man
>> sources.list"):
>>
>> Multivalue options also have -= and += as separators, which instead of
>> replacing the default with the given value(s) modify the default
>> value(s) to remove or include the given values.
>>
>> I haven't tested that one though ;)
>>
>
> The parser just uses "option+" and "option-" as the option keys then. It
> /could/ be interpreted on parsing, but if the user chose to use this
> notation, there's probably a reason and it should be written out the
> same way it came in again. I'll add a test though.
yeah, makes sense (in a way).
>
>> if a file cannot be parsed or is malformed (e.g. because I put "Uris"
>> instead of "URIs" in a .sources file ;)), the whole API call fails with
>> 400. it might be more user-friendly to mark indiviual .list/.sources
>> files as containing invalid entries which are not displayed, and still
>> return the rest? might make the result less actionable since we don't
>> have the complete picture, but it still might be better than a single
>> error message for one of X files..
>>
>
> I did consider something like this for a bit, but not sure how to
> cleanly organize the API then. The call should still error out in my
> opinion and syntactic errors should be rather rare anyways (apt also
> just complains when it cannot parse). We could continue parsing and
> collect all the errors at once at least, but not sure if that's worth it?
I was thinking more of further inconsistencies between APT and our
parser like the case-sensitivity issue. like you said, APT will just
ignore a wrong/broken entry or file, and not error out altogether. so
maybe it would make sense to mimic that behaviour -> add a
warnings/error field, and let the caller decide whether it just wants to
display that or the (partial) result + the additional information. e.g.,
we could disable editing via the GUI for invalid files, but still allow
it for other files.
IMHO for this the request as a whole does not have to error out on the
API/HTTP level if the parser encounters something it does not
understand.
>> we have a warning for Debian unstable, but none for Debian testing which
>> should also never be enabled on a production machine.
>>
>
> If there is an "upgrade suites" button/API call, then there would be
> warnings after using that. But since enabling that button/API call needs
> to happen anyways before each major release, I guess removing the e.g.
> 'bullseye' warnings then is just one more place to touch.
no, because 'bullseye' is not 'testing' ;) or at least, they should not
be treated the same. having 'bullseye' as a suite is fine when upgrading
from 'buster' to 'bullseye'. having 'testing' there is never good on a
stable/production system.
> Or maybe add a 'before_major_release' parameter to check_repositories
> and also to the UI? Then only the product specific code needs to be
> touched before each major release. Of course there still needs to be a
> new version of the library for after each major release.
the check for 'bullseye' could contain text that indicates that the
warning is benign if you are preparing the major release upgrade? or the
'upgrade suites' button sets a flag somewhere (comment? ;)), and that
then automatically skips that check for the given suite?
>> we might want to match known-official repo URIs (ftp.*.debian.org,
>> deb.debian.org, download.proxmox.com, enterprise.proxmox.com) to mark
>> "potentially dangerous external repositories" (or to give the official
>> ones a "official mirror" badge or something like that)
>>
>
> Might be done as part of the check_repositories call if we go for
> warnings. If we go for badges, we'd need something new or change the
> interface for check_repositories. I feel like badges would be preferable
> though...
badges are nicer since they just highlight "this is good", which is
easier to determine than "this is bad" (which is actually just "this
might be bad"). having some custom repo that is okay is not that
uncommon (internal software, monitoring, ansible integration packages,
..), so being too noisy might be a bad idea.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI
2021-03-24 10:06 ` Fabian Grünbichler
@ 2021-03-24 12:08 ` Fabian Ebner
2021-03-24 12:39 ` Fabian Grünbichler
0 siblings, 1 reply; 16+ messages in thread
From: Fabian Ebner @ 2021-03-24 12:08 UTC (permalink / raw)
To: Fabian Grünbichler, pbs-devel
Am 24.03.21 um 11:06 schrieb Fabian Grünbichler:
> snipped ;)
>
> On March 24, 2021 10:40 am, Fabian Ebner wrote:
>> Am 23.03.21 um 11:29 schrieb Fabian Grünbichler:
>>> the "options" part is missing support for the following "feature" ("man
>>> sources.list"):
>>>
>>> Multivalue options also have -= and += as separators, which instead of
>>> replacing the default with the given value(s) modify the default
>>> value(s) to remove or include the given values.
>>>
>>> I haven't tested that one though ;)
>>>
>>
>> The parser just uses "option+" and "option-" as the option keys then. It
>> /could/ be interpreted on parsing, but if the user chose to use this
>> notation, there's probably a reason and it should be written out the
>> same way it came in again. I'll add a test though.
>
> yeah, makes sense (in a way).
>
>>
>>> if a file cannot be parsed or is malformed (e.g. because I put "Uris"
>>> instead of "URIs" in a .sources file ;)), the whole API call fails with
>>> 400. it might be more user-friendly to mark indiviual .list/.sources
>>> files as containing invalid entries which are not displayed, and still
>>> return the rest? might make the result less actionable since we don't
>>> have the complete picture, but it still might be better than a single
>>> error message for one of X files..
>>>
>>
>> I did consider something like this for a bit, but not sure how to
>> cleanly organize the API then. The call should still error out in my
>> opinion and syntactic errors should be rather rare anyways (apt also
>> just complains when it cannot parse). We could continue parsing and
>> collect all the errors at once at least, but not sure if that's worth it?
>
> I was thinking more of further inconsistencies between APT and our
> parser like the case-sensitivity issue. like you said, APT will just
> ignore a wrong/broken entry or file, and not error out altogether. so
> maybe it would make sense to mimic that behaviour -> add a
> warnings/error field, and let the caller decide whether it just wants to
> display that or the (partial) result + the additional information. e.g.,
> we could disable editing via the GUI for invalid files, but still allow
> it for other files.
>
> IMHO for this the request as a whole does not have to error out on the
> API/HTTP level if the parser encounters something it does not
> understand.
>
apt seems to error out on the first problem for each file, and it will
error out before doing anything else when the parsing failed.
Is it really worth keeping the partial result in the error case? I feel
like it'd be cleaner to have something like
enum APTFileResult {
Ok(Vec<APTRepository>),
Error(String),
}
if we go for a file-level approach.
>>> we have a warning for Debian unstable, but none for Debian testing which
>>> should also never be enabled on a production machine.
>>>
>>
>> If there is an "upgrade suites" button/API call, then there would be
>> warnings after using that. But since enabling that button/API call needs
>> to happen anyways before each major release, I guess removing the e.g.
>> 'bullseye' warnings then is just one more place to touch.
>
> no, because 'bullseye' is not 'testing' ;) or at least, they should not
> be treated the same. having 'bullseye' as a suite is fine when upgrading
> from 'buster' to 'bullseye'. having 'testing' there is never good on a
> stable/production system.
>
>> Or maybe add a 'before_major_release' parameter to check_repositories
>> and also to the UI? Then only the product specific code needs to be
>> touched before each major release. Of course there still needs to be a
>> new version of the library for after each major release.
>
> the check for 'bullseye' could contain text that indicates that the
> warning is benign if you are preparing the major release upgrade? or the
> 'upgrade suites' button sets a flag somewhere (comment? ;)), and that
> then automatically skips that check for the given suite?
>
I'd be in favor of the benign warning message, but the problem is that
it would be displayed once for every repository. The comment flag seems
a bit hacky, who would remove that again after the upgrade? As one
should always update to the last version of the current suite before
attempting a major upgrade, and because we need to touch the code to
enable the button/API call anyways, why is my suggestion with the
parameter not good enough? And just a quick idea that would need to be
fleshed out: having a major_upgrade_possible file packaged somewhere and
a major_update_possible API call, would lead to a single place to touch.
>>> we might want to match known-official repo URIs (ftp.*.debian.org,
>>> deb.debian.org, download.proxmox.com, enterprise.proxmox.com) to mark
>>> "potentially dangerous external repositories" (or to give the official
>>> ones a "official mirror" badge or something like that)
>>>
>>
>> Might be done as part of the check_repositories call if we go for
>> warnings. If we go for badges, we'd need something new or change the
>> interface for check_repositories. I feel like badges would be preferable
>> though...
>
> badges are nicer since they just highlight "this is good", which is
> easier to determine than "this is bad" (which is actually just "this
> might be bad"). having some custom repo that is okay is not that
> uncommon (internal software, monitoring, ansible integration packages,
> ..), so being too noisy might be a bad idea.
>
I'll try and hope to find a clean way to adapt the check_repositories
interface.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI
2021-03-24 12:08 ` Fabian Ebner
@ 2021-03-24 12:39 ` Fabian Grünbichler
0 siblings, 0 replies; 16+ messages in thread
From: Fabian Grünbichler @ 2021-03-24 12:39 UTC (permalink / raw)
To: Fabian Ebner, pbs-devel
On March 24, 2021 1:08 pm, Fabian Ebner wrote:
> Am 24.03.21 um 11:06 schrieb Fabian Grünbichler:
>> snipped ;)
>>
>> On March 24, 2021 10:40 am, Fabian Ebner wrote:
>>> Am 23.03.21 um 11:29 schrieb Fabian Grünbichler:
>>>> if a file cannot be parsed or is malformed (e.g. because I put "Uris"
>>>> instead of "URIs" in a .sources file ;)), the whole API call fails with
>>>> 400. it might be more user-friendly to mark indiviual .list/.sources
>>>> files as containing invalid entries which are not displayed, and still
>>>> return the rest? might make the result less actionable since we don't
>>>> have the complete picture, but it still might be better than a single
>>>> error message for one of X files..
>>>>
>>>
>>> I did consider something like this for a bit, but not sure how to
>>> cleanly organize the API then. The call should still error out in my
>>> opinion and syntactic errors should be rather rare anyways (apt also
>>> just complains when it cannot parse). We could continue parsing and
>>> collect all the errors at once at least, but not sure if that's worth it?
>>
>> I was thinking more of further inconsistencies between APT and our
>> parser like the case-sensitivity issue. like you said, APT will just
>> ignore a wrong/broken entry or file, and not error out altogether. so
>> maybe it would make sense to mimic that behaviour -> add a
>> warnings/error field, and let the caller decide whether it just wants to
>> display that or the (partial) result + the additional information. e.g.,
>> we could disable editing via the GUI for invalid files, but still allow
>> it for other files.
>>
>> IMHO for this the request as a whole does not have to error out on the
>> API/HTTP level if the parser encounters something it does not
>> understand.
>>
>
> apt seems to error out on the first problem for each file, and it will
> error out before doing anything else when the parsing failed.
>
> Is it really worth keeping the partial result in the error case? I feel
> like it'd be cleaner to have something like
>
> enum APTFileResult {
> Ok(Vec<APTRepository>),
> Error(String),
> }
>
> if we go for a file-level approach.
yeah, I meant 'partial' as in return the parser results for good files,
and 'don't know how to parse this' for the bad files. sorry for not
being more explicit.
>
>>>> we have a warning for Debian unstable, but none for Debian testing which
>>>> should also never be enabled on a production machine.
>>>>
>>>
>>> If there is an "upgrade suites" button/API call, then there would be
>>> warnings after using that. But since enabling that button/API call needs
>>> to happen anyways before each major release, I guess removing the e.g.
>>> 'bullseye' warnings then is just one more place to touch.
>>
>> no, because 'bullseye' is not 'testing' ;) or at least, they should not
>> be treated the same. having 'bullseye' as a suite is fine when upgrading
>> from 'buster' to 'bullseye'. having 'testing' there is never good on a
>> stable/production system.
>>
>>> Or maybe add a 'before_major_release' parameter to check_repositories
>>> and also to the UI? Then only the product specific code needs to be
>>> touched before each major release. Of course there still needs to be a
>>> new version of the library for after each major release.
>>
>> the check for 'bullseye' could contain text that indicates that the
>> warning is benign if you are preparing the major release upgrade? or the
>> 'upgrade suites' button sets a flag somewhere (comment? ;)), and that
>> then automatically skips that check for the given suite?
>>
>
> I'd be in favor of the benign warning message, but the problem is that
> it would be displayed once for every repository. The comment flag seems
> a bit hacky, who would remove that again after the upgrade? As one
> should always update to the last version of the current suite before
> attempting a major upgrade, and because we need to touch the code to
> enable the button/API call anyways, why is my suggestion with the
> parameter not good enough? And just a quick idea that would need to be
> fleshed out: having a major_upgrade_possible file packaged somewhere and
> a major_update_possible API call, would lead to a single place to touch.
parameter is okay as well - I was just pointing out other potential
solutions. ideally we'd have some sort of "admin says they want to
upgrade" and after that we treat bullseye repos as okay, as opposed to
"PVE 6.x is recent enough to upgrade" already triggering that. how we
get there is largely irrelevant ;)
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-03-24 12:40 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-22 11:59 [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-apt 01/10] initial commit Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-apt 02/10] add files for Debian packaging Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-apt 03/10] add functions to check for Proxmox repositories Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-apt 04/10] add check_repositories function Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-backup 05/10] depend on new proxmox-apt crate Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-backup 06/10] api: apt: add repositories call Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-backup 07/10] ui: add repositories Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 proxmox-backup 08/10] add check_repositories_call Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 widget-toolkit 09/10] add UI for APT repositories Fabian Ebner
2021-03-22 11:59 ` [pbs-devel] [PATCH v3 widget-toolkit 10/10] APT repositories: add warnings Fabian Ebner
2021-03-23 10:29 ` [pbs-devel] [PATCH-SERIES v3] APT repositories API/UI Fabian Grünbichler
2021-03-24 9:40 ` Fabian Ebner
2021-03-24 10:06 ` Fabian Grünbichler
2021-03-24 12:08 ` Fabian Ebner
2021-03-24 12:39 ` Fabian Grünbichler
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox