Assert observedGeneration is incremented in Status.Conditions.

[dprotaso]

What type of PR is this?

/kind cleanup
/kind api-change

What this PR does / why we need it:
This PR ensures Gateway Resources and their conditions update observedGeneration as described in the GEP.

Current it tests

• Gateway
• GatewayClass
• HTTPRoute

Which issue(s) this PR fixes:

Fixes: #1584

Does this PR introduce a user-facing change?:

Conformance tests now assert observedGeneration is incremented in Status.Conditions (GatewayClass, Gateway, HTTPRoute)

[k8s-ci-robot]

Hi @dprotaso. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dprotaso]

/test pull-gateway-api-test
/test pull-gateway-api-verify

[k8s-ci-robot]

@dprotaso: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test pull-gateway-api-test
/test pull-gateway-api-verify

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dprotaso]

Also question: How come there are no existing conformance tests for GatewayClass mutations?

[robscott]

/ok-to-test

[robscott]

Also question: How come there are no existing conformance tests for GatewayClass mutations?

What would that look like? The conformance tests accept a GatewayClass to run tests against, but I can't think of anything meaningful to actually test on a GatewayClass other than that it is accepted.

[dprotaso]

but I can't think of anything meaningful to actually test on a GatewayClass other than that it is accepted.

Until now with ensuring observedGeneration gets bumped.

Thus changing the parameterRef or even description should trigger this

[dprotaso]

/test all

[dprotaso]

/test all

[youngnick]

The approach LGTM. This could go into v0.6.0, or a v0.6.1 update later if need be.

[youngnick]

This seems good to me, especially with the opt-out for the GatewayClass for now, so I'm going to hit the approve button. I'll leave it to one of the other approvers to give it the tick.

/approve

[shaneutt]

In terms of implementability, @pmalek @mlavacca and myself all talked about this and don't have any concerns for the multiple GWAPI implementations @ Kong 👍

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, shaneutt, youngnick

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• conformance/OWNERS [shaneutt,youngnick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[robscott]

Hey @dprotaso, thanks for all your work on this! The tests LGTM, can you squash this PR into a single commit? (See https://kubernetes.io/docs/contribute/new-content/open-a-pr/#squashing-commits for more info). Once that's done, ping any of us for an LGTM and we'll get it merged.

[dprotaso]

Weird isn’t your prow configured to squash on merge?

…

On Mon, Dec 19, 2022 at 16:37 Rob Scott ***@***.***> wrote: Hey @dprotaso <https://github.com/dprotaso>, thanks for all your work on this! The tests LGTM, can you squash this PR into a single commit? (See https://kubernetes.io/docs/contribute/new-content/open-a-pr/#squashing-commits for more info). Once that's done, ping any of us for an LGTM and we'll get it merged. — Reply to this email directly, view it on GitHub <#1586 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAERARXJ4BFYCJWUNZYLXDWODIRZANCNFSM6AAAAAASXLU5OQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[shaneutt]

Weird isn’t your prow configured to squash on merge?

Yes, but I think the suggestion here is since this PR is quite large we would like to give you the opportunity to squash into less commits, if not necessarily one commit. That is to say: if you feel it's important for there to be more than one commit to help keep the history more clear and trackable, do a rebase and squash to a history you feel is solid and we can merge that without simply squashing it all into one.

[dprotaso]

I have no preference - if you can squash and merge into a single commit it’ll save me the trouble of rebasing

…

On Mon, Dec 19, 2022 at 17:46 Shane Utt ***@***.***> wrote: Weird isn’t your prow configured to squash on merge? Yes, but I think the suggestion here is since this PR is quite large we would like to give you the opportunity to squash into *less* commits, if not necessarily *one* commit. That is to say: if you feel it's important for there to be more than one commit to help keep the history more clear and trackable, do a rebase and squash to a history you feel is solid and we can merge that *without* simply squashing it all into one. — Reply to this email directly, view it on GitHub <#1586 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAERATFHT4WDGYFSTPLYG3WODQUDANCNFSM6AAAAAASXLU5OQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[robscott]

/hold cancel
/lgtm

[candita]

@dprotaso I rebased to include this PR and I'm getting an error that wasn't present before. Is this an issue in the PR or in the implementation I'm testing?

   gateway-observed-generation-bump.go:73: 
                        Error Trace:    /home/cholman/code/gateway-api/conformance/gateway-observed-generation-bump.go:73
                        Error:          Received unexpected error:
                                        Operation cannot be fulfilled on gateways.gateway.networking.k8s.io "gateway-observed-generation-bump": the object has been modified; please apply your changes to the latest version and try again
                        Test:           TestConformance/GatewayObservedGenerationBump/observedGeneration_should_increment
                        Messages:       error updating the Gateway: Operation cannot be fulfilled on gateways.gateway.networking.k8s.io "gateway-observed-generation-bump": the object has been modified; please apply your changes to the latest version and try again

[dprotaso]

The test waits for all gateways to be Accepted prior to getting the Gateway and updating it.

The update can fail if the resource is modified after we fetch it but before we perform the update - this is just regular K8s semantics.

My intention of the test is that the gateway resource is stable prior to doing the Get/Update - it may make more sense that we wait on another condition rather than Accepted.

Can you confirm what the exact mutation is?