Gateway resources consolidation

[mlavacca]

What this PR does / why we need it:
This PR aims at consolidating and factorizing the gatewayClass and Gateway resources across all the integration tests, to remove duplicated code and improve code quality.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #2461

Special notes for your reviewer:
You can find the PR's change list below:

• All the explicit instantiation of GatewayClass and Gateway objects, followed by the respective GatewayV1alpha2().GatewayClasses().Create() and GatewayV1alpha2().Gateway().Create() function calls have been replaced by a helper call. These helpers are in the helpers_test.go file.
• Each function that needed a gatewayClass, created it at the beginning and deleted it at the end of the test (via a deferred function). Since about 90% of these objects were copypasta from one test function to another, a unique shared GatewayClass is now created at environment setup time; this object creation is in the suite_test.go file. Each needed GatewayClass different from the default one is created through the aforementioned helper.
• The files tlsroute_test.go, tcproute_test.go, httproute_test.go, and udproute_test.go used specific helpers to check the linking or unlinking of the routes from the Gateway. They were copypasta one from another. They have been deleted and factorized in a unique function with two additional parameters:
  • protocolType: to specify the protocol to check
  • ensureLinked: to specify whether to check the linking or the unlinking
• The cleanup in each test case has been made uniform by using the utility suggested in the comment below. NOTE: only the files under refactoring have been subjected to this change.
• Minor name refactoring throughout the touched files.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

• the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

[shaneutt]

While you're in here refactoring tests @mlavacca, I wanted to mention that in recent weeks KTF has gotten a new cluster cleanup utility:

https://github.com/Kong/kubernetes-testing-framework/blob/main/pkg/clusters/cleanup.go#L27

There are some examples of its use in the GWO:

• https://github.com/Kong/gateway-operator/blob/main/test/integration/gateway_test.go#L38
• https://github.com/Kong/gateway-operator/blob/main/test/integration/gateway_test.go#L51

Not saying this is required as part of this work you're doing so don't consider it a blocker by any stretch of the imagination, but since you're already in here doing some test cleanup if you would like to clean up some of the myriad defer functions which clean up individual items this could be employed to do so.

[pmalek]

Would it be too much to ask for If I'd suggest to put a summary of what were the goals of this refactoring? e.g. what were the functions/methods/structs/interfaces added/removed? 🙏

Given +650 −1,269 and the fact that it touches many files it's a bit hard to review reliably.

[mlavacca]

Would it be too much to ask for If I'd suggest to put a summary of what were the goals of this refactoring? e.g. what were the functions/methods/structs/interfaces added/removed? 🙏

Given +650 −1,269 and the fact that it touches many files it's a bit hard to review reliably.

@pmalek I updated the PR description to summarize the changes.

[pmalek]

Minor nits but 🎖️ overall

[pmalek]

🚢

[shaneutt]

Looking good, I really do appreciate the emphasis on cleaning up some of our test code, and I particularly like that we're switching over to the KTF cluster.Cleaner for cleanup because that saves a lot of space (don't forget to resolve #2618 if this does indeed update that for all cases).

I do have a few comments to share, take a look and let me know your thoughts.

Dismissing my own review to make sure it doesn't block the PR in my absence.

[mlavacca]

Looking good, I really do appreciate the emphasis on cleaning up some of our test code, and I particularly like that we're switching over to the KTF cluster.Cleaner for cleanup because that saves a lot of space (don't forget to resolve #2618 if this does indeed update that for all cases).

I used that cleaner only in the files touched by this refactoring. There are many other tests that still use the old cleaning approach.

[mlavacca]

@pmalek If you don't pass the variable testing.T to the function, you end up in a situation in which you are not able to log anything related to the test (unless we start using another logger): you just return false, and the callback is retried after a while. In this scenario, Whenever a GET fails, it's hard to spot the reason.

For what concern retrying a prior all the GETs, in a real environment I would definitely agree with you, but during tests, I'm not sure it is relevant. Given my thought, I have no problem in replacing the require.NoError(t, err) with t.Log() and return false afterward. But the point raised by Shane was to not pass the testing.T instance at all, and if we follow this approach, we are not able neither to break the test because of the error nor to log the error.

[pmalek]

@pmalek If you don't pass the variable testing.T to the function, you end up in a situation in which you are not able to log anything related to the test (unless we start using another logger): you just return false, and the callback is retried after a while. ...

... the point raised by Shane was to not pass the testing.T instance at all, and if we follow this approach, we are not able neither to break the test because of the error nor to log the error.

I somehow got fixated on the logging part of it and dismissed (in my head) Shane's concerns.

So, I do agree with what was brought up (w.r.t. passing testing.T) and having an unexpected assertion fail the test and abort it (from a distance), I also believe that not having relevant logs in case of a failure are a sever debugging hindrance.

(Perhaps there's a better way to approach this but) I'd suggest to do pass the testing.T to the helper, but use it mostly for logging and make the eventuallyGatewayIsLinked return bool so that it can be used in .Eventually().

If need be we can create an issue and revisit passing the t into that helper.

[pmalek]

OMG, instead of commenting I edited your comment @mlavacca 😠 sorry for that. Should be reverted already now.

[mlavacca]

(Perhaps there's a better way to approach this but) I'd suggest to do pass the testing.T to the helper, but use it mostly for logging and make the eventuallyGatewayIsLinked return bool so that it can be used in .Eventually().

If need be we can create an issue and revisit passing the t into that helper.

@pmalek I followed this approach: the testing.T instance is passed to the function only for logging purposes. Furthermore, I added a callback builder to follow Shane's idea. This concern should be completely addressed, I don't think we need to create a follow-up issue.

[pmalek]

One nit and I think this is good to get merged

[pmalek]

Ideally, I'd like Shane to resolve his reservations

[mlavacca]

Ideally, I'd like Shane to resolve his reservations

Ok, let's wait for Shane's feedback. I have put the do-not-merge label.
@shaneutt