feat: bump gwapi to v0.6.0 by Xunzhuo · Pull Request #853 · envoyproxy/gateway

Xunzhuo · 2023-01-04T07:11:32Z

Fixes: #839
Fixes: #716

Closes: #828

codecov-commenter · 2023-01-04T07:15:48Z

Codecov Report

❌ Patch coverage is 28.57143% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.00%. Comparing base (309566d) to head (f6a179e).
⚠️ Report is 4011 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/gatewayapi/contexts.go	0.00%	1 Missing and 1 partial ⚠️
internal/provider/kubernetes/controller.go	33.33%	2 Missing ⚠️
internal/status/status.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #853      +/-   ##
==========================================
- Coverage   64.06%   64.00%   -0.06%     
==========================================
  Files          50       51       +1     
  Lines        6486     6873     +387     
==========================================
+ Hits         4155     4399     +244     
- Misses       2075     2200     +125     
- Partials      256      274      +18

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

internal/provider/kubernetes/controller.go

arkodg · 2023-01-04T20:59:40Z

looks like the core issue is interpretation of mapping of Observed Generation to Status Conditions
The new GAPI tests assert that the new Observed Generation value be asserted on all Status Conditions (kubernetes-sigs/gateway-api#1586) but Envoy Gateway does not update the status conditions for all older conditions if the core fields (such as Status, Reason and Message) have not changed

gateway/internal/status/conditions.go

Line 127 in 085e6fa

func conditionChanged(a, b metav1.Condition) bool {

and

gateway/internal/gatewayapi/contexts.go

Line 484 in 085e6fa

// return early if the condition is unchanged

. Imho the latter approach seems more logical. Is there any spec governing this mapping @youngnick @skriss

sunjayBhatia · 2023-01-04T21:04:33Z

looks like the core issue is interpretation of mapping of Observed Generation to Status Conditions The new GAPI tests assert that the new Observed Generation value be asserted on all Status Conditions (kubernetes-sigs/gateway-api#1586) but Envoy Gateway does not update the status conditions for all older conditions if the core fields (such as Status, Reason and Message) have not changed

gateway/internal/status/conditions.go

Line 127 in 085e6fa

func conditionChanged(a, b metav1.Condition) bool {

and

gateway/internal/gatewayapi/contexts.go

Line 484 in 085e6fa

// return early if the condition is unchanged

. Imho the latter approach seems more logical. Is there any spec governing this mapping @youngnick @skriss

from the Condition spec: https://github.com/kubernetes/apimachinery/blob/6c409361e35e40e38c4056ba0b86647d4244c047/pkg/apis/meta/v1/types.go#L1480-L1485

even if the type/reason/status haven't changed, the observed generation should be updated to make clear the status condition applies to the latest version of the resource

arkodg · 2023-01-04T21:11:39Z

thanks for sharing that @sunjayBhatia ! , the spec makes it clear that the condition is considered out of date if the observed generation is older
hey @Xunzhuo if you can enhance these snippets (

gateway/internal/status/conditions.go

Line 127 in 085e6fa

func conditionChanged(a, b metav1.Condition) bool {

and

gateway/internal/gatewayapi/contexts.go

Line 484 in 085e6fa

// return early if the condition is unchanged

) with an extra check to consider a condition unchanged or changed if the observed generation is also unchanged or changed respectively, should fix the issue, thanks !

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

Xunzhuo · 2023-01-06T06:55:53Z

thanks for sharing that @sunjayBhatia ! , the spec makes it clear that the condition is considered out of date if the observed generation is older hey @Xunzhuo if you can enhance these snippets (

gateway/internal/status/conditions.go

Line 127 in 085e6fa

func conditionChanged(a, b metav1.Condition) bool {

and

gateway/internal/gatewayapi/contexts.go

Line 484 in 085e6fa

// return early if the condition is unchanged

) with an extra check to consider a condition unchanged or changed if the observed generation is also unchanged or changed respectively, should fix the issue, thanks !

Done.

internal/gatewayapi/contexts.go

arkodg · 2023-01-06T17:13:46Z

internal/provider/kubernetes/controller.go

 			gCopy := g.DeepCopy()
 			gCopy.Status.Conditions = status.MergeConditions(gCopy.Status.Conditions, gtw.Status.Conditions...)
+			for index := range gCopy.Status.Conditions {
+				gCopy.Status.Conditions[index].ObservedGeneration = gtw.Generation


this loop needs to be removed, this should happen automatically

After removing them, CI fails : (

arkodg · 2023-01-06T17:13:59Z

internal/provider/kubernetes/controller.go

 						gCopy := g.DeepCopy()
 						gCopy.Status.Listeners = val.Status.Listeners
+
+						for index := range gCopy.Status.Conditions {


this loop needs to be removed as well

arkodg · 2023-01-06T17:14:09Z

internal/provider/kubernetes/controller.go

 						hCopy := h.DeepCopy()
 						hCopy.Status.Parents = val.Status.Parents
+
+						for parentIndex, parent := range hCopy.Status.Parents {


same as above

arkodg · 2023-01-06T17:15:23Z

@Xunzhuo you will also need to edit this code to

func conditionChanged(a, b metav1.Condition) bool {
	return a.Status != b.Status || a.Reason != b.Reason || a.Message != b.Message || a.ObservedGeneration != b.ObservedGeneration
}

arkodg · 2023-01-06T18:06:37Z

@Xunzhuo you will also need to rm ObservedGeneration from here

gateway/internal/status/status.go

Line 164 in 97032be

    
           opts := cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime", "ObservedGeneration")

since it is now a comparable field

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

arkodg · 2023-01-09T21:36:47Z

hey @Xunzhuo looks like there was an issue where the gateway API status updater code path would try and update Status.Listeners without updating the ObservedGeneraton with Status.Conditions causing the test to fail. Ive updated the logic here #874

Xunzhuo requested a review from a team as a code owner January 4, 2023 07:11

Xunzhuo self-assigned this Jan 4, 2023

Xunzhuo added area/api API-related issues area/conformance Gateway API Conformance Related Issues labels Jan 4, 2023

Xunzhuo added this to the 0.3.0-rc.1 milestone Jan 4, 2023

arkodg reviewed Jan 4, 2023

View reviewed changes

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

arkodg mentioned this pull request Jan 4, 2023

Bump sigs.k8s.io/gateway-api from 0.6.0-rc2 to 0.6.0 #828

Closed

haq204 mentioned this pull request Jan 5, 2023

enable HTTPRoute method matching conformance test #659

Closed

Xunzhuo force-pushed the bump-gwapi-version branch from c50c1e1 to 3a0c333 Compare January 6, 2023 06:43

feat: bump gwapi to v0.6.0

d8fc059

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

Xunzhuo force-pushed the bump-gwapi-version branch from 3a0c333 to d8fc059 Compare January 6, 2023 06:55

Xunzhuo force-pushed the bump-gwapi-version branch from 74434ff to d8fc059 Compare January 6, 2023 08:08

arkodg reviewed Jan 6, 2023

View reviewed changes

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

arkodg reviewed Jan 6, 2023

View reviewed changes

update

f6a179e

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

Xunzhuo force-pushed the bump-gwapi-version branch from 1f4f5d0 to f6a179e Compare January 9, 2023 06:14

Xunzhuo closed this Jan 10, 2023

Conversation

Xunzhuo commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jan 4, 2023 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

arkodg commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sunjayBhatia commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arkodg commented Jan 4, 2023

Uh oh!

Xunzhuo commented Jan 6, 2023

Uh oh!

Uh oh!

arkodg Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

Xunzhuo Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

arkodg Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

arkodg Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

arkodg commented Jan 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arkodg commented Jan 6, 2023

Uh oh!

arkodg commented Jan 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Xunzhuo commented Jan 4, 2023 •

edited

Loading

codecov-commenter commented Jan 4, 2023 •

edited by codecov bot

Loading

arkodg commented Jan 4, 2023 •

edited

Loading

sunjayBhatia commented Jan 4, 2023 •

edited

Loading

arkodg commented Jan 6, 2023 •

edited

Loading