elliminate premature termination of DNS controller logic

[maksymvavilov]

What

If we specify the first listener in the GW spec to be the one without attached routes we never get to the second and never create DNS records for it (even if there are attached routes). This PR fixes it.
I'm not including integration test for this case - the suite has over 150 cases now and I do not consider this scenario to be critical for testing on each pull request

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.81%. Comparing base (ece13e8) to head (54cb7ce).
Report is 100 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #655      +/-   ##
==========================================
+ Coverage   80.20%   82.81%   +2.60%     
==========================================
  Files          64       72       +8     
  Lines        4492     5416     +924     
==========================================
+ Hits         3603     4485     +882     
- Misses        600      626      +26     
- Partials      289      305      +16     

Flag	Coverage Δ
integration	74.25% <100.00%> (+2.97%)	⬆️
unit	33.75% <0.00%> (+3.71%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
api/v1beta1 (u)	71.42% <ø> (ø)
api/v1beta2 (u)	93.58% <100.00%> (+2.16%)	⬆️
pkg/common (u)	83.78% <ø> (-5.05%)	⬇️
pkg/istio (u)	75.86% <ø> (+1.94%)	⬆️
pkg/log (u)	94.73% <ø> (ø)
pkg/reconcilers (u)	∅ <ø> (∅)
pkg/rlptools (u)	79.84% <ø> (+0.39%)	⬆️
controllers (i)	81.21% <83.62%> (+4.41%)	⬆️

Files	Coverage Δ
controllers/dnspolicy_dnsrecords.go	65.21% <100.00%> (-0.47%)	⬇️

... and 31 files with indirect coverage changes

[maksymvavilov]

For verification you could use this branch and run kustomize build test | k apply -f - to easily reproduce the bug

[mikenairn]

I'm not including integration test for this case - the suite has over 150 cases now and I do not consider this scenario to be critical for testing on each pull request

It seems fairly straightforward to force this error so not sure why we can't ensure we have it covered in our tests. It doesn't have to be an explicit test case if that is what you are concerned about, you can update the existing ones to include another listener with a status that has 0 AttachedRoutes.

https://github.com/Kuadrant/kuadrant-operator/blob/main/controllers/dnspolicy_controller_single_cluster_test.go#L50-L88

gateway = NewGatewayBuilder(TestGatewayName, gatewayClass.Name, testNamespace).
	WithHTTPListener("foo", "foo.example.com"). //<-- Listener with no attached routes
	WithHTTPListener(TestListenerNameOne, TestHostOne).
	WithHTTPListener(TestListenerNameWildcard, TestHostWildcard).
	Gateway
Expect(k8sClient.Create(ctx, gateway)).To(Succeed())

clusterHash = common.ToBase36HashLen(clusterUID, utils.ClusterIDLength)
ownerID = common.ToBase36HashLen(fmt.Sprintf("%s-%s-%s", clusterUID, gateway.Name, gateway.Namespace), utils.ClusterIDLength)

gwHash = common.ToBase36HashLen(gateway.Name+"-"+gateway.Namespace, 6)

//Set single cluster gateway status
Eventually(func() error {
	gateway.Status.Addresses = []gatewayapiv1.GatewayStatusAddress{
		{
			Type:  ptr.To(gatewayapiv1.IPAddressType),
			Value: TestIPAddressOne,
		},
		{
			Type:  ptr.To(gatewayapiv1.IPAddressType),
			Value: TestIPAddressTwo,
		},
	}
	gateway.Status.Listeners = []gatewayapiv1.ListenerStatus{
		{
			Name:           "foo",
			SupportedKinds: []gatewayapiv1.RouteGroupKind{},
			AttachedRoutes: 0,
			Conditions:     []metav1.Condition{},
		},
		{
			Name:           TestListenerNameOne,
			SupportedKinds: []gatewayapiv1.RouteGroupKind{},
			AttachedRoutes: 1,
			Conditions:     []metav1.Condition{},
		},
		{
			Name:           TestListenerNameWildcard,
			SupportedKinds: []gatewayapiv1.RouteGroupKind{},
			AttachedRoutes: 1,
			Conditions:     []metav1.Condition{},
		},
	}
	return k8sClient.Status().Update(ctx, gateway)
}, TestTimeoutMedium, TestRetryIntervalMedium).Should(Succeed())

This will fail without the fix.

[maksymvavilov]

I'm not including integration test for this case - the suite has over 150 cases now and I do not consider this scenario to be critical for testing on each pull request

It seems fairly straightforward to force this error so not sure why we can't ensure we have it covered in our tests. It doesn't have to be an explicit test case if that is what you are concerned about, you can update the existing ones to include another listener with a status that has 0 AttachedRoutes.

https://github.com/Kuadrant/kuadrant-operator/blob/main/controllers/dnspolicy_controller_single_cluster_test.go#L50-L88

gateway = NewGatewayBuilder(TestGatewayName, gatewayClass.Name, testNamespace).
	WithHTTPListener("foo", "foo.example.com"). //<-- Listener with no attached routes
	WithHTTPListener(TestListenerNameOne, TestHostOne).
	WithHTTPListener(TestListenerNameWildcard, TestHostWildcard).
	Gateway
Expect(k8sClient.Create(ctx, gateway)).To(Succeed())

clusterHash = common.ToBase36HashLen(clusterUID, utils.ClusterIDLength)
ownerID = common.ToBase36HashLen(fmt.Sprintf("%s-%s-%s", clusterUID, gateway.Name, gateway.Namespace), utils.ClusterIDLength)

gwHash = common.ToBase36HashLen(gateway.Name+"-"+gateway.Namespace, 6)

//Set single cluster gateway status
Eventually(func() error {
	gateway.Status.Addresses = []gatewayapiv1.GatewayStatusAddress{
		{
			Type:  ptr.To(gatewayapiv1.IPAddressType),
			Value: TestIPAddressOne,
		},
		{
			Type:  ptr.To(gatewayapiv1.IPAddressType),
			Value: TestIPAddressTwo,
		},
	}
	gateway.Status.Listeners = []gatewayapiv1.ListenerStatus{
		{
			Name:           "foo",
			SupportedKinds: []gatewayapiv1.RouteGroupKind{},
			AttachedRoutes: 0,
			Conditions:     []metav1.Condition{},
		},
		{
			Name:           TestListenerNameOne,
			SupportedKinds: []gatewayapiv1.RouteGroupKind{},
			AttachedRoutes: 1,
			Conditions:     []metav1.Condition{},
		},
		{
			Name:           TestListenerNameWildcard,
			SupportedKinds: []gatewayapiv1.RouteGroupKind{},
			AttachedRoutes: 1,
			Conditions:     []metav1.Condition{},
		},
	}
	return k8sClient.Status().Update(ctx, gateway)
}, TestTimeoutMedium, TestRetryIntervalMedium).Should(Succeed())

This will fail without the fix.

I was mostly thinking about explicitly testing for it, but this could work as well, i guess 🤔 added

[mikenairn]

Build Images job is failing due to this #580. We can ignore it on this PR.