From 773ba45c7843b060215660fb4da8c0ea4782b485 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 24 Jul 2025 16:44:34 +0200 Subject: [PATCH 1/3] distro: make the regex for the `rhel7,fedora` stricter This commit makes the regexp for the rhel7 and fedora distro stricter. It also updates/moves the tests so that the stricter conditions are matched. Thanks to Thozza for suggesting this. --- pkg/distro/defs/distros.yaml | 4 ++-- pkg/distro/generic/fedora_test.go | 6 +----- pkg/distro/generic/rhel7_test.go | 8 +++++++- pkg/distro/generic/rhel8_test.go | 34 +++++++++++++++++++++++++++++++ pkg/distro/generic/rhel9_test.go | 34 +++++++++++++++++++++++++++++++ 5 files changed, 78 insertions(+), 8 deletions(-) diff --git a/pkg/distro/defs/distros.yaml b/pkg/distro/defs/distros.yaml index b9ce314cca..92942e0885 100644 --- a/pkg/distro/defs/distros.yaml +++ b/pkg/distro/defs/distros.yaml @@ -43,7 +43,7 @@ distros: - &fedora_stable <<: *fedora_rawhide name: "fedora-{{.MajorVersion}}" - match: 'fedora-[0-9][0-9]+' + match: 'fedora-[1-9][0-9]+' preview: false os_version: "{{.MajorVersion}}" release_version: "{{.MajorVersion}}" @@ -297,7 +297,7 @@ distros: - &rhel7 name: "rhel-{{.MajorVersion}}.{{.MinorVersion}}" - match: 'rhel-7\.[0-9]{1,2}' + match: 'rhel-7\.[0-9]' distro_like: rhel-7 product: "Red Hat Enterprise Linux" codename: "Maipo" diff --git a/pkg/distro/generic/fedora_test.go b/pkg/distro/generic/fedora_test.go index fafd1e158c..496bb4535f 100644 --- a/pkg/distro/generic/fedora_test.go +++ b/pkg/distro/generic/fedora_test.go @@ -965,11 +965,7 @@ func TestFedoraDistroFactory(t *testing.T) { expected: nil, }, { - strID: "rhel-9", - expected: nil, - }, - { - strID: "rhel-8.4.1", + strID: "fedora-043", expected: nil, }, } diff --git a/pkg/distro/generic/rhel7_test.go b/pkg/distro/generic/rhel7_test.go index 287610727f..40849ddf45 100644 --- a/pkg/distro/generic/rhel7_test.go +++ b/pkg/distro/generic/rhel7_test.go @@ -449,7 +449,13 @@ func TestRhel7DistroFactory(t *testing.T) { expected: nil, }, { - strID: "rhel-79", // this is intentionally not supported for el7 + // the latest RHEL-7 is 7.9 and there won't be any newer one + strID: "rhel-7.10", + expected: nil, + }, + { + // this is intentionally not supported for el7 + strID: "rhel-79", expected: nil, }, { diff --git a/pkg/distro/generic/rhel8_test.go b/pkg/distro/generic/rhel8_test.go index a66ddeddc7..b51f087117 100644 --- a/pkg/distro/generic/rhel8_test.go +++ b/pkg/distro/generic/rhel8_test.go @@ -983,3 +983,37 @@ func TestRH8_DiskCustomizationsCheckOptions(t *testing.T) { } } } + +func TestRhel8_DistroFactory(t *testing.T) { + type testCase struct { + strID string + expected distro.Distro + } + + testCases := []testCase{ + { + strID: "rhel-8.10", + expected: generic.DistroFactory("rhel-8.10"), + }, + { + strID: "rhel-8.4.1", + expected: nil, + }, + { + strID: "rhel-8", + expected: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.strID, func(t *testing.T) { + d := generic.DistroFactory(tc.strID) + if tc.expected == nil { + assert.Nil(t, d) + } else { + assert.NotNil(t, d) + assert.Equal(t, tc.expected.Name(), d.Name()) + } + }) + } +} diff --git a/pkg/distro/generic/rhel9_test.go b/pkg/distro/generic/rhel9_test.go index d5e1080432..175f2cc5bf 100644 --- a/pkg/distro/generic/rhel9_test.go +++ b/pkg/distro/generic/rhel9_test.go @@ -939,3 +939,37 @@ func TestRhel9_NoDiskCustomizationsNoError(t *testing.T) { } } } + +func TestRhel9_DistroFactory(t *testing.T) { + type testCase struct { + strID string + expected distro.Distro + } + + testCases := []testCase{ + { + strID: "rhel-9.6", + expected: generic.DistroFactory("rhel-9.6"), + }, + { + strID: "rhel-9.6.1", + expected: nil, + }, + { + strID: "rhel-9", + expected: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.strID, func(t *testing.T) { + d := generic.DistroFactory(tc.strID) + if tc.expected == nil { + assert.Nil(t, d) + } else { + assert.NotNil(t, d) + assert.Equal(t, tc.expected.Name(), d.Name()) + } + }) + } +} From d8f8a80253fc3169b47375b471e951df4f1138eb Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 24 Jul 2025 16:54:03 +0200 Subject: [PATCH 2/3] distro: make `matchAndNormalize` more robust and add test This commit adds a test for the matchAndNormalize() helper that checks for invalid/missing capture group names. This uncovered a crash in the code that is also fixed. Thanks to Thozza and Achilleas for suggesting this. --- pkg/distro/defs/id.go | 15 +++++++++++++++ pkg/distro/defs/id_test.go | 4 ++++ 2 files changed, 19 insertions(+) diff --git a/pkg/distro/defs/id.go b/pkg/distro/defs/id.go index 434a02f0a0..345e5d2e8c 100644 --- a/pkg/distro/defs/id.go +++ b/pkg/distro/defs/id.go @@ -7,6 +7,15 @@ import ( "github.com/osbuild/images/pkg/distro" ) +func validateSubexpMatch(re *regexp.Regexp, names ...string) error { + for _, name := range names { + if re.SubexpIndex(name) == -1 { + return fmt.Errorf("cannot find submatch field %q", name) + } + } + return nil +} + // matchAndNormalize() matches and normalizes the given nameVer // based on the reStr. On match it returns the normalized version // of the given nameVer. @@ -32,9 +41,15 @@ func matchAndNormalize(reStr, nameVer string) (string, error) { return "", fmt.Errorf("invalid number of submatches for %q %q (%v)", reStr, nameVer, len(l)) case 3: // distro only uses major ver and needs normalizing + if err := validateSubexpMatch(re, "name", "major"); err != nil { + return "", err + } return fmt.Sprintf("%s-%s", l[re.SubexpIndex("name")], l[re.SubexpIndex("major")]), nil case 4: // common case, major/minor and normalizing + if err := validateSubexpMatch(re, "name", "major", "minor"); err != nil { + return "", err + } return fmt.Sprintf("%s-%s.%s", l[re.SubexpIndex("name")], l[re.SubexpIndex("major")], l[re.SubexpIndex("minor")]), nil default: return "", fmt.Errorf("invalid number of submatches for %q %q (%v)", reStr, nameVer, len(l)) diff --git a/pkg/distro/defs/id_test.go b/pkg/distro/defs/id_test.go index d3cf10675a..4449053d2f 100644 --- a/pkg/distro/defs/id_test.go +++ b/pkg/distro/defs/id_test.go @@ -39,6 +39,10 @@ func TestMatchAndNormalizeSad(t *testing.T) { {`rhel-([0-9]+)`, "rhel-100", `invalid number of submatches for "rhel-([0-9]+)" "rhel-100" (2)`}, // too many capture groups {`(rhel)-([0-9])([0-9])([0-9])`, "rhel-100", `invalid number of submatches for "(rhel)-([0-9])([0-9])([0-9])" "rhel-100" (5)`}, + // capture groups have incorrect names + {`(?Pcentos)-(?P[0-9])stream`, "centos-9stream", `cannot find submatch field "name"`}, + {`(?Pcentos)-(?P[0-9])stream`, "centos-9stream", `cannot find submatch field "major"`}, + {`(?Prhel)-(?P8)\.?(?P[0-9]{1,2})`, "rhel-8.10", `cannot find submatch field "minor"`}, } { _, err := matchAndNormalize(tc.reStr, tc.nameVer) assert.ErrorContains(t, err, tc.expectedErr) From d1577e8bd7cb6368405b505daeb103ee58250c1e Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Thu, 24 Jul 2025 16:57:25 +0200 Subject: [PATCH 3/3] distro: simplify `matchAndNormalize()` Trivial change to drop the explicit handling of an incomplete case in `matchAndNormalize()`. It was added mostly for symetry but then the `default:` is already covering it and a comment is good enough to show that `case 2:` was considered. Thanks to Achilleas for suggesting this. --- pkg/distro/defs/id.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/distro/defs/id.go b/pkg/distro/defs/id.go index 345e5d2e8c..4800ee219b 100644 --- a/pkg/distro/defs/id.go +++ b/pkg/distro/defs/id.go @@ -36,9 +36,8 @@ func matchAndNormalize(reStr, nameVer string) (string, error) { case 1: // simple match, no named matching return nameVer, nil - case 2: - // incomplete match, user did not provide ,, - return "", fmt.Errorf("invalid number of submatches for %q %q (%v)", reStr, nameVer, len(l)) + // handling case 2: is not needed, its an incomplete match, we need at least name,major and + // captured by the "default" below case 3: // distro only uses major ver and needs normalizing if err := validateSubexpMatch(re, "name", "major"); err != nil {