Skip to content

Update Gateway/Listener status for consistency #1888

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 30, 2023
Merged

Update Gateway/Listener status for consistency #1888

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1dc8c01
Update Gateway/Listener status for consistency
howardjohn Mar 28, 2023
4e81374
add back ListenersNotValid
howardjohn Mar 29, 2023
abbe96c
Adopt Nick's language
howardjohn Mar 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 54 additions & 86 deletions apis/v1beta1/gateway_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,8 @@ const (
//
// * "Invalid"
// * "Pending"
// * "NoResources"
// * "AddressNotAssigned"
//
// Possible reasons for this condition to be Unknown are:
//
Expand All @@ -550,9 +552,20 @@ const (
// true.
GatewayReasonProgrammed GatewayConditionReason = "Programmed"

// This reason is used with the "Programmed" condition when the Listener is
// This reason is used with the "Programmed" and "Accepted" condition when the Gateway is
// syntactically or semantically invalid.
GatewayReasonInvalid GatewayConditionReason = "Invalid"

// This reason is used with the "Programmed" condition when the
// Gateway is not scheduled because insufficient infrastructure
// resources are available.
GatewayReasonNoResources GatewayConditionReason = "NoResources"

// This reason is used with the "Programmed" condition when none of the requested
// addresses have been assigned to the Gateway. This reason can be used to
// express a range of circumstances, including (but not limited to) IPAM
// address exhaustion, address not yet allocated, or a named address not being found.
GatewayReasonAddressNotAssigned GatewayConditionReason = "AddressNotAssigned"
)

const (
Expand All @@ -564,11 +577,13 @@ const (
// Possible reasons for this condition to be True are:
//
// * "Accepted"
// * "ListenersNotValid"
//
// Possible reasons for this condition to be False are:
//
// * "Invalid"
// * "NotReconciled"
// * "NoResources"
// * "UnsupportedAddress"
//
// Possible reasons for this condition to be Unknown are:
//
Expand All @@ -579,76 +594,54 @@ const (
// interoperability.
GatewayConditionAccepted GatewayConditionType = "Accepted"

// Deprecated: use "Accepted" instead.
GatewayConditionScheduled GatewayConditionType = "Scheduled"

// This reason is used with the "Accepted" condition when the condition is
// True.
GatewayReasonAccepted GatewayConditionReason = "Accepted"

// This reason is used with the "Accepted" and "Programmed"
// conditions when the status is "Unknown" and no controller has reconciled
// the Gateway.
GatewayReasonPending GatewayConditionReason = "Pending"

// This reason is used with the "Accepted" condition when the Gateway could not be configured
// because the requested address is not supported. This reason could be used in a number of
// instances, including:
//
// * The address is already in use.
// * The type of address is not supported by the implementation.
GatewaReasonUnsupportedAddress GatewayConditionReason = "UnsupportedAddress"
)

const (
// Deprecated: use "Accepted" instead.
GatewayConditionScheduled GatewayConditionType = "Scheduled"

// This reason is used with the "Scheduled" condition when the condition is
// True.
//
// Deprecated: use the "Accepted" condition with reason "Accepted" instead.
GatewayReasonScheduled GatewayConditionReason = "Scheduled"

// This reason is used with the "Accepted", "Programmed" and "Ready"
// conditions when the status is "Unknown" and no controller has reconciled
// the Gateway.
GatewayReasonPending GatewayConditionReason = "Pending"

// Deprecated: Use "Pending" instead.
GatewayReasonNotReconciled GatewayConditionReason = "NotReconciled"

// This reason is used with the "Accepted" condition when the
// Gateway is not scheduled because insufficient infrastructure
// resources are available.
GatewayReasonNoResources GatewayConditionReason = "NoResources"
)

const (
// Ready is an optional Condition that has Extended support. When it's set,
// the condition indicates whether the Gateway has been completely configured
// and traffic is ready to flow through the data plane immediately.
//
// If both the "ListenersNotValid" and "ListenersNotReady"
// reasons are true, the Gateway controller should prefer the
// "ListenersNotValid" reason.
//
// Possible reasons for this condition to be true are:
// "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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@youngnick youngnick Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @youngnick , that looks good.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

//
// * "Ready"
//
// Possible reasons for this condition to be False are:
//
// * "ListenersNotValid"
// * "ListenersNotReady"
// * "AddressNotAssigned"
//
// Controllers may raise this condition with other reasons,
// but should prefer to use the reasons listed above to improve
// interoperability.
// Deprecated: Ready is reserved for future use
GatewayConditionReady GatewayConditionType = "Ready"

// This reason is used with the "Ready" condition when the condition is
// true.
// Deprecated: Ready is reserved for future use
GatewayReasonReady GatewayConditionReason = "Ready"

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like it should move to "Programmed"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I will add it if folks want

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added it back

GatewayReasonListenersNotValid GatewayConditionReason = "ListenersNotValid"

// This reason is used with the "Ready" condition when one or
// more Listeners are not ready to serve traffic.
// Deprecated: Ready is reserved for future use
GatewayReasonListenersNotReady GatewayConditionReason = "ListenersNotReady"

// This reason is used with the "Ready" condition when none of the requested
// addresses have been assigned to the Gateway. This reason can be used to
// express a range of circumstances, including (but not limited to) IPAM
// address exhaustion, invalid or unsupported address requests, or a named
// address not being found.
GatewayReasonAddressNotAssigned GatewayConditionReason = "AddressNotAssigned"
)

// ListenerStatus is the status associated with a Listener.
Expand Down Expand Up @@ -745,7 +738,6 @@ const (
//
// * "PortUnavailable"
// * "UnsupportedProtocol"
// * "UnsupportedAddress"
//
// Possible reasons for this condition to be Unknown are:
//
Expand Down Expand Up @@ -781,14 +773,6 @@ const (
// Listener could not be attached to be Gateway because its
// protocol type is not supported.
ListenerReasonUnsupportedProtocol ListenerConditionReason = "UnsupportedProtocol"

// This reason is used with the "Accepted" condition when the Listener could
// not be attached to the Gateway because the requested address is not
// supported. This reason could be used in a number of instances, including:
//
// * The address is already in use.
// * The type of address is not supported by the implementation.
ListenerReasonUnsupportedAddress ListenerConditionReason = "UnsupportedAddress"
)

const (
Expand Down Expand Up @@ -870,34 +854,6 @@ const (
// This reason is used with the "Programmed" condition when the condition is
// true.
ListenerReasonProgrammed ListenerConditionReason = "Programmed"
)

const (
// Ready is an optional Condition that has Extended support. When it's set,
// the condition indicates whether the Listener has been configured on the
// Gateway and traffic is ready to flow through the data plane immediately.
//
// Possible reasons for this condition to be True are:
//
// * "Ready"
//
// Possible reasons for this condition to be False are:
//
// * "Invalid"
// * "Pending"
//
// Possible reasons for this condition to be Unknown are:
//
// * "Pending"
//
// Controllers may raise this condition with other reasons,
// but should prefer to use the reasons listed above to improve
// interoperability.
ListenerConditionReady ListenerConditionType = "Ready"

// This reason is used with the "Ready" condition when the condition is
// true.
ListenerReasonReady ListenerConditionReason = "Ready"

// This reason is used with the "Ready" and "Programmed" conditions when the
// Listener is syntactically or semantically invalid.
Expand All @@ -908,3 +864,15 @@ const (
// online and ready to accept client traffic.
ListenerReasonPending ListenerConditionReason = "Pending"
)

const (
// "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.
//
// Deprecated: Ready is reserved for future use
ListenerConditionReady ListenerConditionType = "Ready"

// Deprecated: Ready is reserved for future use
ListenerReasonReady ListenerConditionReason = "Ready"
)