Enhance Ensure_ functions from extensions/pkg/webhook package

[Kostov6]

How to categorize this PR?

/area quality
/kind enhancement

What this PR does / why we need it:

• EnsureNo_, EnsureStringWithPrefix, EnsureStringWithPrefixContains act on all matches
• rewrites utility functions from package extensions/pkg/webhook using slices library
• add test covering Ensure_ functions

Which issue(s) this PR fixes:
Fixes #8296

Special notes for your reviewer:

Release note:

There are several breaking changes in the `github.com/gardener/gardener/extensions/pkg/webhook` package:
- `EnsureNoStringWithPrefix`, `EnsureNoStringWithPrefixContains`, `EnsureNoEnvVarWithName`, `EnsureNoVolumeMountWithName`, `EnsureNoVolumeWithName`, `EnsureNoContainerWithName`, `EnsureNoPVCWithName` now delete all matching entries. Previously they were deleting only the first occurrence.
- `EnsureStringWithPrefix`, `EnsureStringWithPrefixContains` now act on all prefix matches.
- `StringIndex` is removed. instead, use `slices.Index`.

[Kostov6]

/cc @dimityrmirchev

[ialidzhikov]

/assign

[dimityrmirchev]

I left some initial comments. Overall there are a lot of breaking changes that I think we should not introduce.

[ary1992]

Thanks for the PR
We can simplify few functions to make it more readable.

[dimityrmirchev]

Two last notes from my side, other looks good:

1. make generate should be ran
2. please add a release note that StringIndex was removed and users should use slices.Index instead

[ialidzhikov]

Cosmetic suggestions, otherwise lgtm

Please:

1. Use the imperative, present tense for commit/PR messages:

Enhance `Ensure*` functions from `extensions/pkg/webhook` package

2. Improve the release note by enumerating which funcs have the changed behaviour (instead of the used EnsureNo_ thingy) and by adding a release note for the removed func (StringIndex).

[ialidzhikov]

A more real-world example (which is the motivation for #8296) would be:

Suggested change

	"--prefix1=key1=value1,key2=value2,key3=value3",
	"--prefix2=key4=value4,key5=value5,key6=value6",
	"--prefix1=key7=value7,key8=value8,key9=value9",
	flags = []string{
	"--prefix1-flag1=value1",
	"--flag2=value2",
	"--prefix1-flag3=value3",
	}

Idk why you put many key-value pairs as flag values. In the common case, flag supports a single value. You could make your flags everywhere single-value ones to make the tests easier to read.

[Kostov6]

Thanks for the better real-world test. The problem is that for EnsureNoStringWithPrefixContains and EnsureStringWithPrefixContains without a breaking change these scenarios can't be covered. We can:

1. Cover these cases by introducing breaking change to the implementation of EnsureNoStringWithPrefixContains and EnsureStringWithPrefixContains
2. Leave those cases and cover only

--prefix1=value1,value3
--prefix2=value2
--prefix1=value1,value4

by result := webhook.EnsureStringWithPrefixContains(flags, "--prefix1=", "value1", ",")

[ialidzhikov]

What is exactly the problem with these funcs?

They seem to behave as expected according to my testing:

diff --git a/extensions/pkg/webhook/utils_test.go b/extensions/pkg/webhook/utils_test.go
index 37d8f0b80..59d72234b 100644
--- a/extensions/pkg/webhook/utils_test.go
+++ b/extensions/pkg/webhook/utils_test.go
@@ -501,24 +501,24 @@ var _ = Describe("Utils", func() {
 
 			BeforeEach(func() {
 				flags = []string{
-					"--flag1=key1=value1,key2=value2,key3=value3",
-					"--flag2=key4=value4,key5=value5,key6=value6",
-					"--flag1=key7=value7,key8=value8,key9=value9",
+					"--prefix1-flag1=value1",
+					"--flag2=value2",
+					"--prefix1-flag3=value3",
 				}
 			})
 
 			It("should ensure the specified key-value pair is in all flags with a given prefix", func() {
-				result := webhook.EnsureStringWithPrefixContains(flags, "--flag1=", "key3=value3", ",")
+				result := webhook.EnsureStringWithPrefixContains(flags, "--prefix1", "value4", ",")
 				Expect(result).To(Equal([]string{
-					"--flag1=key1=value1,key2=value2,key3=value3",
-					"--flag2=key4=value4,key5=value5,key6=value6",
-					"--flag1=key7=value7,key8=value8,key9=value9,key3=value3",
+					"--prefix1-flag1=value1,value4",
+					"--flag2=value2",
+					"--prefix1-flag3=value3,value4",
 				}))
 			})
 
 			It("should add a new flag with the specified key-value pair if no flag with the given prefix exists", func() {
-				result := webhook.EnsureStringWithPrefixContains(flags, "--flag3=", "key10=value10", ",")
-				Expect(result).To(Equal(append(flags, "--flag3=key10=value10")))
+				result := webhook.EnsureStringWithPrefixContains(flags, "--flag3=", "value4", ",")
+				Expect(result).To(Equal(append(flags, "--flag3=value4")))
 			})
 		})
 	})
@@ -663,18 +663,18 @@ var _ = Describe("Utils", func() {
 
 			BeforeEach(func() {
 				flags = []string{
-					"--prefix1=key1=value1,key2=value2,key3=value3",
-					"--prefix2=key4=value4,key5=value5,key6=value6",
-					"--prefix1=key7=value7,key8=value8,key9=value9",
+					"--prefix1-flag1=value1,value2",
+					"--flag2=value2",
+					"--prefix1-flag3=value3",
 				}
 			})
 
 			It("should delete the specified key-value pair from all flags with a given prefix", func() {
-				result := webhook.EnsureNoStringWithPrefixContains(flags, "--prefix1=", "key2=value2", ",")
+				result := webhook.EnsureNoStringWithPrefixContains(flags, "--prefix1", "value2", ",")
 				Expect(result).To(Equal([]string{
-					"--prefix1=key1=value1,key3=value3",
-					"--prefix2=key4=value4,key5=value5,key6=value6",
-					"--prefix1=key7=value7,key8=value8,key9=value9",
+					"--prefix1-flag1=value1",
+					"--flag2=value2",
+					"--prefix1-flag3=value3",
 				}))
 			})
 		})

Am I missing something?

[Kostov6]

Currently those incorrect tests pass:

			It("should ensure the specified key-value pair is in all flags with a given prefix", func() {
				result := webhook.EnsureStringWithPrefixContains(flags, "--prefix1", "value1", ",")
				Expect(result).To(Equal([]string{
					"--prefix1-flag1=value1,value1",
					"--flag2=value2",
					"--prefix1-flag3=value3,value1",
				}))
			})

			It("should ensure the specified key-value pair is in all flags with a given prefix", func() {
				result := webhook.EnsureStringWithPrefixContains(flags, "--prefix1", "-flag1=value1", ",")
				Expect(result).To(Equal([]string{
					"--prefix1-flag1=value1",
					"--flag2=value2",
					"--prefix1-flag3=value3,-flag1=value1",
				}))
			})

Because of this logic on line 150

		values := strings.Split(strings.TrimPrefix(items[i], prefix), sep)

in these tests values is respectively:

["-flag1=value1"]
skipped because not prefixed with --prefix1
["-flag3=value3"]

[ialidzhikov]

I see. Then let's add tests for the flag prefix only to the funcs that support it. As shared above, such flags with common prefixes are the motivation for the issue this PR is fixing, hence it would be nice to cover these scenarios with unit tests. You init the flags in BeforeEach for every Describe, hence we could have different flags (with and without prefixes) for the different Describes.

[ialidzhikov]

We had a call with @Kostov6 and we agreed to:

1. Add a doc string to EnsureStringWithPrefixContains that it does not support cases where a flag prefix (not the whole flag) is passed
2. Adapt the tests for EnsureNoStringWithPrefix to include a test case for deletion by flag prefix
3. Randomize the used flags as test data - as shared above, right now we only have flags that have key-value pairs as flag value. Instead, let's have flags with single value, multiple values and key-value pairs.

[dimitar-kostadinov]

/lgtm

[gardener-prow]

LGTM label has been added.

Git tree hash: 73413987a0a094f06ed3db78626b7e6906a70a3a

[ary1992]

Minors Nits, otherwise lgtm !

[gardener-ci-robot]

The Gardener project currently lacks enough active contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:

• After 15d of inactivity, lifecycle/stale is applied
• After 15d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 7d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Mark this PR as rotten with /lifecycle rotten
• Close this PR with /close

/lifecycle stale

[ialidzhikov]

/remove-lifecycle stale

[ialidzhikov]

Minor suggestions otherwise lgtm

[gardener-prow]

@Kostov6: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-gardener-apidiff	44022c9	link	false	/test pull-gardener-apidiff

Full PR test history. Your PR dashboard. Command help for this repository.
Please help us cut down on flakes by linking this test failure to an open flake report or filing a new flake report if you can't find an existing one. Also see our testing guideline for how to avoid and hunt flakes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[ialidzhikov]

/lgtm

[gardener-prow]

LGTM label has been added.

Git tree hash: 43fbf52f4d43cfb43ae1c17bd3d19349b045e477

[gardener-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ialidzhikov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ialidzhikov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment