Add a feature flag to match Kubernetes overwrite allow lists against the whole value
What does this MR do?
Adds the FF_KUBERNETES_ANCHOR_OVERWRITE_ALLOWED feature flag, off by default. With the flag on, the Kubernetes executor matches each *_overwrite_allowed setting that takes a regular expression (namespace_overwrite_allowed, service_account_overwrite_allowed, pod_labels_overwrite_allowed, pod_annotations_overwrite_allowed, node_selector_overwrite_allowed and node_tolerations_overwrite_allowed) against the whole value that a job requests, as if the expression were written ^(?:expression)$. With the flag off, nothing changes.
overwriteRegexCheckinexecutors/kubernetes/overwrites.gotakes whether to anchor the expression. It compiles the expression once, as written, and with the flag on accepts a value only when the leftmost-longest match spans all of it, which is what^(?:expression)$matches. An expression that does not compile is refused in both modes, even one such asa)|(bwhose anchored form would compile, and the error names the expression as it is configured. One that compiles only as written still works: wrapped in^(?:and)$, an unclosed\Qquote would quote the)$too. An alternation stays between both anchors: with the flag on,ci-builds|ci-reviewrefusesmy-ci-review.createOverwritespasses the flag to the namespace and service account checks and to the four map overwrites. The.+thatbearer_token_overwrite_allowedstands for is not a setting, so it is never anchored.- The executor reads the flag with
s.Build.IsFeatureFlagOn, like the other feature flags:populateFeatureFlagstakes it from[runners.feature_flags]when that sets it, and otherwise a job variable can turn it on. Anchoring only refuses more values, so a job can make its own matching stricter, never looser, and cannot turn off anchoring that the runner configuration turns on. If the default ever becomestrue, the read has to go back tos.Config.IsFeatureFlagOn, or a job could turn anchoring off; a comment at the read says so. make update_feature_flags_docsadds the flag's row, which says that[runners.feature_flags]turns it on for every job and that a job can otherwise turn it on for itself. The "Regular expressions in overwrite settings" section of the Kubernetes executor page gets a paragraph on the flag, including what it does to an alternation such as^ci-builds|ci-review$.
Why was this MR needed?
#40168 (closed) has the details. An unanchored expression matches anywhere in the value, so ci-.* also allows review-ci-apps, and kubernetes.io/arch=.* also allows beta.kubernetes.io/arch=amd64. !7661 (merged) documented that and pinned it in a test. Matching the whole value by default would change what existing configurations accept: the Buildah rootless tutorial's buildah-*, for example, would stop allowing buildah-sa.
This follows the path suggested in the review of !7661: a feature flag that keeps the current behaviour as the default, with TestOverwritesAllowedPatternMatching run in both modes. The first version read the flag from the runner configuration only; review moved it to the build's feature flags, for the reason in the third point above.
Flipping the default, and the deprecation note that should come before it, are not in this MR: they are for the team to decide and schedule. Until then, the rows of the settings table keep describing the default and link to the section that now describes the flag, and the descriptions in common/config.go name the flag.
What's the best way to test this MR?
go test -race -count=1 -run 'TestOverwritesAllowedPatternMatching|Test_overwriteRegexCheck|TestPrepareOverwritesAnchorFeatureFlagPrecedence' ./executors/kubernetes/TestOverwritesAllowedPatternMatchingruns each of its 23 cases, the 16 from !7661 (merged) and 7 new ones, with the flag off and on. With the flag on, the 7 cases where an unanchored expression matches only part of the value are refused (refusedWhenAnchored). Every other case has the same result in both modes, including an empty setting, which still disables the overwrite, and a bearer token that contains a newline, which.+accepts and its anchored form would refuse.Test_overwriteRegexCheckchecks the error text, an alternation with and without anchored branches, an alternation whose first branch is a prefix of the value, an inline flag such as(?i), an expression that is already anchored, an expression that compiles only in its anchored form, and an unclosed\Qquote, which compiles only as written.Test_overwriteRegexCheck_anchorMatchesWrappedExpression, added in review, compares the anchored check with^(?:expression)$over 7 expressions and 9 values.TestPrepareOverwritesAnchorFeatureFlagPrecedencegoes throughprepareOverwriteswith the flag set in[runners.feature_flags], in a job variable, or both, and checks that the runner configuration wins.
I wrote the tests first. With the flag passed through but not applied, the 7 refusedWhenAnchored cases fail with the flag on, as do 4 Test_overwriteRegexCheck cases and the 3 executor test cases where the flag ends up on. The test added in review fails too, in 21 of its 63 cases. With the flag read from the runner configuration only, as before review, the case where only a job variable sets it fails. I also checked each part of the change against a modified copy: compiling ^(?:expression)$ instead of the expression fails the case that compiles only when anchored and the 2 \Q cases, compiling the expression and then ^(?:expression)$ fails the 2 \Q cases, leftmost-first matching fails the alternation whose first branch is a prefix of the value, checking only where the match starts fails 3 cases and checking only where it ends fails 9, and anchoring the bearer token's .+ fails the bearer token case.
On main at 833abf92, with Go 1.26.10, I ran go build ./..., go vet on the touched packages (also with -tags 'integration kubernetes', and with GOOS=windows and GOOS=darwin), go test -race -count=1 on ./executors/kubernetes/..., ./helpers/featureflags/... and ./common/..., and make lint in the runner-linters:2.14.0-go1.26.10 image that the go:lint job uses (0 issues.). make update_feature_flags_docs leaves no diff. For the docs, I ran Vale at the error level, markdownlint-cli2, rumdl and lychee in the image that the docs:lint markdown job uses, all clean, and Vale at its default level reports nothing on the added lines. After review, I ran the same checks again on this branch, with the same results.
Not run: the Kubernetes integration tests, which need a cluster. The change is in the overwrite checks, which the unit tests call directly and through prepareOverwrites.
Test output (excerpt)
With the flag passed through but not applied, three of the failing cases:
--- FAIL: TestPrepareOverwritesAnchorFeatureFlagPrecedence (0.01s)
--- FAIL: TestPrepareOverwritesAnchorFeatureFlagPrecedence/flag_set_in_the_runner_configuration (0.00s)
--- FAIL: TestOverwritesAllowedPatternMatching (0.03s)
--- FAIL: TestOverwritesAllowedPatternMatching/namespace_pattern_matches_anywhere_in_the_value (0.00s)
--- FAIL: TestOverwritesAllowedPatternMatching/namespace_pattern_matches_anywhere_in_the_value/FF_KUBERNETES_ANCHOR_OVERWRITE_ALLOWED=true (0.00s)
--- FAIL: Test_overwriteRegexCheck (0.01s)
--- FAIL: Test_overwriteRegexCheck/anchored_alternation_is_matched_as_a_whole (0.00s)After review, the new executor test against the code before review, which read the flag from the runner configuration only:
--- FAIL: TestPrepareOverwritesAnchorFeatureFlagPrecedence (0.00s)
--- FAIL: TestPrepareOverwritesAnchorFeatureFlagPrecedence/flag_set_by_a_job_variable_only (0.00s)On this branch, all 153 test and subtest results of the four tests pass:
ok gitlab.com/gitlab-org/gitlab-runner/executors/kubernetes 2.684sEnvironment: Linux amd64, Go 1.26.10 (the version in go.mod).
What are the relevant issue numbers?
Closes #40168 (closed)