* Re: [pbs-devel] [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function
@ 2021-06-18 6:44 Wolfgang Bumiller
2021-06-18 6:53 ` Fabian Ebner
0 siblings, 1 reply; 11+ messages in thread
From: Wolfgang Bumiller @ 2021-06-18 6:44 UTC (permalink / raw)
To: Fabian Ebner; +Cc: pve-devel, pbs-devel
> On 06/18/2021 8:42 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
> >> + Some((last, rest)) => match rest.split_last() {
> >> + Some((second_to_last, _rest)) => {
> >> + (*last == "org" && *second_to_last == "debian")
> >> + || (*last == "com" && *second_to_last == "proxmox")
> >> + }
> >> + None => false,
> >> + },
> >> + None => false,
> >> + };
> >> +
> >> + for uri in self.uris.iter() {
> >> + if let Some(host) = host_from_uri(uri) {
> >> + let domains = host.split('.').collect();
> >
> > ^ But instead of building a vector here, why not just do:
> >
> > if host == "proxmox.com" || host.ends_with(".proxmox.com")
> > || host == "debian.org" || host.ends_with(".debian.org")
> > {
> > ...
> > }
> >
>
> Misses FQDNs?
Such as?
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [pbs-devel] [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function 2021-06-18 6:44 [pbs-devel] [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function Wolfgang Bumiller @ 2021-06-18 6:53 ` Fabian Ebner 0 siblings, 0 replies; 11+ messages in thread From: Fabian Ebner @ 2021-06-18 6:53 UTC (permalink / raw) To: Wolfgang Bumiller; +Cc: pve-devel, pbs-devel Am 18.06.21 um 08:44 schrieb Wolfgang Bumiller: > >> On 06/18/2021 8:42 AM Fabian Ebner <f.ebner@proxmox.com> wrote: >>>> + Some((last, rest)) => match rest.split_last() { >>>> + Some((second_to_last, _rest)) => { >>>> + (*last == "org" && *second_to_last == "debian") >>>> + || (*last == "com" && *second_to_last == "proxmox") >>>> + } >>>> + None => false, >>>> + }, >>>> + None => false, >>>> + }; >>>> + >>>> + for uri in self.uris.iter() { >>>> + if let Some(host) = host_from_uri(uri) { >>>> + let domains = host.split('.').collect(); >>> >>> ^ But instead of building a vector here, why not just do: >>> >>> if host == "proxmox.com" || host.ends_with(".proxmox.com") >>> || host == "debian.org" || host.ends_with(".debian.org") >>> { >>> ... >>> } >>> >> >> Misses FQDNs? > > Such as? > http://security.debian.org. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [pbs-devel] [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function
@ 2021-06-18 7:17 Wolfgang Bumiller
0 siblings, 0 replies; 11+ messages in thread
From: Wolfgang Bumiller @ 2021-06-18 7:17 UTC (permalink / raw)
To: Fabian Ebner, pbs-devel, Proxmox VE development discussion
> On 06/18/2021 9:07 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
>
>
> Am 18.06.21 um 08:58 schrieb Fabian Ebner:
> > Am 18.06.21 um 08:56 schrieb Wolfgang Bumiller:
> >>
> >>> On 06/18/2021 8:53 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
> >>>
> >>> Am 18.06.21 um 08:44 schrieb Wolfgang Bumiller:
> >>>>
> >>>>> On 06/18/2021 8:42 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
> >>>>>>> + Some((last, rest)) => match rest.split_last() {
> >>>>>>> + Some((second_to_last, _rest)) => {
> >>>>>>> + (*last == "org" && *second_to_last == "debian")
> >>>>>>> + || (*last == "com" && *second_to_last ==
> >>>>>>> "proxmox")
> >>>>>>> + }
> >>>>>>> + None => false,
> >>>>>>> + },
> >>>>>>> + None => false,
> >>>>>>> + };
> >>>>>>> +
> >>>>>>> + for uri in self.uris.iter() {
> >>>>>>> + if let Some(host) = host_from_uri(uri) {
> >>>>>>> + let domains = host.split('.').collect();
> >>>>>>
> >>>>>> ^ But instead of building a vector here, why not just do:
> >>>>>>
> >>>>>> if host == "proxmox.com" || host.ends_with(".proxmox.com")
> >>>>>> || host == "debian.org" || host.ends_with(".debian.org")
> >>>>>> {
> >>>>>> ...
> >>>>>> }
> >>>>>>
> >>>>>
> >>>>> Misses FQDNs?
> >>>>
> >>>> Such as?
> >>>>
> >>>
> >>> http://security.debian.org.
> >>
> >> Why is that not caught by `.ends_with(".debian.org")`?
> >>
> >
> > Because of the final dot. But it is likely very uncommon and simply
> > splitting by '.' leads to false results with e.g.
> > http://security..debian.org too, so it might not be worth worrying about...
> >
>
> Sorry, should've been http://security.debian..org
I still can't navigate to that in my browser ;-)
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [pbs-devel] [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function @ 2021-06-18 7:16 Wolfgang Bumiller 2021-06-18 7:26 ` Fabian Ebner 0 siblings, 1 reply; 11+ messages in thread From: Wolfgang Bumiller @ 2021-06-18 7:16 UTC (permalink / raw) To: Fabian Ebner; +Cc: pve-devel, pbs-devel > On 06/18/2021 8:58 AM Fabian Ebner <f.ebner@proxmox.com> wrote: > > > Am 18.06.21 um 08:56 schrieb Wolfgang Bumiller: > > > >> On 06/18/2021 8:53 AM Fabian Ebner <f.ebner@proxmox.com> wrote: > >> > >> > >> Am 18.06.21 um 08:44 schrieb Wolfgang Bumiller: > >>> > >>>> On 06/18/2021 8:42 AM Fabian Ebner <f.ebner@proxmox.com> wrote: > >>>>>> + Some((last, rest)) => match rest.split_last() { > >>>>>> + Some((second_to_last, _rest)) => { > >>>>>> + (*last == "org" && *second_to_last == "debian") > >>>>>> + || (*last == "com" && *second_to_last == "proxmox") > >>>>>> + } > >>>>>> + None => false, > >>>>>> + }, > >>>>>> + None => false, > >>>>>> + }; > >>>>>> + > >>>>>> + for uri in self.uris.iter() { > >>>>>> + if let Some(host) = host_from_uri(uri) { > >>>>>> + let domains = host.split('.').collect(); > >>>>> > >>>>> ^ But instead of building a vector here, why not just do: > >>>>> > >>>>> if host == "proxmox.com" || host.ends_with(".proxmox.com") > >>>>> || host == "debian.org" || host.ends_with(".debian.org") > >>>>> { > >>>>> ... > >>>>> } > >>>>> > >>>> > >>>> Misses FQDNs? > >>> > >>> Such as? > >>> > >> > >> http://security.debian.org. > > > > Why is that not caught by `.ends_with(".debian.org")`? > > > > Because of the final dot. Splitting at '.' gives you an empty element in your vector, so that's the same in your code... Feel free to just strip the final dot, though, if it makes you feel any better :-P > But it is likely very uncommon and simply Do people even really do that, ever, outside of zone files? > splitting by '.' leads to false results with e.g. > http://security..debian.org too, so it might not be worth worrying about... That doesn't work anyway... ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [pbs-devel] [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function 2021-06-18 7:16 Wolfgang Bumiller @ 2021-06-18 7:26 ` Fabian Ebner 0 siblings, 0 replies; 11+ messages in thread From: Fabian Ebner @ 2021-06-18 7:26 UTC (permalink / raw) To: Wolfgang Bumiller; +Cc: pve-devel, pbs-devel Am 18.06.21 um 09:16 schrieb Wolfgang Bumiller: > >> On 06/18/2021 8:58 AM Fabian Ebner <f.ebner@proxmox.com> wrote: >> >> >> Am 18.06.21 um 08:56 schrieb Wolfgang Bumiller: >>> >>>> On 06/18/2021 8:53 AM Fabian Ebner <f.ebner@proxmox.com> wrote: >>>> >>>> >>>> Am 18.06.21 um 08:44 schrieb Wolfgang Bumiller: >>>>> >>>>>> On 06/18/2021 8:42 AM Fabian Ebner <f.ebner@proxmox.com> wrote: >>>>>>>> + Some((last, rest)) => match rest.split_last() { >>>>>>>> + Some((second_to_last, _rest)) => { >>>>>>>> + (*last == "org" && *second_to_last == "debian") >>>>>>>> + || (*last == "com" && *second_to_last == "proxmox") >>>>>>>> + } >>>>>>>> + None => false, >>>>>>>> + }, >>>>>>>> + None => false, >>>>>>>> + }; >>>>>>>> + >>>>>>>> + for uri in self.uris.iter() { >>>>>>>> + if let Some(host) = host_from_uri(uri) { >>>>>>>> + let domains = host.split('.').collect(); >>>>>>> >>>>>>> ^ But instead of building a vector here, why not just do: >>>>>>> >>>>>>> if host == "proxmox.com" || host.ends_with(".proxmox.com") >>>>>>> || host == "debian.org" || host.ends_with(".debian.org") >>>>>>> { >>>>>>> ... >>>>>>> } >>>>>>> >>>>>> >>>>>> Misses FQDNs? >>>>> >>>>> Such as? >>>>> >>>> >>>> http://security.debian.org. >>> >>> Why is that not caught by `.ends_with(".debian.org")`? >>> >> >> Because of the final dot. > > Splitting at '.' gives you an empty element in your vector, so that's the same in your code... > Good to know. > Feel free to just strip the final dot, though, if it makes you feel any better :-P > >> But it is likely very uncommon and simply > > Do people even really do that, ever, outside of zone files? > >> splitting by '.' leads to false results with e.g. >> http://security..debian.org too, so it might not be worth worrying about... > > That doesn't work anyway... > Yeah, I meant http://security.debian..org and thought it would be a false positive, but if there's an empty element in the vector, it isn't. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [pbs-devel] [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function
@ 2021-06-18 6:56 Wolfgang Bumiller
2021-06-18 6:58 ` Fabian Ebner
0 siblings, 1 reply; 11+ messages in thread
From: Wolfgang Bumiller @ 2021-06-18 6:56 UTC (permalink / raw)
To: Fabian Ebner; +Cc: pve-devel, pbs-devel
> On 06/18/2021 8:53 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
>
>
> Am 18.06.21 um 08:44 schrieb Wolfgang Bumiller:
> >
> >> On 06/18/2021 8:42 AM Fabian Ebner <f.ebner@proxmox.com> wrote:
> >>>> + Some((last, rest)) => match rest.split_last() {
> >>>> + Some((second_to_last, _rest)) => {
> >>>> + (*last == "org" && *second_to_last == "debian")
> >>>> + || (*last == "com" && *second_to_last == "proxmox")
> >>>> + }
> >>>> + None => false,
> >>>> + },
> >>>> + None => false,
> >>>> + };
> >>>> +
> >>>> + for uri in self.uris.iter() {
> >>>> + if let Some(host) = host_from_uri(uri) {
> >>>> + let domains = host.split('.').collect();
> >>>
> >>> ^ But instead of building a vector here, why not just do:
> >>>
> >>> if host == "proxmox.com" || host.ends_with(".proxmox.com")
> >>> || host == "debian.org" || host.ends_with(".debian.org")
> >>> {
> >>> ...
> >>> }
> >>>
> >>
> >> Misses FQDNs?
> >
> > Such as?
> >
>
> http://security.debian.org.
Why is that not caught by `.ends_with(".debian.org")`?
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [pbs-devel] [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function 2021-06-18 6:56 Wolfgang Bumiller @ 2021-06-18 6:58 ` Fabian Ebner 2021-06-18 7:07 ` Fabian Ebner 0 siblings, 1 reply; 11+ messages in thread From: Fabian Ebner @ 2021-06-18 6:58 UTC (permalink / raw) To: Wolfgang Bumiller; +Cc: pve-devel, pbs-devel Am 18.06.21 um 08:56 schrieb Wolfgang Bumiller: > >> On 06/18/2021 8:53 AM Fabian Ebner <f.ebner@proxmox.com> wrote: >> >> >> Am 18.06.21 um 08:44 schrieb Wolfgang Bumiller: >>> >>>> On 06/18/2021 8:42 AM Fabian Ebner <f.ebner@proxmox.com> wrote: >>>>>> + Some((last, rest)) => match rest.split_last() { >>>>>> + Some((second_to_last, _rest)) => { >>>>>> + (*last == "org" && *second_to_last == "debian") >>>>>> + || (*last == "com" && *second_to_last == "proxmox") >>>>>> + } >>>>>> + None => false, >>>>>> + }, >>>>>> + None => false, >>>>>> + }; >>>>>> + >>>>>> + for uri in self.uris.iter() { >>>>>> + if let Some(host) = host_from_uri(uri) { >>>>>> + let domains = host.split('.').collect(); >>>>> >>>>> ^ But instead of building a vector here, why not just do: >>>>> >>>>> if host == "proxmox.com" || host.ends_with(".proxmox.com") >>>>> || host == "debian.org" || host.ends_with(".debian.org") >>>>> { >>>>> ... >>>>> } >>>>> >>>> >>>> Misses FQDNs? >>> >>> Such as? >>> >> >> http://security.debian.org. > > Why is that not caught by `.ends_with(".debian.org")`? > Because of the final dot. But it is likely very uncommon and simply splitting by '.' leads to false results with e.g. http://security..debian.org too, so it might not be worth worrying about... ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [pbs-devel] [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function 2021-06-18 6:58 ` Fabian Ebner @ 2021-06-18 7:07 ` Fabian Ebner 0 siblings, 0 replies; 11+ messages in thread From: Fabian Ebner @ 2021-06-18 7:07 UTC (permalink / raw) To: pbs-devel, Wolfgang Bumiller, Proxmox VE development discussion Am 18.06.21 um 08:58 schrieb Fabian Ebner: > Am 18.06.21 um 08:56 schrieb Wolfgang Bumiller: >> >>> On 06/18/2021 8:53 AM Fabian Ebner <f.ebner@proxmox.com> wrote: >>> >>> Am 18.06.21 um 08:44 schrieb Wolfgang Bumiller: >>>> >>>>> On 06/18/2021 8:42 AM Fabian Ebner <f.ebner@proxmox.com> wrote: >>>>>>> + Some((last, rest)) => match rest.split_last() { >>>>>>> + Some((second_to_last, _rest)) => { >>>>>>> + (*last == "org" && *second_to_last == "debian") >>>>>>> + || (*last == "com" && *second_to_last == >>>>>>> "proxmox") >>>>>>> + } >>>>>>> + None => false, >>>>>>> + }, >>>>>>> + None => false, >>>>>>> + }; >>>>>>> + >>>>>>> + for uri in self.uris.iter() { >>>>>>> + if let Some(host) = host_from_uri(uri) { >>>>>>> + let domains = host.split('.').collect(); >>>>>> >>>>>> ^ But instead of building a vector here, why not just do: >>>>>> >>>>>> if host == "proxmox.com" || host.ends_with(".proxmox.com") >>>>>> || host == "debian.org" || host.ends_with(".debian.org") >>>>>> { >>>>>> ... >>>>>> } >>>>>> >>>>> >>>>> Misses FQDNs? >>>> >>>> Such as? >>>> >>> >>> http://security.debian.org. >> >> Why is that not caught by `.ends_with(".debian.org")`? >> > > Because of the final dot. But it is likely very uncommon and simply > splitting by '.' leads to false results with e.g. > http://security..debian.org too, so it might not be worth worrying about... > Sorry, should've been http://security.debian..org > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [pbs-devel] [PATCH-SERIES v6] APT repositories API/UI
@ 2021-06-11 11:43 Fabian Ebner
2021-06-11 11:43 ` [pbs-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function Fabian Ebner
0 siblings, 1 reply; 11+ messages in thread
From: Fabian Ebner @ 2021-06-11 11:43 UTC (permalink / raw)
To: pve-devel, pbs-devel
List the configured repositories, have some basic checks for them, and
allow upgrading the package distribution before a major release.
The plan is to use perlmod to make the Rust implementation available for PVE+PMG
as well.
Changes from v5:
* The main addition is how the upgrade call is enabled/disabled:
The idea is to do it in the proxmox-apt library itself and bump the
versions accordingly. It'll always be disabled in the master branch and
after each major release the major version will be bumped, while the
stable branch will bump the minor version when it enables the call. Each
product can then depend on the new major version in its master branch and
on the new minor version in its stable branch.
Hope I didn't miss a much easier/better way to do this.
* Replace buster/updates with bullseye-security for the upgrade.
* Assert that current os-release is what we expect for the upgrade function.
* Add link to the upgrade docs in the upgrade button confirm dialog.
* Avoid false negatives by marking *.proxmox.com *.debian.org as official
(e.g. snapshot.debian.org was missing)
* Handle port and userinfo in URI for host detection.
Still missing (intended as followups):
* integration in PMG.
Changes v4 -> v5:
* some minor style improvements
* call cargo clean with make clean in proxmox-apt
* require 'deb' package type to detect enterprise/no-subscription repos
* moved common_digest helper to the library
* add replace_suite function in proxmox-apt and refactored suite_is_variant
for re-use
* ui: add reload button
* ui: don't pass undefined digest parameter
* add upgrade button
* add RFC for PBS upgrade call
* add RFCs for PVE integration
Changes v3 -> v4:
* incorporate Fabian G.'s feedback:
* switch to a per-file approach
* check for official host names
* fix case-sensitivity issue for .sources keys
* include digests
* fix write issue when there are no components (in case of an absolute suite)
* add more tests
Changes v2 -> v3:
* incorporate Wolfgang's feedback
* improve main warning's UI
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 (9):
initial commit
add files for Debian packaging
add functions to check for Proxmox repositories
add check_repositories function
add common_digest helper
add release_upgrade function and constants for the current and upgrade
suite
bump version to 0.1.1-1
update for bullseye
bump version to 1.0.0-1
Fabian Ebner (2):
allow upgrade to bullseye
bump version to 0.2.0-1
proxmox-widget-toolkit:
Fabian Ebner (3):
add UI for APT repositories
APT repositories: add warnings
add upgrade button
src/Makefile | 1 +
src/node/APTRepositories.js | 477 ++++++++++++++++++++++++++++++++++++
2 files changed, 478 insertions(+)
create mode 100644 src/node/APTRepositories.js
proxmox-backup:
Fabian Ebner (6):
depend on new proxmox-apt crate
api: apt: add repositories call
ui: add APT repositories
api: apt: add check_repositories_call
add upgrade_repositories call
enable release upgrade for package repositories
Cargo.toml | 1 +
debian/control | 1 +
src/api2/node/apt.rs | 177 +++++++++++++++++++++++++++++++++++-
www/ServerAdministration.js | 13 +++
4 files changed, 191 insertions(+), 1 deletion(-)
pve-rs:
Fabian Ebner (3):
initial commit
add files for Debian packaging
apt: add upgrade_repositories call
Fabian Ebner (1):
depend on proxmox-apt 0.2.0
pve-manager:
Fabian Ebner (5):
api: apt: add call to list repositories
ui: add panel for listing APT repositories
api: apt: add call for repository check
api: apt: add upgrade repos call
ui: node config: enable release upgrade button for package
repositories
PVE/API2/APT.pm | 267 ++++++++++++++++++++++++++++++++++++
www/manager6/node/Config.js | 9 ++
2 files changed, 276 insertions(+)
--
2.20.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [pbs-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function 2021-06-11 11:43 [pbs-devel] [PATCH-SERIES v6] APT repositories API/UI Fabian Ebner @ 2021-06-11 11:43 ` Fabian Ebner 2021-06-17 8:39 ` [pbs-devel] [pve-devel] " Wolfgang Bumiller 2021-06-17 14:16 ` Fabian Grünbichler 0 siblings, 2 replies; 11+ messages in thread From: Fabian Ebner @ 2021-06-11 11:43 UTC (permalink / raw) To: pve-devel, pbs-devel which checks for bad suites and official URIs. Signed-off-by: Fabian Ebner <f.ebner@proxmox.com> --- Changes from v5: * split out host_from_uri helper and also handle userinfo and port * test an offical URI with port * match all *.debian.org and *.proxmox.com as official to avoid (future) false negatives. * add bookworm and trixie codenames to the list of new_suites src/repositories/check.rs | 174 +++++++++++++++++++++- src/repositories/mod.rs | 19 ++- src/types.rs | 19 +++ tests/repositories.rs | 97 +++++++++++- tests/sources.list.d.expected/bad.sources | 30 ++++ tests/sources.list.d/bad.sources | 29 ++++ 6 files changed, 364 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 a682b69..585c28d 100644 --- a/src/repositories/check.rs +++ b/src/repositories/check.rs @@ -1,6 +1,45 @@ use anyhow::{bail, Error}; -use crate::types::{APTRepository, APTRepositoryFileType, APTRepositoryPackageType}; +use crate::types::{ + APTRepository, APTRepositoryFile, APTRepositoryFileType, APTRepositoryInfo, + APTRepositoryPackageType, +}; + +/// Splits the suite into its base part and variant. +fn suite_variant(suite: &str) -> (&str, &str) { + let variants = ["-backports-sloppy", "-backports", "-updates", "/updates"]; + + for variant in variants.iter() { + if let Some(base) = suite.strip_suffix(variant) { + return (base, variant); + } + } + + (suite, "") +} + +/// Get the host part from a given URI. +fn host_from_uri(uri: &str) -> Option<&str> { + if let Some(begin) = uri.find("://") { + let mut host = uri.split_at(begin + 3).1; + + if let Some(end) = host.find('/') { + host = host.split_at(end).0; + } + + if let Some(begin) = host.find('@') { + host = host.split_at(begin + 1).1; + } + + if let Some(end) = host.find(':') { + host = host.split_at(end).0; + } + + return Some(host); + } + + None +} impl APTRepository { /// Makes sure that all basic properties of a repository are present and @@ -102,4 +141,137 @@ impl APTRepository { false } } + + /// Checks if old or unstable suites are configured and also that the + /// `stable` keyword is not used. + fn check_suites(&self, add_info: &mut dyn FnMut(String, String)) { + let old_suites = [ + "lenny", + "squeeze", + "wheezy", + "jessie", + "stretch", + "oldoldstable", + "oldstable", + ]; + + let next_suite = "bullseye"; + + let new_suites = [ + "bookworm", + "trixie", + "testing", + "unstable", + "sid", + "experimental", + ]; + + 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_variant(suite).0 == *base_suite) + { + add_info( + "warning".to_string(), + format!("old suite '{}' configured!", suite), + ); + } + + if suite_variant(suite).0 == next_suite { + add_info( + "ignore-pre-upgrade-warning".to_string(), + format!("suite '{}' should not be used in production!", suite), + ); + } + + if new_suites + .iter() + .any(|base_suite| suite_variant(suite).0 == *base_suite) + { + add_info( + "warning".to_string(), + format!("suite '{}' should not be used in production!", suite), + ); + } + + if suite_variant(suite).0 == "stable" { + add_info( + "warning".to_string(), + "use the name of the stable distribution instead of 'stable'!".to_string(), + ); + } + } + } + } + + /// Checks if an official host is configured in the repository. + fn check_uris(&self) -> Option<(String, String)> { + let official_host = |domains: &Vec<&str>| match domains.split_last() { + Some((last, rest)) => match rest.split_last() { + Some((second_to_last, _rest)) => { + (*last == "org" && *second_to_last == "debian") + || (*last == "com" && *second_to_last == "proxmox") + } + None => false, + }, + None => false, + }; + + for uri in self.uris.iter() { + if let Some(host) = host_from_uri(uri) { + let domains = host.split('.').collect(); + + if official_host(&domains) { + return Some(("badge".to_string(), "official host name".to_string())); + } + } + } + + None + } +} + +impl APTRepositoryFile { + /// Checks if old or unstable suites are configured and also that the + /// `stable` keyword is not used. + pub fn check_suites(&self) -> Vec<APTRepositoryInfo> { + let mut infos = vec![]; + + for (n, repo) in self.repositories.iter().enumerate() { + let mut add_info = |kind, message| { + infos.push(APTRepositoryInfo { + path: self.path.clone(), + number: n + 1, + kind, + message, + }) + }; + repo.check_suites(&mut add_info); + } + + infos + } + + /// Checks for official URIs. + pub fn check_uris(&self) -> Vec<APTRepositoryInfo> { + let mut infos = vec![]; + + for (n, repo) in self.repositories.iter().enumerate() { + if let Some((kind, message)) = repo.check_uris() { + infos.push(APTRepositoryInfo { + path: self.path.clone(), + number: n + 1, + kind, + message, + }); + } + } + + infos + } } diff --git a/src/repositories/mod.rs b/src/repositories/mod.rs index b7919a9..c2bbc06 100644 --- a/src/repositories/mod.rs +++ b/src/repositories/mod.rs @@ -4,7 +4,7 @@ use anyhow::{bail, format_err, Error}; use crate::types::{ APTRepository, APTRepositoryFile, APTRepositoryFileError, APTRepositoryFileType, - APTRepositoryOption, + APTRepositoryInfo, APTRepositoryOption, }; mod list_parser; @@ -148,6 +148,23 @@ impl APTRepositoryFile { } } +/// Provides additional information about the repositories. +/// +/// The kind of information can be: +/// `warnings` for bad suites. +/// `ignore-pre-upgrade-warning` when the next stable suite is configured. +/// `badge` for official URIs. +pub fn check_repositories(files: &[APTRepositoryFile]) -> Vec<APTRepositoryInfo> { + let mut infos = vec![]; + + for file in files.iter() { + infos.append(&mut file.check_suites()); + infos.append(&mut file.check_uris()); + } + + infos +} + /// Checks if the enterprise repository for the specified Proxmox product is /// configured and enabled. pub fn enterprise_repository_enabled(files: &[APTRepositoryFile], product: &str) -> bool { diff --git a/src/types.rs b/src/types.rs index bbd8e7e..057fffa 100644 --- a/src/types.rs +++ b/src/types.rs @@ -244,3 +244,22 @@ impl std::error::Error for APTRepositoryFileError { None } } + +#[api] +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] +#[serde(rename_all = "lowercase")] +/// Additional information for a repository. +pub struct APTRepositoryInfo { + /// Path to the defining file. + #[serde(skip_serializing_if = "String::is_empty")] + pub path: String, + + /// Number of the associated respository within the file. + pub number: usize, + + /// Info kind (e.g. "warning") + pub kind: String, + + /// Info message + pub message: String, +} diff --git a/tests/repositories.rs b/tests/repositories.rs index ffb1888..9b0cd56 100644 --- a/tests/repositories.rs +++ b/tests/repositories.rs @@ -3,9 +3,10 @@ use std::path::PathBuf; use anyhow::{bail, format_err, Error}; use proxmox_apt::repositories::{ - enterprise_repository_enabled, no_subscription_repository_enabled, write_repositories, + check_repositories, enterprise_repository_enabled, no_subscription_repository_enabled, + write_repositories, }; -use proxmox_apt::types::APTRepositoryFile; +use proxmox_apt::types::{APTRepositoryFile, APTRepositoryInfo}; #[test] fn test_parse_write() -> Result<(), Error> { @@ -159,3 +160,95 @@ 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 absolute_suite_list = read_dir.join("absolute_suite.list"); + let mut file = APTRepositoryFile::new(&absolute_suite_list)?.unwrap(); + file.parse()?; + + let infos = check_repositories(&vec![file]); + + assert_eq!(infos.is_empty(), true); + let pve_list = read_dir.join("pve.list"); + let mut file = APTRepositoryFile::new(&pve_list)?.unwrap(); + file.parse()?; + + let path_string = pve_list.into_os_string().into_string().unwrap(); + + let mut expected_infos = vec![]; + for n in 1..=5 { + expected_infos.push(APTRepositoryInfo { + path: path_string.clone(), + number: n, + kind: "badge".to_string(), + message: "official host name".to_string(), + }); + } + + let mut infos = check_repositories(&vec![file]); + + assert_eq!(infos.sort(), expected_infos.sort()); + + let bad_sources = read_dir.join("bad.sources"); + let mut file = APTRepositoryFile::new(&bad_sources)?.unwrap(); + file.parse()?; + + let path_string = bad_sources.into_os_string().into_string().unwrap(); + + let mut expected_infos = vec![ + APTRepositoryInfo { + path: path_string.clone(), + number: 1, + kind: "warning".to_string(), + message: "suite 'sid' should not be used in production!".to_string(), + }, + APTRepositoryInfo { + path: path_string.clone(), + number: 2, + kind: "warning".to_string(), + message: "old suite 'lenny-backports' configured!".to_string(), + }, + APTRepositoryInfo { + path: path_string.clone(), + number: 3, + kind: "warning".to_string(), + message: "old suite 'stretch/updates' configured!".to_string(), + }, + APTRepositoryInfo { + path: path_string.clone(), + number: 4, + kind: "warning".to_string(), + message: "use the name of the stable distribution instead of 'stable'!".to_string(), + }, + APTRepositoryInfo { + path: path_string.clone(), + number: 5, + kind: "ignore-pre-upgrade-warning".to_string(), + message: "suite 'bullseye' should not be used in production!".to_string(), + }, + APTRepositoryInfo { + path: path_string.clone(), + number: 6, + kind: "warning".to_string(), + message: "suite 'testing' should not be used in production!".to_string(), + }, + ]; + for n in 1..=6 { + expected_infos.push(APTRepositoryInfo { + path: path_string.clone(), + number: n, + kind: "badge".to_string(), + message: "official URI".to_string(), + }); + } + + let mut infos = check_repositories(&vec![file]); + + assert_eq!(infos.sort(), expected_infos.sort()); + + 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..b630c89 --- /dev/null +++ b/tests/sources.list.d.expected/bad.sources @@ -0,0 +1,30 @@ +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:80 +Suites: stretch/updates +Components: main contrib + +Types: deb +URIs: http://ftp.at.debian.org:80/debian +Suites: stable +Components: main + +Types: deb +URIs: http://ftp.at.debian.org/debian +Suites: bullseye +Components: main + +Types: deb +URIs: http://ftp.at.debian.org/debian +Suites: testing +Components: main + diff --git a/tests/sources.list.d/bad.sources b/tests/sources.list.d/bad.sources new file mode 100644 index 0000000..1aab2ea --- /dev/null +++ b/tests/sources.list.d/bad.sources @@ -0,0 +1,29 @@ +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:80 +Suites: stretch/updates +Components: main contrib + +Suites: stable +URIs: http://ftp.at.debian.org:80/debian +Components: main +Types: deb + +Suites: bullseye +URIs: http://ftp.at.debian.org/debian +Components: main +Types: deb + +Suites: testing +URIs: http://ftp.at.debian.org/debian +Components: main +Types: deb -- 2.20.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [pbs-devel] [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function 2021-06-11 11:43 ` [pbs-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function Fabian Ebner @ 2021-06-17 8:39 ` Wolfgang Bumiller 2021-06-18 6:42 ` Fabian Ebner 2021-06-17 14:16 ` Fabian Grünbichler 1 sibling, 1 reply; 11+ messages in thread From: Wolfgang Bumiller @ 2021-06-17 8:39 UTC (permalink / raw) To: Fabian Ebner; +Cc: pve-devel, pbs-devel some non-blocking cleanups in case you do another version: On Fri, Jun 11, 2021 at 01:43:53PM +0200, Fabian Ebner wrote: > which checks for bad suites and official URIs. > > Signed-off-by: Fabian Ebner <f.ebner@proxmox.com> > --- > > Changes from v5: > * split out host_from_uri helper and also handle userinfo and port > * test an offical URI with port > * match all *.debian.org and *.proxmox.com as official to avoid (future) > false negatives. > * add bookworm and trixie codenames to the list of new_suites > > src/repositories/check.rs | 174 +++++++++++++++++++++- > src/repositories/mod.rs | 19 ++- > src/types.rs | 19 +++ > tests/repositories.rs | 97 +++++++++++- > tests/sources.list.d.expected/bad.sources | 30 ++++ > tests/sources.list.d/bad.sources | 29 ++++ > 6 files changed, 364 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 a682b69..585c28d 100644 > --- a/src/repositories/check.rs > +++ b/src/repositories/check.rs > @@ -1,6 +1,45 @@ > use anyhow::{bail, Error}; > > -use crate::types::{APTRepository, APTRepositoryFileType, APTRepositoryPackageType}; > +use crate::types::{ > + APTRepository, APTRepositoryFile, APTRepositoryFileType, APTRepositoryInfo, > + APTRepositoryPackageType, > +}; > + > +/// Splits the suite into its base part and variant. > +fn suite_variant(suite: &str) -> (&str, &str) { > + let variants = ["-backports-sloppy", "-backports", "-updates", "/updates"]; > + > + for variant in variants.iter() { > + if let Some(base) = suite.strip_suffix(variant) { > + return (base, variant); > + } > + } > + > + (suite, "") > +} > + > +/// Get the host part from a given URI. > +fn host_from_uri(uri: &str) -> Option<&str> { > + if let Some(begin) = uri.find("://") { You could shorten this via `?` (since the function itself also returns an `Option`): let begin = uri.find("://")?; > + let mut host = uri.split_at(begin + 3).1; > + > + if let Some(end) = host.find('/') { > + host = host.split_at(end).0; Personally I'd prefer `host = &host[..end]`, but it probably compiles to the same code in the end. > + } > + > + if let Some(begin) = host.find('@') { > + host = host.split_at(begin + 1).1; (Similarly: `host = &host[(begin + 1)..]`) > + } > + > + if let Some(end) = host.find(':') { > + host = host.split_at(end).0; > + } > + > + return Some(host); > + } > + > + None > +} > > impl APTRepository { > /// Makes sure that all basic properties of a repository are present and > @@ -102,4 +141,137 @@ impl APTRepository { > false > } > } > + > + /// Checks if old or unstable suites are configured and also that the > + /// `stable` keyword is not used. > + fn check_suites(&self, add_info: &mut dyn FnMut(String, String)) { > + let old_suites = [ > + "lenny", > + "squeeze", > + "wheezy", > + "jessie", > + "stretch", > + "oldoldstable", > + "oldstable", > + ]; > + > + let next_suite = "bullseye"; > + > + let new_suites = [ > + "bookworm", > + "trixie", > + "testing", > + "unstable", > + "sid", > + "experimental", > + ]; > + > + if self > + .types > + .iter() > + .any(|package_type| *package_type == APTRepositoryPackageType::Deb) > + { > + for suite in self.suites.iter() { maybe cache `suite_variant(suite).0` at this point let variant = suite_variant(suite).0; > + if old_suites > + .iter() > + .any(|base_suite| suite_variant(suite).0 == *base_suite) ^ then this could be if old_suites.contains(&variant) { I think > + { > + add_info( > + "warning".to_string(), > + format!("old suite '{}' configured!", suite), > + ); > + } > + > + if suite_variant(suite).0 == next_suite { > + add_info( > + "ignore-pre-upgrade-warning".to_string(), > + format!("suite '{}' should not be used in production!", suite), > + ); > + } > + > + if new_suites > + .iter() > + .any(|base_suite| suite_variant(suite).0 == *base_suite) ^ same > + { > + add_info( > + "warning".to_string(), > + format!("suite '{}' should not be used in production!", suite), > + ); > + } > + > + if suite_variant(suite).0 == "stable" { > + add_info( > + "warning".to_string(), > + "use the name of the stable distribution instead of 'stable'!".to_string(), > + ); > + } > + } > + } > + } > + > + /// Checks if an official host is configured in the repository. > + fn check_uris(&self) -> Option<(String, String)> { > + let official_host = |domains: &Vec<&str>| match domains.split_last() { Drop this entire beast (see below), but as a review of it: You can use the slice[1] & rest[2] pattern syntax here: #[allow(clippy::match_like_matches_macro)] match domains[..] { // the `[..]` part is required here [.., "proxmox", "com"] => true, [.., "debian", "org"] => true, _ => false, } Or more concise (but I do find the above a bit quicker to glance over, hence the 'clippy' hint ;-) ): matches!(domains[..], [.., "proxmox", "com"] | [.., "debian", "org"]); [1] https://doc.rust-lang.org/reference/patterns.html#slice-patterns [2] https://doc.rust-lang.org/reference/patterns.html#rest-patterns > + Some((last, rest)) => match rest.split_last() { > + Some((second_to_last, _rest)) => { > + (*last == "org" && *second_to_last == "debian") > + || (*last == "com" && *second_to_last == "proxmox") > + } > + None => false, > + }, > + None => false, > + }; > + > + for uri in self.uris.iter() { > + if let Some(host) = host_from_uri(uri) { > + let domains = host.split('.').collect(); ^ But instead of building a vector here, why not just do: if host == "proxmox.com" || host.ends_with(".proxmox.com") || host == "debian.org" || host.ends_with(".debian.org") { ... } > + > + if official_host(&domains) { > + return Some(("badge".to_string(), "official host name".to_string())); > + } > + } > + } > + > + None > + } > +} > + > +impl APTRepositoryFile { > + /// Checks if old or unstable suites are configured and also that the > + /// `stable` keyword is not used. > + pub fn check_suites(&self) -> Vec<APTRepositoryInfo> { > + let mut infos = vec![]; > + > + for (n, repo) in self.repositories.iter().enumerate() { > + let mut add_info = |kind, message| { > + infos.push(APTRepositoryInfo { > + path: self.path.clone(), > + number: n + 1, > + kind, > + message, > + }) > + }; > + repo.check_suites(&mut add_info); ^ minor nit: the `check_suites` you're calling here is only called at this one spot and private, so personally I'd prefer an `impl FnMut` or generic over a trait object, (also you could inline the closure here) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [pbs-devel] [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function 2021-06-17 8:39 ` [pbs-devel] [pve-devel] " Wolfgang Bumiller @ 2021-06-18 6:42 ` Fabian Ebner 0 siblings, 0 replies; 11+ messages in thread From: Fabian Ebner @ 2021-06-18 6:42 UTC (permalink / raw) To: Wolfgang Bumiller; +Cc: pve-devel, pbs-devel Am 17.06.21 um 10:39 schrieb Wolfgang Bumiller: > some non-blocking cleanups in case you do another version: > > On Fri, Jun 11, 2021 at 01:43:53PM +0200, Fabian Ebner wrote: >> which checks for bad suites and official URIs. >> >> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com> >> --- >> >> Changes from v5: >> * split out host_from_uri helper and also handle userinfo and port >> * test an offical URI with port >> * match all *.debian.org and *.proxmox.com as official to avoid (future) >> false negatives. >> * add bookworm and trixie codenames to the list of new_suites >> >> src/repositories/check.rs | 174 +++++++++++++++++++++- >> src/repositories/mod.rs | 19 ++- >> src/types.rs | 19 +++ >> tests/repositories.rs | 97 +++++++++++- >> tests/sources.list.d.expected/bad.sources | 30 ++++ >> tests/sources.list.d/bad.sources | 29 ++++ >> 6 files changed, 364 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 a682b69..585c28d 100644 >> --- a/src/repositories/check.rs >> +++ b/src/repositories/check.rs >> @@ -1,6 +1,45 @@ >> use anyhow::{bail, Error}; >> >> -use crate::types::{APTRepository, APTRepositoryFileType, APTRepositoryPackageType}; >> +use crate::types::{ >> + APTRepository, APTRepositoryFile, APTRepositoryFileType, APTRepositoryInfo, >> + APTRepositoryPackageType, >> +}; >> + >> +/// Splits the suite into its base part and variant. >> +fn suite_variant(suite: &str) -> (&str, &str) { >> + let variants = ["-backports-sloppy", "-backports", "-updates", "/updates"]; >> + >> + for variant in variants.iter() { >> + if let Some(base) = suite.strip_suffix(variant) { >> + return (base, variant); >> + } >> + } >> + >> + (suite, "") >> +} >> + >> +/// Get the host part from a given URI. >> +fn host_from_uri(uri: &str) -> Option<&str> { >> + if let Some(begin) = uri.find("://") { > > You could shorten this via `?` (since the function itself also returns > an `Option`): > > let begin = uri.find("://")?; > >> + let mut host = uri.split_at(begin + 3).1; >> + >> + if let Some(end) = host.find('/') { >> + host = host.split_at(end).0; > > Personally I'd prefer `host = &host[..end]`, but it probably compiles to > the same code in the end. > >> + } >> + >> + if let Some(begin) = host.find('@') { >> + host = host.split_at(begin + 1).1; > > (Similarly: `host = &host[(begin + 1)..]`) > >> + } >> + >> + if let Some(end) = host.find(':') { >> + host = host.split_at(end).0; >> + } >> + >> + return Some(host); >> + } >> + >> + None >> +} >> >> impl APTRepository { >> /// Makes sure that all basic properties of a repository are present and >> @@ -102,4 +141,137 @@ impl APTRepository { >> false >> } >> } >> + >> + /// Checks if old or unstable suites are configured and also that the >> + /// `stable` keyword is not used. >> + fn check_suites(&self, add_info: &mut dyn FnMut(String, String)) { >> + let old_suites = [ >> + "lenny", >> + "squeeze", >> + "wheezy", >> + "jessie", >> + "stretch", >> + "oldoldstable", >> + "oldstable", >> + ]; >> + >> + let next_suite = "bullseye"; >> + >> + let new_suites = [ >> + "bookworm", >> + "trixie", >> + "testing", >> + "unstable", >> + "sid", >> + "experimental", >> + ]; >> + >> + if self >> + .types >> + .iter() >> + .any(|package_type| *package_type == APTRepositoryPackageType::Deb) >> + { >> + for suite in self.suites.iter() { > > maybe cache `suite_variant(suite).0` at this point > > let variant = suite_variant(suite).0; > >> + if old_suites >> + .iter() >> + .any(|base_suite| suite_variant(suite).0 == *base_suite) > > ^ then this could be > > if old_suites.contains(&variant) { > > I think > >> + { >> + add_info( >> + "warning".to_string(), >> + format!("old suite '{}' configured!", suite), >> + ); >> + } >> + >> + if suite_variant(suite).0 == next_suite { >> + add_info( >> + "ignore-pre-upgrade-warning".to_string(), >> + format!("suite '{}' should not be used in production!", suite), >> + ); >> + } >> + >> + if new_suites >> + .iter() >> + .any(|base_suite| suite_variant(suite).0 == *base_suite) > > ^ same > >> + { >> + add_info( >> + "warning".to_string(), >> + format!("suite '{}' should not be used in production!", suite), >> + ); >> + } >> + >> + if suite_variant(suite).0 == "stable" { >> + add_info( >> + "warning".to_string(), >> + "use the name of the stable distribution instead of 'stable'!".to_string(), >> + ); >> + } >> + } >> + } >> + } >> + >> + /// Checks if an official host is configured in the repository. >> + fn check_uris(&self) -> Option<(String, String)> { >> + let official_host = |domains: &Vec<&str>| match domains.split_last() { > > Drop this entire beast (see below), but as a review of it: > You can use the slice[1] & rest[2] pattern syntax here: > > #[allow(clippy::match_like_matches_macro)] > match domains[..] { // the `[..]` part is required here > [.., "proxmox", "com"] => true, > [.., "debian", "org"] => true, > _ => false, > } > > Or more concise (but I do find the above a bit quicker to glance over, > hence the 'clippy' hint ;-) ): > > matches!(domains[..], [.., "proxmox", "com"] | [.., "debian", "org"]); > > [1] https://doc.rust-lang.org/reference/patterns.html#slice-patterns > [2] https://doc.rust-lang.org/reference/patterns.html#rest-patterns > >> + Some((last, rest)) => match rest.split_last() { >> + Some((second_to_last, _rest)) => { >> + (*last == "org" && *second_to_last == "debian") >> + || (*last == "com" && *second_to_last == "proxmox") >> + } >> + None => false, >> + }, >> + None => false, >> + }; >> + >> + for uri in self.uris.iter() { >> + if let Some(host) = host_from_uri(uri) { >> + let domains = host.split('.').collect(); > > ^ But instead of building a vector here, why not just do: > > if host == "proxmox.com" || host.ends_with(".proxmox.com") > || host == "debian.org" || host.ends_with(".debian.org") > { > ... > } > Misses FQDNs? Thanks for the tips, I was not aware that one can do tail-matching with the matches! macro. >> + >> + if official_host(&domains) { >> + return Some(("badge".to_string(), "official host name".to_string())); >> + } >> + } >> + } >> + >> + None >> + } >> +} >> + >> +impl APTRepositoryFile { >> + /// Checks if old or unstable suites are configured and also that the >> + /// `stable` keyword is not used. >> + pub fn check_suites(&self) -> Vec<APTRepositoryInfo> { >> + let mut infos = vec![]; >> + >> + for (n, repo) in self.repositories.iter().enumerate() { >> + let mut add_info = |kind, message| { >> + infos.push(APTRepositoryInfo { >> + path: self.path.clone(), >> + number: n + 1, >> + kind, >> + message, >> + }) >> + }; >> + repo.check_suites(&mut add_info); > > ^ minor nit: > the `check_suites` you're calling here is only called at this one spot > and private, so personally I'd prefer an `impl FnMut` or generic over a > trait object, (also you could inline the closure here) > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [pbs-devel] [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function 2021-06-11 11:43 ` [pbs-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function Fabian Ebner 2021-06-17 8:39 ` [pbs-devel] [pve-devel] " Wolfgang Bumiller @ 2021-06-17 14:16 ` Fabian Grünbichler 1 sibling, 0 replies; 11+ messages in thread From: Fabian Grünbichler @ 2021-06-17 14:16 UTC (permalink / raw) To: pbs-devel, Proxmox VE development discussion On June 11, 2021 1:43 pm, Fabian Ebner wrote: > which checks for bad suites and official URIs. > > Signed-off-by: Fabian Ebner <f.ebner@proxmox.com> > --- > > Changes from v5: > * split out host_from_uri helper and also handle userinfo and port > * test an offical URI with port > * match all *.debian.org and *.proxmox.com as official to avoid (future) > false negatives. > * add bookworm and trixie codenames to the list of new_suites > > src/repositories/check.rs | 174 +++++++++++++++++++++- > src/repositories/mod.rs | 19 ++- > src/types.rs | 19 +++ > tests/repositories.rs | 97 +++++++++++- > tests/sources.list.d.expected/bad.sources | 30 ++++ > tests/sources.list.d/bad.sources | 29 ++++ > 6 files changed, 364 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 a682b69..585c28d 100644 > --- a/src/repositories/check.rs > +++ b/src/repositories/check.rs > @@ -1,6 +1,45 @@ > use anyhow::{bail, Error}; > > -use crate::types::{APTRepository, APTRepositoryFileType, APTRepositoryPackageType}; > +use crate::types::{ > + APTRepository, APTRepositoryFile, APTRepositoryFileType, APTRepositoryInfo, > + APTRepositoryPackageType, > +}; > + > +/// Splits the suite into its base part and variant. > +fn suite_variant(suite: &str) -> (&str, &str) { > + let variants = ["-backports-sloppy", "-backports", "-updates", "/updates"]; > + > + for variant in variants.iter() { > + if let Some(base) = suite.strip_suffix(variant) { > + return (base, variant); > + } > + } > + > + (suite, "") > +} > + > +/// Get the host part from a given URI. > +fn host_from_uri(uri: &str) -> Option<&str> { this has false-positives for file:// URIs. the way it is currently used, it might make sense to limit it to http(s)? > + if let Some(begin) = uri.find("://") { > + let mut host = uri.split_at(begin + 3).1; > + > + if let Some(end) = host.find('/') { > + host = host.split_at(end).0; > + } > + > + if let Some(begin) = host.find('@') { > + host = host.split_at(begin + 1).1; > + } > + > + if let Some(end) = host.find(':') { > + host = host.split_at(end).0; > + } > + > + return Some(host); > + } > + > + None > +} > > impl APTRepository { > /// Makes sure that all basic properties of a repository are present and > @@ -102,4 +141,137 @@ impl APTRepository { > false > } > } > + > + /// Checks if old or unstable suites are configured and also that the > + /// `stable` keyword is not used. > + fn check_suites(&self, add_info: &mut dyn FnMut(String, String)) { > + let old_suites = [ > + "lenny", > + "squeeze", > + "wheezy", > + "jessie", > + "stretch", > + "oldoldstable", > + "oldstable", > + ]; > + > + let next_suite = "bullseye"; > + > + let new_suites = [ > + "bookworm", > + "trixie", > + "testing", > + "unstable", > + "sid", > + "experimental", > + ]; > + > + 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_variant(suite).0 == *base_suite) > + { > + add_info( > + "warning".to_string(), > + format!("old suite '{}' configured!", suite), > + ); > + } > + > + if suite_variant(suite).0 == next_suite { > + add_info( > + "ignore-pre-upgrade-warning".to_string(), > + format!("suite '{}' should not be used in production!", suite), > + ); > + } > + > + if new_suites > + .iter() > + .any(|base_suite| suite_variant(suite).0 == *base_suite) > + { > + add_info( > + "warning".to_string(), > + format!("suite '{}' should not be used in production!", suite), > + ); > + } > + > + if suite_variant(suite).0 == "stable" { > + add_info( > + "warning".to_string(), > + "use the name of the stable distribution instead of 'stable'!".to_string(), > + ); > + } > + } > + } > + } > + > + /// Checks if an official host is configured in the repository. > + fn check_uris(&self) -> Option<(String, String)> { > + let official_host = |domains: &Vec<&str>| match domains.split_last() { > + Some((last, rest)) => match rest.split_last() { > + Some((second_to_last, _rest)) => { > + (*last == "org" && *second_to_last == "debian") > + || (*last == "com" && *second_to_last == "proxmox") > + } > + None => false, > + }, > + None => false, > + }; > + > + for uri in self.uris.iter() { > + if let Some(host) = host_from_uri(uri) { > + let domains = host.split('.').collect(); > + > + if official_host(&domains) { > + return Some(("badge".to_string(), "official host name".to_string())); > + } > + } > + } > + > + None > + } > +} > + > +impl APTRepositoryFile { > + /// Checks if old or unstable suites are configured and also that the > + /// `stable` keyword is not used. > + pub fn check_suites(&self) -> Vec<APTRepositoryInfo> { > + let mut infos = vec![]; > + > + for (n, repo) in self.repositories.iter().enumerate() { > + let mut add_info = |kind, message| { > + infos.push(APTRepositoryInfo { > + path: self.path.clone(), > + number: n + 1, > + kind, > + message, > + }) > + }; > + repo.check_suites(&mut add_info); > + } > + > + infos > + } > + > + /// Checks for official URIs. > + pub fn check_uris(&self) -> Vec<APTRepositoryInfo> { > + let mut infos = vec![]; > + > + for (n, repo) in self.repositories.iter().enumerate() { > + if let Some((kind, message)) = repo.check_uris() { > + infos.push(APTRepositoryInfo { > + path: self.path.clone(), > + number: n + 1, > + kind, > + message, > + }); > + } > + } > + > + infos > + } > } > diff --git a/src/repositories/mod.rs b/src/repositories/mod.rs > index b7919a9..c2bbc06 100644 > --- a/src/repositories/mod.rs > +++ b/src/repositories/mod.rs > @@ -4,7 +4,7 @@ use anyhow::{bail, format_err, Error}; > > use crate::types::{ > APTRepository, APTRepositoryFile, APTRepositoryFileError, APTRepositoryFileType, > - APTRepositoryOption, > + APTRepositoryInfo, APTRepositoryOption, > }; > > mod list_parser; > @@ -148,6 +148,23 @@ impl APTRepositoryFile { > } > } > > +/// Provides additional information about the repositories. > +/// > +/// The kind of information can be: > +/// `warnings` for bad suites. > +/// `ignore-pre-upgrade-warning` when the next stable suite is configured. > +/// `badge` for official URIs. > +pub fn check_repositories(files: &[APTRepositoryFile]) -> Vec<APTRepositoryInfo> { > + let mut infos = vec![]; > + > + for file in files.iter() { > + infos.append(&mut file.check_suites()); > + infos.append(&mut file.check_uris()); > + } > + > + infos > +} > + > /// Checks if the enterprise repository for the specified Proxmox product is > /// configured and enabled. > pub fn enterprise_repository_enabled(files: &[APTRepositoryFile], product: &str) -> bool { > diff --git a/src/types.rs b/src/types.rs > index bbd8e7e..057fffa 100644 > --- a/src/types.rs > +++ b/src/types.rs > @@ -244,3 +244,22 @@ impl std::error::Error for APTRepositoryFileError { > None > } > } > + > +#[api] > +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] > +#[serde(rename_all = "lowercase")] > +/// Additional information for a repository. > +pub struct APTRepositoryInfo { > + /// Path to the defining file. > + #[serde(skip_serializing_if = "String::is_empty")] > + pub path: String, > + > + /// Number of the associated respository within the file. > + pub number: usize, > + > + /// Info kind (e.g. "warning") > + pub kind: String, > + > + /// Info message > + pub message: String, > +} > diff --git a/tests/repositories.rs b/tests/repositories.rs > index ffb1888..9b0cd56 100644 > --- a/tests/repositories.rs > +++ b/tests/repositories.rs > @@ -3,9 +3,10 @@ use std::path::PathBuf; > use anyhow::{bail, format_err, Error}; > > use proxmox_apt::repositories::{ > - enterprise_repository_enabled, no_subscription_repository_enabled, write_repositories, > + check_repositories, enterprise_repository_enabled, no_subscription_repository_enabled, > + write_repositories, > }; > -use proxmox_apt::types::APTRepositoryFile; > +use proxmox_apt::types::{APTRepositoryFile, APTRepositoryInfo}; > > #[test] > fn test_parse_write() -> Result<(), Error> { > @@ -159,3 +160,95 @@ 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 absolute_suite_list = read_dir.join("absolute_suite.list"); > + let mut file = APTRepositoryFile::new(&absolute_suite_list)?.unwrap(); > + file.parse()?; > + > + let infos = check_repositories(&vec![file]); > + > + assert_eq!(infos.is_empty(), true); > + let pve_list = read_dir.join("pve.list"); > + let mut file = APTRepositoryFile::new(&pve_list)?.unwrap(); > + file.parse()?; > + > + let path_string = pve_list.into_os_string().into_string().unwrap(); > + > + let mut expected_infos = vec![]; > + for n in 1..=5 { > + expected_infos.push(APTRepositoryInfo { > + path: path_string.clone(), > + number: n, > + kind: "badge".to_string(), > + message: "official host name".to_string(), > + }); > + } > + > + let mut infos = check_repositories(&vec![file]); > + > + assert_eq!(infos.sort(), expected_infos.sort()); > + > + let bad_sources = read_dir.join("bad.sources"); > + let mut file = APTRepositoryFile::new(&bad_sources)?.unwrap(); > + file.parse()?; > + > + let path_string = bad_sources.into_os_string().into_string().unwrap(); > + > + let mut expected_infos = vec![ > + APTRepositoryInfo { > + path: path_string.clone(), > + number: 1, > + kind: "warning".to_string(), > + message: "suite 'sid' should not be used in production!".to_string(), > + }, > + APTRepositoryInfo { > + path: path_string.clone(), > + number: 2, > + kind: "warning".to_string(), > + message: "old suite 'lenny-backports' configured!".to_string(), > + }, > + APTRepositoryInfo { > + path: path_string.clone(), > + number: 3, > + kind: "warning".to_string(), > + message: "old suite 'stretch/updates' configured!".to_string(), > + }, > + APTRepositoryInfo { > + path: path_string.clone(), > + number: 4, > + kind: "warning".to_string(), > + message: "use the name of the stable distribution instead of 'stable'!".to_string(), > + }, > + APTRepositoryInfo { > + path: path_string.clone(), > + number: 5, > + kind: "ignore-pre-upgrade-warning".to_string(), > + message: "suite 'bullseye' should not be used in production!".to_string(), > + }, > + APTRepositoryInfo { > + path: path_string.clone(), > + number: 6, > + kind: "warning".to_string(), > + message: "suite 'testing' should not be used in production!".to_string(), > + }, > + ]; > + for n in 1..=6 { > + expected_infos.push(APTRepositoryInfo { > + path: path_string.clone(), > + number: n, > + kind: "badge".to_string(), > + message: "official URI".to_string(), > + }); > + } > + > + let mut infos = check_repositories(&vec![file]); > + > + assert_eq!(infos.sort(), expected_infos.sort()); > + > + 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..b630c89 > --- /dev/null > +++ b/tests/sources.list.d.expected/bad.sources > @@ -0,0 +1,30 @@ > +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:80 > +Suites: stretch/updates > +Components: main contrib > + > +Types: deb > +URIs: http://ftp.at.debian.org:80/debian > +Suites: stable > +Components: main > + > +Types: deb > +URIs: http://ftp.at.debian.org/debian > +Suites: bullseye > +Components: main > + > +Types: deb > +URIs: http://ftp.at.debian.org/debian > +Suites: testing > +Components: main > + > diff --git a/tests/sources.list.d/bad.sources b/tests/sources.list.d/bad.sources > new file mode 100644 > index 0000000..1aab2ea > --- /dev/null > +++ b/tests/sources.list.d/bad.sources > @@ -0,0 +1,29 @@ > +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:80 > +Suites: stretch/updates > +Components: main contrib > + > +Suites: stable > +URIs: http://ftp.at.debian.org:80/debian > +Components: main > +Types: deb > + > +Suites: bullseye > +URIs: http://ftp.at.debian.org/debian > +Components: main > +Types: deb > + > +Suites: testing > +URIs: http://ftp.at.debian.org/debian > +Components: main > +Types: deb > -- > 2.20.1 > > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-06-18 7:26 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-18 6:44 [pbs-devel] [pve-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function Wolfgang Bumiller 2021-06-18 6:53 ` Fabian Ebner -- strict thread matches above, loose matches on Subject: below -- 2021-06-18 7:17 Wolfgang Bumiller 2021-06-18 7:16 Wolfgang Bumiller 2021-06-18 7:26 ` Fabian Ebner 2021-06-18 6:56 Wolfgang Bumiller 2021-06-18 6:58 ` Fabian Ebner 2021-06-18 7:07 ` Fabian Ebner 2021-06-11 11:43 [pbs-devel] [PATCH-SERIES v6] APT repositories API/UI Fabian Ebner 2021-06-11 11:43 ` [pbs-devel] [PATCH v6 proxmox-apt 04/11] add check_repositories function Fabian Ebner 2021-06-17 8:39 ` [pbs-devel] [pve-devel] " Wolfgang Bumiller 2021-06-18 6:42 ` Fabian Ebner 2021-06-17 14:16 ` 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