conformance: enable GatewayClassObservedGenerationBump

[Xunzhuo]

Fixes: #883

Signed-off-by: bitliu bitliu@tencent.com

[codecov-commenter]

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]

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]

@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]

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]

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]

@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]

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

[danehans]

@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]

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]

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]

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]

Waiting for gw api v0.6.1.

[arkodg]

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]

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

[arkodg]

🚀