New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Hap Diff Test #1814

Open

ralikio wants to merge 5 commits into kyma-project:main from ralikio:features/corrected-hap-selection-test

Member

ralikio commented Mar 4, 2025 •

edited

Loading

Description

Changes proposed in this pull request:

Corrected HAP Migration test to guarantee that old and new implementations return the same results.

Related issue(s)


          Hap Diff Test

fc895e5

ralikio added the kind/enhancement label

ralikio self-assigned this

ralikio requested a review from a team as a code owner

March 4, 2025 14:49

kyma-gopher-bot enabled auto-merge (squash)

March 4, 2025 14:49

github-actions bot commented Mar 4, 2025

Add one of following labels

- kind/feature -> Use it when you want to submit a new feature

- kind/enhancement -> Use it when you modify or improve an existing feature

- kind/bug -> Use it when you fix a bug

kyma-bot added the size/XXL label

ralikio mentioned this pull request

KEB HAP Refactoring #1796

Open

jaroslaw-pieszka self-requested a review

March 4, 2025 14:56

jaroslaw-pieszka reviewed

View reviewed changes

internal/process/provisioning/cluster_regions.go

+              	"us-west1",
+              	"us-east4",
+              	"europe-west4",
+              }

Contributor

jaroslaw-pieszka Mar 4, 2025 •

edited

Loading

How come:

INPUT: PlanID: 4deee563-e5ec-4731-b9b1-53b42d855f0c, PlatformRegion: cf-ap21, ClusterRegion: centralus, Provider: AWS
!dirty,!euAccess,hyperscalerType=aws,tenantName=e8f7ec0a-0cd6-41f0-905d-5d1efa9fb6c4
INPUT: PlanID: 4deee563-e5ec-4731-b9b1-53b42d855f0c, PlatformRegion: cf-ap21, ClusterRegion: centralus, Provider: Azure
!dirty,!euAccess,hyperscalerType=azure,tenantName=e8f7ec0a-0cd6-41f0-905d-5d1efa9fb6c4
INPUT: PlanID: 4deee563-e5ec-4731-b9b1-53b42d855f0c, PlatformRegion: cf-ap21, ClusterRegion: centralus, Provider: GCP
!dirty,!euAccess,hyperscalerType=gcp,tenantName=e8f7ec0a-0cd6-41f0-905d-5d1efa9fb6c4

centralus is only valid for azure provider (and related plans).
So we test not allowed combinations of plan and cluster region.

Contributor

jaroslaw-pieszka Mar 4, 2025

This is not issue but could be.
If we have difference for not allowed combinations, we report failure of comparison however it is not necessarily a problem.

Contributor

jaroslaw-pieszka Mar 4, 2025 •

edited

Loading

And here we generate over 50k cases instead of around 3k.

Member Author

ralikio Mar 6, 2025

It was easier for just to go through all combinations. Test will be removed with removal of old resolve_creds steps.

jaroslaw-pieszka reviewed

View reviewed changes

internal/process/provisioning/resolve_creds_hap_selection_migration_test.go Outdated

+              	var newImplementationLog *bufio.Writer
+              	if writeFiles {
+              		oldLog, err := os.OpenFile("./hap-old-implementation.log", os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0755)

Contributor

jaroslaw-pieszka Mar 4, 2025 •

edited

Loading

executable? I think we need to reconsider permissions.

Member Author

ralikio Mar 6, 2025

ralikio@d15d50f

jaroslaw-pieszka reviewed

View reviewed changes

internal/process/provisioning/resolve_creds_hap_selection_migration_test.go Outdated


		oldImplementationLog = bufio.NewWriter(oldLog)

		newLog, err := os.OpenFile("./hap-new-implementation.log", os.O_RDWR\|os.O_CREATE\|os.O_TRUNC, 0755)

Contributor

jaroslaw-pieszka Mar 4, 2025

executable? Is this really needed?

Member Author

ralikio Mar 6, 2025

ralikio@8b3ca99

jaroslaw-pieszka reviewed

View reviewed changes

internal/process/provisioning/resolve_creds_hap_selection_migration_test.go

+              					require.Zero(t, when)
+              					for label, count := range queriesCountMap {
+              						require.NotZero(t, count, "first invocation produced wrong number of queries for label: %s, count: %d", label, count)

Contributor

jaroslaw-pieszka Mar 4, 2025 •

edited

Loading

If count > 0 regardless of the actual value we are fine?
What if count > 1?

Contributor

jaroslaw-pieszka Mar 4, 2025

What if implementation wrongly requests twice the same (or different) secret bindings?
Should we detect it?

jaroslaw-pieszka reviewed

View reviewed changes

internal/process/provisioning/resolve_creds_hap_selection_migration_test.go Outdated Show resolved Hide resolved

jaroslaw-pieszka reviewed

View reviewed changes

internal/process/provisioning/resolve_creds_hap_selection_migration_test.go Outdated Show resolved Hide resolved

jaroslaw-pieszka reviewed

View reviewed changes

internal/process/provisioning/resolve_creds_hap_selection_migration_test.go

+              )
+              // To generate output hap-old-implementation files (used to compare results by hand) switch writeFiles to true
+              func TestResolveCredentials_IntegrationAzure2(t *testing.T) {

Contributor

jaroslaw-pieszka Mar 4, 2025

Could you consider changing the name of function?

jaroslaw-pieszka reviewed

View reviewed changes

internal/process/provisioning/resolve_creds_hap_selection_migration_test.go

+              	increase := true
+              	gc.PrependReactor("list", gardener.SecretBindingResource.Resource, func(action k8sTesting.Action) (bool, runtime.Object, error) {
+              		originalLabel := action.(k8sTesting.ListActionImpl).GetListRestrictions().Labels.String()

Contributor

jaroslaw-pieszka Mar 4, 2025

These are labels (as GetListRestrictions().Labels suggests).

jaroslaw-pieszka reviewed

View reviewed changes

internal/process/provisioning/resolve_creds_hap_selection_migration_test.go

+              					// first run
+              					increase = true
+              					_, when, err := step.Run(op, fixLogger())

Contributor

jaroslaw-pieszka Mar 4, 2025 •

edited

Loading

So here we have reactor called and queriesCountMap filled.
Comparison is based on assumption that map key will be the same (GetListRestrictions().Labels.String()).
What if order is not stable?
What if there is irrelevant label added?
Shouldn't we compare explicitely shared, euaccess, hyperscalertype labels?
What if tenant change?

jaroslaw-pieszka reviewed

View reviewed changes

internal/process/provisioning/resolve_creds_hap_selection_migration_test.go

+              			// for example !dirty selects a secret without dirty label
+              			filteredSplit := []string{}
+              			split := strings.Split(originalLabel, ",")
+              			for _, label := range split {

Contributor

jaroslaw-pieszka Mar 4, 2025

This part is needed only to fix appropriate SecretBinding.
So could be extracted as it has different responsibility.

jaroslaw-pieszka reviewed

View reviewed changes

internal/process/provisioning/resolve_creds_hap_selection_migration_test.go

+              			split := strings.Split(originalLabel, ",")
+              			for _, label := range split {
+              				label = strings.Trim(label, " ")
+              				if label != "" && label != " " && label != "!dirty" && label != "!euAccess" && label != "shared!=true" && label != "!tenantName" {

Contributor

jaroslaw-pieszka Mar 4, 2025

So we omit "!euAccess".

jaroslaw-pieszka reviewed

View reviewed changes

internal/process/provisioning/resolve_creds_hap_selection_migration_test.go

+              			split := strings.Split(originalLabel, ",")
+              			for _, label := range split {
+              				label = strings.Trim(label, " ")
+              				if label != "" && label != " " && label != "!dirty" && label != "!euAccess" && label != "shared!=true" && label != "!tenantName" {

Contributor

jaroslaw-pieszka Mar 4, 2025

Could "shared!=true" actually happen?

jaroslaw-pieszka reviewed

View reviewed changes

internal/process/provisioning/resolve_creds_hap_selection_migration_test.go Show resolved Hide resolved

ralikio and others added 4 commits

March 6, 2025 09:57


          Review Remarks

d15d50f


          Review Remarks

8b3ca99


          Update internal/process/provisioning/resolve_creds_hap_selection_migr…

a22f068

…ation_test.go

Co-authored-by: Jarosław Pieszka <[email protected]>


          Update internal/process/provisioning/resolve_creds_hap_selection_migr…

a273e5d

…ation_test.go

Co-authored-by: Jarosław Pieszka <[email protected]>

ralikio assigned jaroslaw-pieszka and piotrmiskiewicz and unassigned ralikio

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement size/XXL