Skip to content
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

Jump to bottom

Reverses Gateway Condition Polarity #104

Closed
wants to merge 1 commit into from
Closed

Reverses Gateway Condition Polarity #104

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
55 changes: 24 additions & 31 deletions api/v1alpha1/gateway_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,28 +270,22 @@ type GatewayStatus struct {
type GatewayConditionType string

const (
// ConditionNoSuchGatewayClass indicates that the specified GatewayClass
// does not exist.
ConditionNoSuchGatewayClass GatewayConditionType = "NoSuchGatewayClass"
// ConditionGatewayNotScheduled indicates that the Gateway has not been
// scheduled.
ConditionGatewayNotScheduled GatewayConditionType = "GatewayNotScheduled"
// ConditionListenersNotReady indicates that at least one of the specified
// listeners is not ready. If this condition has a status of True, a more
// detailed ListenerCondition should be present in the corresponding
// ListenerStatus.
ConditionListenersNotReady GatewayConditionType = "ListenersNotReady"
// ConditionInvalidListeners indicates that at least one of the specified
// listeners is invalid. If this condition has a status of True, a more
// detailed ListenerCondition should be present in the corresponding
// ListenerStatus.
ConditionInvalidListeners GatewayConditionType = "InvalidListeners"
// ConditionRoutesNotReady indicates that at least one of the specified
// routes is not ready.
ConditionRoutesNotReady GatewayConditionType = "RoutesNotReady"
// ConditionInvalidRoutes indicates that at least one of the specified
// routes is invalid.
ConditionInvalidRoutes GatewayConditionType = "InvalidRoutes"
// ConditionGatewayClassExists indicates that the specified GatewayClass exists.
ConditionGatewayClassExists GatewayConditionType = "GatewayClassExists"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we start off with GatewayClassInvalid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bowei GatewayClassInvalid is a negative condition type. This PR aims to use positive conditions per kubernetes/community#4521. I can update "GatewayClassExists" to "GatewayClassValid" with the same meaning that the GatewayClass referenced in gateway.spec.class exists, unless I'm missing your point.

// ConditionGatewayScheduled indicates that the Gateway has been scheduled.
ConditionGatewayScheduled GatewayConditionType = "GatewayScheduled"
Copy link
Contributor

Choose a reason for hiding this comment

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

You have to be more specific here. "Scheduled" means what for the end user?
For stuff that is informational, e.g. automation cannot act on it, has no semantic API meaning, it's worth thinking about whether it requires its own constant or just put the information into the human readable form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You have to be more specific here. "Scheduled" means what for the end user?

@bowei I tried to make minimal changes to the docs to support the intent of this PR, which is to reverse the polarity of the existing conditions. This was an existing condition. Do you prefer I make changes to the previously created condition types in this PR or have this PR strictly focus on reversing the polarity?

// ConditionListenersReady indicates that all of the specified listeners
// are ready. If this condition has a status of False, a more detailed
// ListenerCondition should be present in the corresponding ListenerStatus.
ConditionListenersReady GatewayConditionType = "ListenersReady"
// ConditionListenersValid indicates that all of the specified listeners
// are valid. If this condition has a status of False, a more detailed
// ListenerCondition should be present in the corresponding ListenerStatus.
ConditionListenersValid GatewayConditionType = "ListenersValid"
// ConditionRoutesReady indicates that all of the specified routes are ready.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we combine routes ready with routes valid? (can you give a good example why we want to distinguish these two states)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bowei this comes from https://kubernetes.slack.com/archives/CR0H13KGA/p1584484692070800. A valid route means the config has passed validation. A ready route means some type of readiness probe has passed. I'm not opposed to sticking with valid listener/route now to simplify the conditions until a concrete use case is presented for adding a route ready state status condition. cc: @robscott

Copy link
Contributor

Choose a reason for hiding this comment

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

My comments here are mostly around making the comments more descriptive. (e.g. can you take your responses and put them in the comments, so when people read it, they know exactly what the conditions mean?)

ConditionRoutesReady GatewayConditionType = "RoutesReady"
// ConditionRoutesValid indicates that all of the specified routes are valid.
ConditionRoutesValid GatewayConditionType = "RoutesValid"
)

// GatewayCondition is an error status for a given route.
Expand Down Expand Up @@ -326,15 +320,14 @@ type ListenerStatus struct {
type ListenerConditionType string

const (
// ConditionInvalidListener is a generic condition that is a catch all for
// unsupported configurations that do not match a more specific condition.
// Implementors should try to use a more specific condition instead of this
// one to give users and automation more information.
ConditionInvalidListener ListenerConditionType = "InvalidListener"
// ConditionListenerNotReady indicates the listener is not ready.
ConditionListenerNotReady ListenerConditionType = "ListenerNotReady"
// ConditionInvalidAddress indicates the Address is invalid.
ConditionInvalidAddress ListenerConditionType = "InvalidAddress"
// ConditionListenerValid is a generic condition that is a catch all for
// supported configurations. Implementors should try to use a more specific
// condition instead of this one to give users and automation more information.
ConditionListenerValid ListenerConditionType = "ListenerValid"
// ConditionListenerReady indicates the listener is ready to receive traffic.
ConditionListenerReady ListenerConditionType = "ListenerReady"
// ConditionValidAddress indicates the listener Address is valid.
ConditionValidAddress ListenerConditionType = "ValidAddress"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we clarify that these will always overlap:

Valid implies Ready?
Or Valid && Ready == can serve traffic?

)

// ListenerCondition is an error status for a given listener.
Expand Down