provider: EG should not provision if `parametersRef` not found #5081

zirain · 2025-01-16T10:10:32Z

codecov · 2025-01-16T10:20:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.93%. Comparing base (126d507) to head (dba53d8).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5081      +/-   ##
==========================================
+ Coverage   66.82%   66.93%   +0.10%     
==========================================
  Files         210      210              
  Lines       32957    32945      -12     
==========================================
+ Hits        22024    22052      +28     
+ Misses       9601     9564      -37     
+ Partials     1332     1329       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

internal/infrastructure/kubernetes/infra_resource.go

zirain · 2025-01-16T15:22:17Z

tools/make/kube.mk

@@ -138,10 +138,10 @@ kube-demo-undeploy: ## Uninstall the Kubernetes resources installed from the `ma
 #	tools/hack/run-kube-local.sh

 .PHONY: conformance
-conformance: create-cluster kube-install-image kube-deploy run-conformance delete-cluster ## Create a kind cluster, deploy EG into it, run Gateway API conformance, and clean up.
+conformance: create-cluster kube-install-image kube-deploy install-eg-addons run-conformance delete-cluster ## Create a kind cluster, deploy EG into it, run Gateway API conformance, and clean up.


GC(envoy-gateway) refernece EnvoyProxy(proxy-config), that make service monitoring/otel-collector is required.

is this because we are using the same GC for conformance and e2e ? should we maintain 2 different GatewayClasses to keep each test suite more lightweight ?

yes, we can do that later.

internal/provider/kubernetes/controller.go

arkodg · 2025-01-16T22:24:01Z

internal/provider/kubernetes/status.go

@@ -568,8 +568,7 @@ func (r *gatewayAPIReconciler) updateStatusForGateway(ctx context.Context, gtw *
 		r.log.Info("failed to get Service for gateway",
 			"namespace", gtw.Namespace, "name", gtw.Name)
 	}
-	// update accepted condition
-	status.UpdateGatewayStatusAcceptedCondition(gtw, true)


curious why this was removed

I think we should respect the logic in controller.go.

this made gateway always accepted, even the ref EnvoyProxy is not found.

test/config/gatewayclass.yaml

internal/gatewayapi/listener.go

zirain · 2025-01-18T01:05:15Z

internal/provider/kubernetes/controller.go

 		}
+		// Set Gateway as accepted to passed test?
+		status.SetGatewayAccepted(&gtw, true, gwapiv1.GatewayReasonAccepted, "The Gateway has not been scheduled by Envoy Gateway")


@arkodg do you think we should remove this and update the test.
because now we reply the final result in translator.

Signed-off-by: zirain <[email protected]>

zirain · 2025-01-20T10:24:06Z

internal/gatewayapi/testdata/envoyproxy-accesslog-cel-with-invalid.out.yaml

      status: "False"
-      type: ListenersNotValid


this seems not right, GatewayConditionType only allow Accepted and Programmed.

zhaohuabing · 2025-01-21T07:08:20Z

internal/gatewayapi/listener.go

 			fmt.Sprintf("Invalid metrics backendRefs: %v", err))
 		return
 	}
+
+	// need explicit setting accepted status to true, as the status may have been set to false in previous loop,
+	// e.g. when access log backendRefs created later than the Gateway or EnvoyProxy.


This is unnecessary, as the Gateway status is always set to nil in every translation loop. Addtionally, setting Gatway Accepted status solely based on the listener status here can be confusing and should be avoided.

gateway/internal/provider/kubernetes/controller.go

Lines 1002 to 1009 in 126d507

// Discard Status to reduce memory consumption in watchable

// It will be recomputed by the gateway-api layer

gtw.Status = gwapiv1.GatewayStatus{}

if !resourceMap.allAssociatedGateways.Has(gtwNamespacedName) {

resourceMap.allAssociatedGateways.Insert(gtwNamespacedName)

resourceTree.Gateways = append(resourceTree.Gateways, &gtw)

}

}

this's necessary during the test.

zhaohuabing · 2025-01-21T07:14:44Z

internal/gatewayapi/status/gateway.go

+	if accepted {
+		status = metav1.ConditionTrue
+	}
+	cond := newCondition(string(gwapiv1.GatewayReasonAccepted), status, string(reason), message, time.Now(), gw.Generation)


Suggested change

cond := newCondition(string(gwapiv1.GatewayReasonAccepted), status, string(reason), message, time.Now(), gw.Generation)

cond := newCondition(string(gwapiv1.GatewayConditionAccepted), status, string(reason), message, time.Now(), gw.Generation)

zhaohuabing · 2025-01-21T07:24:17Z

internal/provider/kubernetes/controller.go

@@ -940,7 +940,13 @@ func (r *gatewayAPIReconciler) processGateways(ctx context.Context, managedGC *g

 		if err := r.processGatewayParamsRef(ctx, &gtw, resourceMap, resourceTree); err != nil {
 			r.log.Error(err, "failed to process infrastructure.parametersRef for gateway", "namespace", gtw.Namespace, "name", gtw.Name)
+			status.SetGatewayAccepted(&gtw, false, gwapiv1.GatewayReasonInvalidParameters, err.Error())
+			r.updateGatewayStatus(&gtw)


r.updateGatewayStatus is used to trigger a dummy Gateway Status update event to update the gateway IP address and can't be used for update Accepted status.

I suggest to add the Accepted status to the Gateway resource and pass it to the Gateway API translator to handle it.

zhaohuabing · 2025-01-21T07:31:41Z

@zirain I reponed #5117 as I pefer the approach to handle the EnvoyProxy error in that PR.

zirain requested a review from a team as a code owner January 16, 2025 10:10

zirain force-pushed the eg-daemonset branch 3 times, most recently from ec1bcd6 to 99ed84d Compare January 16, 2025 12:50

zirain changed the title ~~provider: EG should not provision if not found~~ provider: EG should not provision if parametersRef not found Jan 16, 2025

zirain force-pushed the eg-daemonset branch from 99ed84d to 0242241 Compare January 16, 2025 14:36

zirain commented Jan 16, 2025

View reviewed changes

internal/infrastructure/kubernetes/infra_resource.go Show resolved Hide resolved

zirain commented Jan 16, 2025

View reviewed changes

zirain force-pushed the eg-daemonset branch from 6306c93 to 8da4ce9 Compare January 16, 2025 15:23

zirain mentioned this pull request Jan 16, 2025

xds: move ready to dynamic listener #5079

Open

arkodg reviewed Jan 16, 2025

View reviewed changes

internal/provider/kubernetes/controller.go Outdated Show resolved Hide resolved

arkodg reviewed Jan 16, 2025

View reviewed changes

zirain force-pushed the eg-daemonset branch from 8da4ce9 to 29959e1 Compare January 16, 2025 23:34

zirain commented Jan 17, 2025

View reviewed changes

test/config/gatewayclass.yaml Outdated Show resolved Hide resolved

zirain force-pushed the eg-daemonset branch 4 times, most recently from 7aabde8 to 68ca8eb Compare January 17, 2025 09:22

arkodg reviewed Jan 17, 2025

View reviewed changes

internal/gatewayapi/listener.go Outdated Show resolved Hide resolved

zirain commented Jan 18, 2025

View reviewed changes

zirain force-pushed the eg-daemonset branch from 5cf46c7 to 26069e9 Compare January 18, 2025 01:17

zirain requested a review from arkodg January 18, 2025 01:56

zirain force-pushed the eg-daemonset branch from aa78303 to 81f3cd5 Compare January 18, 2025 03:12

zirain added 2 commits January 20, 2025 14:20

update e2e test

ac9077a

Signed-off-by: zirain <[email protected]>

skip if ref EnvoyProxy not found

dba53d8

Signed-off-by: zirain <[email protected]>

zirain force-pushed the eg-daemonset branch from 81f3cd5 to dba53d8 Compare January 20, 2025 06:27

zirain commented Jan 20, 2025

View reviewed changes

zirain mentioned this pull request Jan 21, 2025

fix the gateway accepted condition #5117

Merged

zhaohuabing reviewed Jan 21, 2025

View reviewed changes

zirain closed this Jan 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider: EG should not provision if `parametersRef` not found #5081

provider: EG should not provision if `parametersRef` not found #5081

zirain commented Jan 16, 2025

codecov bot commented Jan 16, 2025 •

edited

Loading

zirain Jan 16, 2025

arkodg Jan 16, 2025

zirain Jan 16, 2025

zirain Jan 18, 2025

arkodg Jan 16, 2025

zirain Jan 16, 2025

zirain Jan 17, 2025

zirain Jan 18, 2025

zirain Jan 20, 2025

zhaohuabing Jan 21, 2025 •

edited

Loading

zirain Jan 21, 2025

zhaohuabing Jan 21, 2025

zhaohuabing Jan 21, 2025

zhaohuabing commented Jan 21, 2025

	// Discard Status to reduce memory consumption in watchable
	// It will be recomputed by the gateway-api layer
	gtw.Status = gwapiv1.GatewayStatus{}
	if !resourceMap.allAssociatedGateways.Has(gtwNamespacedName) {
	resourceMap.allAssociatedGateways.Insert(gtwNamespacedName)
	resourceTree.Gateways = append(resourceTree.Gateways, &gtw)
	}
	}

	cond := newCondition(string(gwapiv1.GatewayReasonAccepted), status, string(reason), message, time.Now(), gw.Generation)
	cond := newCondition(string(gwapiv1.GatewayConditionAccepted), status, string(reason), message, time.Now(), gw.Generation)

provider: EG should not provision if parametersRef not found #5081

provider: EG should not provision if parametersRef not found #5081

Conversation

zirain commented Jan 16, 2025

codecov bot commented Jan 16, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaohuabing Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaohuabing commented Jan 21, 2025

provider: EG should not provision if `parametersRef` not found #5081

provider: EG should not provision if `parametersRef` not found #5081

codecov bot commented Jan 16, 2025 •

edited

Loading

zhaohuabing Jan 21, 2025 •

edited

Loading