Skip to content

harden OSTypeProber with regex list against version updates#300

Merged
majewsky merged 1 commit intomasterfrom
regexify_vmware_os_types
Mar 24, 2026
Merged

harden OSTypeProber with regex list against version updates#300
majewsky merged 1 commit intomasterfrom
regexify_vmware_os_types

Conversation

@wagnerd3
Copy link
Contributor

@wagnerd3 wagnerd3 commented Mar 9, 2026

We recently had the problem, that unknown vmware_ostype values are present on newer images, which makes the OSTypeProber render them invalid. This then causes wrong figures in billing.

In order to make this robust, we could go with regexes which replace the version numbers with placeholders. IMHO, this should be very safe, because a specific version number should not be an indicator to not bill an image. I copied the outdated list with specific entries to the test, so we make sure we don't loose any coverage.

cc @kc1r74p

@wagnerd3 wagnerd3 requested a review from a team as a code owner March 9, 2026 15:47
@wagnerd3 wagnerd3 force-pushed the regexify_vmware_os_types branch from cc570a4 to 559fefa Compare March 9, 2026 15:47
@wagnerd3 wagnerd3 force-pushed the regexify_vmware_os_types branch from 559fefa to 89ad1e9 Compare March 23, 2026 15:44
// We do not recommend utilizing this regex list for anything else other than
// validating the "vmware_ostype" attribute on existing images.

var isValidVMwareOSTypeRegex = map[regexpext.BoundedRegexp]bool{
Copy link
Contributor

@majewsky majewsky Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BoundedRegexp has a very limited cache for regex compilations. Putting this many regexes in there will make the cache perform very badly. Please compile the regexes explicitly, e.g.

var validVMWareOsTypeRegexes = []*regexp.Regexp{
  regexp.MustCompile(`^almalinux_64Guest$`),
  regexp.MustCompile(`^amazonlinux(\d+)_64Guest$`),
  ...
}

@wagnerd3 wagnerd3 force-pushed the regexify_vmware_os_types branch 2 times, most recently from e3226ad to d59d2ad Compare March 23, 2026 16:15
Copy link
Contributor

@majewsky majewsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can reproduce the CI failure. Do you have the latest golangci-lint version locally (2.11.4)?

@wagnerd3 wagnerd3 force-pushed the regexify_vmware_os_types branch from d59d2ad to 3804bc1 Compare March 24, 2026 09:33
@github-actions
Copy link

Merging this branch will decrease overall coverage

Impacted Packages Coverage Δ 🤖
github.com/sapcc/go-bits/liquidapi 18.26% (-0.10%) 👎

Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/sapcc/go-bits/liquidapi/constants.go 0.00% (ø) 0 0 0
github.com/sapcc/go-bits/liquidapi/ostype_prober.go 0.00% (ø) 1125 (+30) 0 1125 (+30)

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files

  • github.com/sapcc/go-bits/liquidapi/constants_test.go

@wagnerd3 wagnerd3 requested a review from majewsky March 24, 2026 09:37
@majewsky majewsky merged commit f029426 into master Mar 24, 2026
6 checks passed
@majewsky majewsky deleted the regexify_vmware_os_types branch March 24, 2026 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants