conformance: enable GatewayClassObservedGenerationBump by Xunzhuo · Pull Request #896 · envoyproxy/gateway

Xunzhuo · 2023-01-13T07:00:13Z

Fixes: #883

codecov-commenter · 2023-01-13T07:05:14Z

Codecov Report

Merging #896 (5e255a1) into main (545702c) will increase coverage by 0.26%.
The diff coverage is 73.38%.

@@            Coverage Diff             @@
##             main     #896      +/-   ##
==========================================
+ Coverage   63.48%   63.75%   +0.26%     
==========================================
  Files          53       54       +1     
  Lines        7476     7603     +127     
==========================================
+ Hits         4746     4847     +101     
- Misses       2431     2450      +19     
- Partials      299      306       +7

Impacted Files	Coverage Δ
internal/gatewayapi/resource.go	`54.83% <0.00%> (-1.83%)`	⬇️
internal/gatewayapi/zz_generated.deepcopy.go	`0.00% <0.00%> (ø)`
internal/status/gatewayclass.go	`0.00% <0.00%> (ø)`
internal/provider/kubernetes/controller.go	`49.18% <66.25%> (+2.05%)`	⬆️
api/config/v1alpha1/validation/envoyproxy.go	`90.00% <90.00%> (ø)`
internal/provider/kubernetes/helpers.go	`82.43% <100.00%> (+7.02%)`	⬆️
internal/status/conditions.go	`96.15% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

danehans · 2023-01-13T16:30:55Z

The failing GatewayClassObservedGenerationBump conformance test tries creating another GC with the same controller name and modifies its description to test the ObservedGeneration update correctly. EG only supports managing a single GC that manages its configured controller name. I don't think EG will pass this test without rearchitecting the system to support managing multiple GCs. Thoughts @youngnick @skriss @youngnick @AliceProxy @zirain @Xunzhuo?

arkodg · 2023-01-14T02:56:25Z

@danehans from this test https://github.com/kubernetes-sigs/gateway-api/blob/release-0.6/conformance/tests/gatewayclass-observed-generation-bump.go it looks like its updating the description of the same GatewayClass, not a new one (NamespacedName used is same)

Xunzhuo · 2023-01-14T03:33:23Z

The GC name is different, so I think it's created a new one. And tries to update the new one's description. But for EG, the new one is not accepted.

https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/tests/gatewayclass-observed-generation-bump.yaml

We are using this as the default GC and apply it at the first time. See make run-conformance target.

https://github.com/envoyproxy/gateway/blob/main/internal/provider/kubernetes/config/samples/gatewayclass.yaml

@arkodg

arkodg · 2023-01-14T05:15:16Z

ah thanks for the clarification @Xunzhuo
looking at #861, it updates status for GCs with matching controller name but not accepted, will that fix this test ?

Xunzhuo · 2023-01-16T02:35:09Z

@arkodg I might not, see: https://github.com/kubernetes-sigs/gateway-api/blob/release-0.6/conformance/tests/gatewayclass-observed-generation-bump.go#L65. It cannot process util GC is accepted.

I am having a try by adding @danehans `s commit.

Xunzhuo · 2023-01-16T02:53:06Z

See: https://kubernetes.slack.com/archives/CR0H13KGA/p1673599220709819

danehans · 2023-01-17T18:07:46Z

@Xunzhuo thanks for discussing this issue with the Gateway API community. I have confirmed that EG is handling observedGeneration properly for GCs with a matching controllerName. I performed the following to reproduce:

Install latest EG: kubectl apply -f https://github.com/envoyproxy/gateway/releases/download/latest/install.yaml
Install latest quickstart: kubectl apply -f https://github.com/envoyproxy/gateway/releases/download/latest/quickstart.yaml
Confirm quickstart resources are reporting the expected status conditions.

Create a 2nd GC, eg2, with a controllerName that matches EG:

apiVersion: gateway.networking.k8s.io/v1beta1
kind: GatewayClass
metadata:
  name: eg2
spec:
  controllerName: gateway.envoyproxy.io/gatewayclass-controller

Verify the 2nd GC reports the expected "Accepted=False" status condition and an observedGeneration that matches spec.generation, e.g. 1.

Update the 2nd GC to bump spec.generation:

apiVersion: gateway.networking.k8s.io/v1beta1
kind: GatewayClass
metadata:
  name: eg2
spec:
  controllerName: gateway.envoyproxy.io/gatewayclass-controller
  parametersRef:
    group: config.gateway.envoyproxy.io
    kind: EnvoyProxy
    namespace: envoy-gateway-system
    name: eg

Verify the 2nd GC, eg2, still surfaces the Accepted=False status condition and the observedGeneration matches spec.generation, e.g. 2.

Instead of changing the GC name as done by this PR, I suggest that we comment-out this test with a link to kubernetes-sigs/gateway-api#1650. Thoughts @arkodg @skriss @youngnick @AliceProxy @zirain ?

arkodg · 2023-01-17T18:17:35Z

yah @danehans I agree with commenting out this extended test (GatewayClassObservedGenerationBump) . In the initial design, the project decided to support a single accepted GatewayClass to not have to deal with multi tenancy (multiple accepted GatewayClass) within a single process (users should instead create a new Envoy Gateway controller).
The design can def be changed in the future, but imho we will incur more complexity and community cycles without adding much value prop if we take on this design change to make above test pass

arkodg · 2023-01-17T21:28:40Z

based on @youngnick 's suggestion in https://kubernetes.slack.com/archives/CR0H13KGA/p1673836949189379?thread_ts=1673599220.709819&cid=CR0H13KGA EG should be able to support the test if the conformance test is edited to enforce an Accepted condition rather than a Accpeted=True condition .

arkodg · 2023-01-18T18:07:30Z

based on @youngnick 's suggestion in https://kubernetes.slack.com/archives/CR0H13KGA/p1673836949189379?thread_ts=1673599220.709819&cid=CR0H13KGA EG should be able to support the test if the conformance test is edited to enforce an Accepted condition rather than a Accpeted=True condition .

raised kubernetes-sigs/gateway-api#1655 to address this.

danehans · 2023-02-07T22:21:56Z

Waiting for gw api v0.6.1.

arkodg · 2023-02-08T01:07:39Z

can confirm via #780 that this test has been fixed, we should use this PR to insert GatewayClassObservedGenerationBump into the list of conformance tests that need to un

danehans · 2023-02-08T20:03:54Z

@Xunzhuo now that gw api has been bumped to v0.6.1, can you please rebase and remove draft?

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

arkodg

🚀

Signed-off-by: bitliu <[email protected]> (cherry picked from commit b668002) Signed-off-by: Arko Dasgupta <[email protected]>

Signed-off-by: bitliu <[email protected]> (cherry picked from commit b668002) Signed-off-by: bitliu <[email protected]>

* fix: incorrect command in release schedule (#973) * chore: rename release-notes v0.3.0-rc1.yaml to v0.3.0-rc.1.yaml Signed-off-by: bitliu <[email protected]> * fix: incorrect command in release schedule Signed-off-by: bitliu <[email protected]> --------- Signed-off-by: bitliu <[email protected]> (cherry picked from commit 7639637) Signed-off-by: bitliu <[email protected]> * xds: Deprecated http2_protocol_options (#974) * xds: Deprecated http2_protocol_options Signed-off-by: hejianpeng <[email protected]> (cherry picked from commit 4a04915) Signed-off-by: bitliu <[email protected]> * Use HTTP1.1 to connect to upstream jwks endpoint (#977) Use HTTP1.1 to connec to upstream jwks endpoint Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit 82e7672) Signed-off-by: bitliu <[email protected]> * Add status in HTTPRoute when Ratelimit is disabled (#982) * Add status in HTTPRoute when Ratelimit is disabled Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit ebb902b) Signed-off-by: bitliu <[email protected]> * build(deps): bump actions/deploy-pages from 1.2.3 to 1.2.4 (#984) Bumps [actions/deploy-pages](https://github.com/actions/deploy-pages) from 1.2.3 to 1.2.4. - [Release notes](https://github.com/actions/deploy-pages/releases) - [Commits](actions/deploy-pages@v1.2.3...v1.2.4) --- updated-dependencies: - dependency-name: actions/deploy-pages dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit 1bea44e) Signed-off-by: bitliu <[email protected]> * build(deps): bump sigs.k8s.io/controller-runtime from 0.14.2 to 0.14.4 (#985) Bumps [sigs.k8s.io/controller-runtime](https://github.com/kubernetes-sigs/controller-runtime) from 0.14.2 to 0.14.4. - [Release notes](https://github.com/kubernetes-sigs/controller-runtime/releases) - [Changelog](https://github.com/kubernetes-sigs/controller-runtime/blob/master/RELEASE.md) - [Commits](kubernetes-sigs/controller-runtime@v0.14.2...v0.14.4) --- updated-dependencies: - dependency-name: sigs.k8s.io/controller-runtime dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit fadc142) Signed-off-by: bitliu <[email protected]> * fix function names for RL Infra (#976) The create and delete function names were incorrectly swapped. Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit 265c419) Signed-off-by: bitliu <[email protected]> * Add docs for GRPCRoute (#969) * Add docs for GRPCRoute Fixes: #642 Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit 3516d10) Signed-off-by: bitliu <[email protected]> * align wellknown package (#981) Signed-off-by: hejianpeng <[email protected]> (cherry picked from commit eec43d2) Signed-off-by: bitliu <[email protected]> * Plug in rate limit service URL into xds cluster (#983) * Plug in rate limit service URL into xds cluster Signed-off-by: Arko Dasgupta <[email protected]> * more guardrails Signed-off-by: Arko Dasgupta <[email protected]> * set to grpcPort and pin image Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit 1b26380) Signed-off-by: bitliu <[email protected]> * Adds JWT Authn User Docs (#991) Signed-off-by: danehans <[email protected]> (cherry picked from commit 2a431c1) Signed-off-by: bitliu <[email protected]> * user docs for global rate limit (#989) * user docs for global rate limit Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit ab47d2e) Signed-off-by: bitliu <[email protected]> * docs: add ref to global ratelimit (#999) Signed-off-by: bitliu <[email protected]> (cherry picked from commit 5115330) Signed-off-by: bitliu <[email protected]> * Update gateway-api to v0.6.1 (#1003) https://github.com/kubernetes-sigs/gateway-api/releases/tag/v0.6.1 Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit d01daec) Signed-off-by: bitliu <[email protected]> * Add GRPCRoute to SupportedKinds (#990) Fixes: #950 Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit beb6eae) Signed-off-by: bitliu <[email protected]> * Use `path_separated_prefix` Route match (#1004) * Use `path_separated_prefix` Route match We were using `prefix` Route match in xds for the `PathPrefix` Gateway API match defined here https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPPathMatch The match translates to a `path_separated_prefix` instead https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#config-route-v3-routematch This should fix the `HTTPRouteMatching` conformance test where requests with `/v2example` should match `/` and not `/v2` Fixes: #995 Signed-off-by: Arko Dasgupta <[email protected]> * rewrite empty match case Signed-off-by: Arko Dasgupta <[email protected]> * use prefix match for `/` Signed-off-by: Arko Dasgupta <[email protected]> --------- Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit c26d894) Signed-off-by: bitliu <[email protected]> * Adds API Docs Tooling (#980) Signed-off-by: danehans <[email protected]> (cherry picked from commit d5a5b25) Signed-off-by: bitliu <[email protected]> * Updates API godocs for improved markdown rendering (#1010) Updates API godocs for improved html rendering Signed-off-by: danehans <[email protected]> (cherry picked from commit b43574a) Signed-off-by: bitliu <[email protected]> * chore: bump testdata to gwapi v0.6.1 (#1011) Signed-off-by: bitliu <[email protected]> (cherry picked from commit f39b025) Signed-off-by: bitliu <[email protected]> * conformance: enable GatewayClassObservedGenerationBump (#896) Signed-off-by: bitliu <[email protected]> (cherry picked from commit b668002) Signed-off-by: bitliu <[email protected]> * Run all conformance tests except redirect tests (#1014) Redirect tests are failing due to a possible issue with the way upstream conformance tests have made assumptions Skipping them for now until below issues are resolved #992 #993 #994 Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit 38a6ddd) Signed-off-by: bitliu <[email protected]> * remove empty route error check in auth xds logic (#1019) the Xds translator should not error out when the IR routes are empty Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit 821b7d5) Signed-off-by: bitliu <[email protected]> * Removes GatewayObservedGenerationBump and HTTPRouteObservedGenerationBump Tests (#1021) (cherry picked from commit d016a66) Signed-off-by: bitliu <[email protected]> * Add Gateway API support doc (#1017) add gatewayapi support doc Signed-off-by: AliceProxy <[email protected]> (cherry picked from commit a33e627) Signed-off-by: bitliu <[email protected]> * Adds Cherry-Pick Steps to Release Doc (#1018) Signed-off-by: danehans <[email protected]> (cherry picked from commit 3d8120e) Signed-off-by: bitliu <[email protected]> * update to validate 1.26 (#1020) * update to validate 1.26 Signed-off-by: Held, Jarad <[email protected]> * correct the tag for kubernetes version for comformance tests Signed-off-by: Held, Jarad <[email protected]> --------- Signed-off-by: Held, Jarad <[email protected]> (cherry picked from commit 6bf8617) Signed-off-by: bitliu <[email protected]> * Bumps Compatibility Matrix for v0.3 (#1002) Signed-off-by: danehans <[email protected]> (cherry picked from commit d7a2e19) Signed-off-by: bitliu <[email protected]> * update gateway api support doc (#1022) * add note about not supported `filters` for `HTTPBackendRef` * rm `extensionRef` support for `GRPCRoute` * move the link under docs/latest Signed-off-by: Arko Dasgupta <[email protected]> (cherry picked from commit 9991359) Signed-off-by: bitliu <[email protected]> * release(v0.3.0): add release notes and release announcement (#1013) * release(v0.3.0): add release notes and release announcement Signed-off-by: bitliu <[email protected]> * update Signed-off-by: bitliu <[email protected]> --------- Signed-off-by: bitliu <[email protected]> (cherry picked from commit e55c15f) Signed-off-by: bitliu <[email protected]> * release: cut v0.3.0 versioned docs (#1025) Signed-off-by: bitliu <[email protected]> (cherry picked from commit 5b316b4) Signed-off-by: bitliu <[email protected]> --------- Signed-off-by: bitliu <[email protected]> Signed-off-by: hejianpeng <[email protected]> Signed-off-by: Arko Dasgupta <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: danehans <[email protected]> Signed-off-by: AliceProxy <[email protected]> Signed-off-by: Held, Jarad <[email protected]> Co-authored-by: zirain <[email protected]> Co-authored-by: Arko Dasgupta <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Daneyon Hansen <[email protected]> Co-authored-by: Alice Wasko <[email protected]> Co-authored-by: Jarad <[email protected]>

Xunzhuo force-pushed the enable-gc-og-bump branch from b63eb42 to 4b5153c Compare January 13, 2023 07:00

Xunzhuo force-pushed the enable-gc-og-bump branch from 36bbcb5 to 4b5153c Compare January 13, 2023 08:18

Xunzhuo force-pushed the enable-gc-og-bump branch 5 times, most recently from 4dcb1aa to 5e255a1 Compare January 16, 2023 02:29

Xunzhuo force-pushed the enable-gc-og-bump branch from 5e255a1 to a86ccdd Compare January 16, 2023 02:46

danehans added this to the 0.3.0 milestone Jan 19, 2023

danehans added the area/ci CI and build related issues label Jan 19, 2023

conformance: enable GatewayClassObservedGenerationBump

8bf0082

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

Xunzhuo force-pushed the enable-gc-og-bump branch from a86ccdd to 8bf0082 Compare February 9, 2023 03:37

Xunzhuo marked this pull request as ready for review February 9, 2023 03:38

Xunzhuo requested a review from a team as a code owner February 9, 2023 03:38

arkodg approved these changes Feb 9, 2023

View reviewed changes

Xunzhuo merged commit b668002 into envoyproxy:main Feb 9, 2023

Xunzhuo added a commit to Xunzhuo/gateway that referenced this pull request Feb 10, 2023

conformance: enable GatewayClassObservedGenerationBump (envoyproxy#896)

c881581

Signed-off-by: bitliu <[email protected]> (cherry picked from commit b668002) Signed-off-by: bitliu <[email protected]>

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

danehans commented Jan 13, 2023

Uh oh!

arkodg commented Jan 14, 2023

Uh oh!

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

Uh oh!

Uh oh!

arkodg commented Jan 14, 2023

Uh oh!

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

Uh oh!

Uh oh!

Xunzhuo commented Jan 16, 2023

Uh oh!

danehans commented Jan 17, 2023

Uh oh!

arkodg commented Jan 17, 2023

Uh oh!

arkodg commented Jan 17, 2023

Uh oh!

arkodg commented Jan 18, 2023

Uh oh!

danehans commented Feb 7, 2023

Uh oh!

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

Uh oh!

Uh oh!

danehans commented Feb 8, 2023

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Xunzhuo commented Jan 13, 2023 •

edited

Loading

codecov-commenter commented Jan 13, 2023 •

edited

Loading

Xunzhuo commented Jan 14, 2023 •

edited

Loading

Xunzhuo commented Jan 16, 2023 •

edited

Loading

arkodg commented Feb 8, 2023 •

edited

Loading