Update Gateway/Listener status for consistency #1888

howardjohn · 2023-03-28T16:39:12Z

What type of PR is this?

/kind documentation

What this PR does / why we need it:

Supercedes #1886
Fixes #1832
Fixes #1877

This PR attempts to introduce a bit more consistency into Gateway conditions, while reducing churn. There are 4 changes here:

The "Ready" Gateway and Listener condition has been reserved for future use.

This condition has no implementations; while it may have some future value, we cannot justify the additional reason going GA without usage. Additionally, it has caused confusion by the subtle distinction between Programmed and Ready, and has "stolen" some useful Reasons to be not Programmed.

This is expected to have no user impact, as no implementation supports this.

The UnsupportedAddress Listener condition reason has been moved to a Gateway condition reason

This does not make sense on a Listener, as Listeners do not have addresses

The AddressNotAssigned Gateway condition reasons has moved from Accepted to Programmed

Accepted is about static analysis of the configuration; assigning an address is about actually programming the resources, so it fits more in Programmed

The NoResources Gateway condition reasons has moved from Ready to Programmed

Lack of resources is a valid reason to not program the data plane; with the reservation of Ready this is needed in Programmed.

Does this PR introduce a user-facing change?:

The "Ready" Gateway and Listener condition has been reserved for future use.
The UnsupportedAddress Listener condition reason has been moved to a Gateway condition reason
The AddressNotAssigned Gateway condition reasons has moved from Accepted to Programmed
The NoResources Gateway condition reasons has moved from Ready to Programmed

howardjohn · 2023-03-28T16:40:21Z

/retest

that is a new error for sure

howardjohn · 2023-03-28T16:49:07Z

/retest

apis/v1beta1/gateway_types.go

robscott

Thanks @howardjohn! Lots of great cleanup in here. Adding a hold because this feels like it should have several approvals before merging.

/hold

robscott · 2023-03-28T18:42:24Z

apis/v1beta1/gateway_types.go

-	// This reason is used with the "Ready" condition when one or
-	// more Listeners have an invalid or unsupported configuration
-	// and cannot be configured on the Gateway.
+	// Deprecated: Ready is reserved for future use


This feels like it should move to "Programmed"?

I mentioned this in #1832 but left it out since it seemed controversial.

I will add it if folks want

I liked the way you specified it in #1832 (comment). I think it could be fine as long as we have the "can be present when Accepted is True" caveat.

Added it back

robscott · 2023-03-28T18:58:43Z

apis/v1beta1/gateway_types.go

+	// "Ready" is a reserved condition type for future use. It should not be used by implementations.
+	// Note: its not really "deprecated", but rather "reserved"; however, deprecated triggers Go linters
+	// to alert about usage.


The rationale from the PR description here makes sense to me:

This condition has no implementations; while it may have some future value, we cannot justify the additional reason going GA without usage. Additionally, it has caused confusion by the subtle distinction between Programmed and Ready, and has "stolen" some useful Reasons to be not Programmed.

I think it would be helpful to provide a bit more context in the type definition though. Maybe describe that we want to reserve this term to represent that all configuration has fully propagated. Although we recognize that this is not possible for most implementations today, we are reserving this term for that purpose if it ever becomes broadly implementable.

I'm not really sure what the best mechanism is for this. I like the idea of a deprecation warning because that should show up in most linters. At some point in the future we may be able to move this out of type definitions altogether and just into something like implementation guidelines.

cc @liwenwu who mentioned they were using this condition in the call yesterday

AWS VPC Lattice Gateway Controller creates a AWS VPC lattice service network (SN) object for each K8S gateway object. After this VPC lattice service network(SN) is fully provisioned, the controller set the status as follow:

status: conditions: - lastTransitionTime: "2023-03-28T04:43:40Z" message: 'aws-gateway-arn: arn:aws:vpc-lattice:us-west-2:xxxxxxxx:servicenetwork/sn-06aa98be0c9c95af6' reason: Reconciled status: "True" type: Accepted

This means, all Pods inside the cluster(VPC) can access all HTTPRoutes behind the K8S Gateway. TLDR, through K8S Gateway and able to access all Lattice service(s) in that Lattice Service Network

Maybe describe that we want to reserve this term to represent that all configuration has fully propagated. Although we recognize that this is not possible for most implementations today, we are reserving this term for that purpose if it ever becomes broadly implementable.

This wording is pretty close, how about:

Suggested change

// "Ready" is a reserved condition type for future use. It should not be used by implementations.

// Note: its not really "deprecated", but rather "reserved"; however, deprecated triggers Go linters

// to alert about usage.

// "Ready" is a reserved condition type for future use. It should not be used by implementations.

// Note: This condition is not really "deprecated", but rather "reserved"; however, deprecated triggers Go linters

// to alert about usage.

//

// If used in the future, "Ready" will represent the final state where all configuration is confirmed good

// _and has completely propagated to the data plane_. That is, it is a _guarantee_ that, as soon as something

// sees the Condition as `true`, then connections will be correctly routed _immediately_.

//

// This is a very strong guarantee, and to date no implementation has satisfied it enough to implement it.

// This reservation can be discussed in the future if necessary.

Edit: You know you've been writing specs for too long when text like that comes out first go. Sigh.

I think these definitions of the two conditions "Programmed" and "Ready" are fine, but the names should be reversed. Intuitively, programmed would be this future "final confirmed routed connections" state. I would think most people would think "Programmed" means having routes configured (i.e., the gateway has been programmed), not just that the gateway is up and running with addresses that you can send requests to. I would call that state "Ready", since you could argue that the gateway is ready to accept requests, it's just not yet programmed (i.e., it doesn't know where to route them, so it will return 404s).

Thanks @youngnick , that looks good.

@frankbu that makes sense in isolation, however a bunch of other Kubernetes objects already use Ready to mean something like what we're using it for in this PR, so we're trying to keep the language consistent with other Kubernetes usage.

youngnick · 2023-03-29T05:17:32Z

LGTM, I'll leave someone else to add the official one though.

k8s-ci-robot · 2023-03-29T22:47:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: howardjohn, shaneutt

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:

~~OWNERS~~ [shaneutt]

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

robscott · 2023-03-30T15:11:13Z

Thanks @howardjohn!

/lgtm

robscott · 2023-03-30T15:20:41Z

Removing hold, we've got more than enough approvals on this.

/hold cancel

Updated as part of kubernetes-sigs/gateway-api#1888

Update Gateway/Listener status for consistency

1dc8c01

k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 28, 2023

k8s-ci-robot requested a review from shaneutt March 28, 2023 16:39

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 28, 2023

k8s-ci-robot requested a review from youngnick March 28, 2023 16:39

howardjohn mentioned this pull request Mar 28, 2023

Reserve the Ready condition for GW/Listener #1886

Closed

shaneutt reviewed Mar 28, 2023

View reviewed changes

apis/v1beta1/gateway_types.go Show resolved Hide resolved

shaneutt requested a review from robscott March 28, 2023 18:58

shaneutt added this to the v0.7.0 milestone Mar 28, 2023

robscott reviewed Mar 28, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 28, 2023

howardjohn added 2 commits March 29, 2023 07:48

add back ListenersNotValid

4e81374

Adopt Nick's language

abbe96c

shaneutt approved these changes Mar 29, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2023

k8s-ci-robot assigned robscott Mar 30, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 30, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 30, 2023

k8s-ci-robot merged commit 6b0c68a into kubernetes-sigs:main Mar 30, 2023

pleshakov mentioned this pull request Mar 31, 2023

Update reported conditions according to latest changes to Gateway API spec nginx/nginx-gateway-fabric#521

Closed

rainest added a commit to Kong/kubernetes-ingress-controller that referenced this pull request Jun 7, 2023

chore(tests) check new default Gateway condition

b1c429e

Updated as part of kubernetes-sigs/gateway-api#1888

rainest mentioned this pull request Jun 9, 2023

chore(deps) update to GWAPI 0.7.1 Kong/kubernetes-ingress-controller#4142

Merged

1 task

rainest added a commit to Kong/kubernetes-ingress-controller that referenced this pull request Jun 9, 2023

chore(tests) check new default Gateway condition

e272f71

Updated as part of kubernetes-sigs/gateway-api#1888

rainest added a commit to Kong/kubernetes-ingress-controller that referenced this pull request Jun 12, 2023

chore(tests) check new default Gateway condition

5806a13

Updated as part of kubernetes-sigs/gateway-api#1888

rainest added a commit to Kong/kubernetes-ingress-controller that referenced this pull request Jun 13, 2023

chore(tests) check new default Gateway condition

4cc97eb

Updated as part of kubernetes-sigs/gateway-api#1888

rainest added a commit to Kong/kubernetes-ingress-controller that referenced this pull request Jun 13, 2023

chore(tests) check new default Gateway condition

7dccaf0

Updated as part of kubernetes-sigs/gateway-api#1888

rainest added a commit to Kong/kubernetes-ingress-controller that referenced this pull request Jun 14, 2023

chore(tests) check new default Gateway condition

f62f940

Updated as part of kubernetes-sigs/gateway-api#1888

robscott mentioned this pull request Jun 23, 2023

Add conformance tests for GatewayConditionReady #1833

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Gateway/Listener status for consistency #1888

Update Gateway/Listener status for consistency #1888

howardjohn commented Mar 28, 2023

howardjohn commented Mar 28, 2023

howardjohn commented Mar 28, 2023

robscott left a comment

robscott Mar 28, 2023

howardjohn Mar 28, 2023

youngnick Mar 29, 2023

howardjohn Mar 29, 2023

robscott Mar 28, 2023

arkodg Mar 28, 2023

liwenwu-amazon Mar 29, 2023

youngnick Mar 29, 2023 •

edited

Loading

frankbu Mar 29, 2023

howardjohn Mar 29, 2023

youngnick Mar 29, 2023

youngnick commented Mar 29, 2023

k8s-ci-robot commented Mar 29, 2023

robscott commented Mar 30, 2023

robscott commented Mar 30, 2023

-	// "Ready" is a reserved condition type for future use. It should not be used by implementations.
-	// Note: its not really "deprecated", but rather "reserved"; however, deprecated triggers Go linters
-	// to alert about usage.
+	// "Ready" is a reserved condition type for future use. It should not be used by implementations.
+	// Note: This condition is not really "deprecated", but rather "reserved"; however, deprecated triggers Go linters
+	// to alert about usage.
+	//
+	// If used in the future, "Ready" will represent the final state where all configuration is confirmed good
+	// _and has completely propagated to the data plane_. That is, it is a _guarantee_ that, as soon as something
+	// sees the Condition as `true`, then connections will be correctly routed _immediately_.
+	//
+	// This is a very strong guarantee, and to date no implementation has satisfied it enough to implement it.
+	// This reservation can be discussed in the future if necessary.

Update Gateway/Listener status for consistency #1888

Update Gateway/Listener status for consistency #1888

Conversation

howardjohn commented Mar 28, 2023

howardjohn commented Mar 28, 2023

howardjohn commented Mar 28, 2023

robscott left a 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

youngnick Mar 29, 2023 • 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

youngnick commented Mar 29, 2023

k8s-ci-robot commented Mar 29, 2023

robscott commented Mar 30, 2023

robscott commented Mar 30, 2023

youngnick Mar 29, 2023 •

edited

Loading