Skip to content

apis: add ServiceExport condition type/reasons #112

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 2 commits into from
Aug 26, 2025
Merged

apis: add ServiceExport condition type/reasons #112

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ad94d6
apis: add ServiceExport condition type/reasons
MrFreezeex Jun 27, 2025
7dbcbd1
apis: reintroduce the existing ServiceExport conflict types
MrFreezeex Aug 6, 2025
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
169 changes: 169 additions & 0 deletions pkg/apis/v1alpha1/serviceexport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,17 @@ const (
// service export has been recognized as valid by an mcs-controller.
// This will be false if the service is found to be unexportable
// (ExternalName, not found).
//
// Deprecated: use ServiceExportConditionValid instead
ServiceExportValid = "Valid"
// ServiceExportConflict means that there is a conflict between two
// exports for the same Service. When "True", the condition message
// should contain enough information to diagnose the conflict:
// field(s) under contention, which cluster won, and why.
// Users should not expect detailed per-cluster information in the
// conflict message.
//
// Deprecated: use ServiceExportConditionConflict instead
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 the existing constant names are fine but if the consensus is to rename them then just change the existing constants instead of deprecating. I think deprecating is overkill in this case. It's not a big deal to modify a constant name after bumping the mcs-api version. Also, if one has linting that flags the use of deprecated fields then it would have to be changed anyway.

Copy link
Member Author

@MrFreezeex MrFreezeex Jun 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The existing names are fine theoretically but I would prefer to have some naming consistency among the new const that we define hence deprecating the old one basically

Copy link
Contributor

Choose a reason for hiding this comment

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

OK but I would prefer not to deprecate for the reasons I outlined. At some point we would want to remove them so users would have to adjust anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

To me the removal of those should probably be in a new version of the apis (v1alpha2 or v1beta1) to try to be mindful of people importing the package and using those. If you have linting that flags deprecated I would tend to say that it actually is achieving what's expected: aka you should probably change your conditions usage 🤔.

Copy link
Contributor

Choose a reason for hiding this comment

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

To me the removal of those should probably be in a new version of the apis (v1alpha2 or v1beta1)

Perhaps but that's the CRD version. These are just Go constants and any changes would be observed when you bump the Go dependency which could be done independent of a CRD version bump. It's not a big deal either way - just trying to avoid the hassle of handling deprecation down the road if we don't need to.

Copy link

Choose a reason for hiding this comment

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

Yea IIRC we've changed/removed constants like these in Gateway API in semver releases (e.g. a future v0.3.0), not tied to or requiring new CRD versions.

Copy link
Member Author

@MrFreezeex MrFreezeex Jun 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Well for instance there's a bunch of deprecation here related to conditions: https://github.com/kubernetes-sigs/gateway-api/blob/e1310bbd66c54b757d28ee65cf363825f6189ca5/apis/v1/gateway_types.go

My reasoning is that when project would use a newer version of the CRD they will import some other go package meaning that they will already need to actively change some code anyway and thus we would be able to not carry over certain deprecation such as this one.

For a (small) go lib version bump though I don't think we should make users of this library change their code by dropping those const IMO.

ServiceExportConflict = "Conflict"
)

Expand All @@ -88,3 +92,168 @@ type ServiceExportList struct {
// +listType=set
Items []ServiceExport `json:"items"`
}

// ServiceExportConditionType is a type of condition associated with a
// ServiceExport. This type should be used with the ServiceExportStatus.Conditions
// field.
type ServiceExportConditionType string

// ServiceExportConditionReason defines the set of reasons that explain why a
// particular ServiceExport condition type has been raised.
type ServiceExportConditionReason string

// NewServiceExportCondition creates a new ServiceExport condition
func NewServiceExportCondition(t ServiceExportConditionType, status metav1.ConditionStatus, reason ServiceExportConditionReason, msg string) metav1.Condition {
return metav1.Condition{
Type: string(t),
Status: status,
Reason: string(reason),
Message: msg,
LastTransitionTime: metav1.Now(),
}
}

const (
// ServiceExportConditionValid is true when the Service Export is valid.
// This does not indicate whether or not the configuration has been exported
// to a control plane / data plane.
//
//
// Possible reasons for this condition to be true are:
//
// * "Valid"
//
// Possible reasons for this condition to be False are:
//
// * "NoService"
// * "InvalidServiceType"
//
// Controllers may raise this condition with other reasons,
// but should prefer to use the reasons listed above to improve
// interoperability.
ServiceExportConditionValid ServiceExportConditionType = "Valid"

// ServiceExportReasonValid is used with the "Valid" condition when the
// condition is True.
ServiceExportReasonValid ServiceExportConditionReason = "Valid"

// ServiceExportReasonNoService is used with the "Valid" condition when
// the associated Service does not exist.
ServiceExportReasonNoService ServiceExportConditionReason = "NoService"

// ServiceExportReasonInvalidServiceType is used with the "Valid"
// condition when the associated Service has an invalid type
// (per the KEP at least the ExternalName type).
ServiceExportReasonInvalidServiceType ServiceExportConditionReason = "InvalidServiceType"
)

const (
// ServiceExportConditionReady is true when the service is exported
// to some control plane or data plane or ready to be pulled.
//
//
// Possible reasons for this condition to be true are:
//
// * "Exported"
// * "Ready"
//
// Possible reasons for this condition to be False are:
//
// * "Pending"
// * "Failed"
//
// 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.
ServiceExportConditionReady ServiceExportConditionType = "Ready"

// ServiceExportReasonExported is used with the "Ready" condition
// when the condition is True and the service has been exported.
// This would be used when an implementation exports a service
// to a control plane or data plane.
ServiceExportReasonExported ServiceExportConditionReason = "Exported"

// ServiceExportReasonReady is used with the "Ready" condition
// when the condition is True and the service has been exported.
// This would typically be used in an implementation that uses a
// pull model.
ServiceExportReasonReady ServiceExportConditionReason = "Ready"

// ServiceExportReasonPending is used with the "Ready" condition
// when the service is in the process of being exported.
ServiceExportReasonPending ServiceExportConditionReason = "Pending"

// ServiceExportReasonFailed is used with the "Ready" condition
// when the service failed to be exported with the message providing
// the specific reason.
ServiceExportReasonFailed ServiceExportConditionReason = "Failed"
)

const (
// ServiceExportConditionConflict indicates that some property of an
// exported service has conflicting values across the constituent
// ServiceExports. This condition must be at least raised on the
// conflicting ServiceExport and is recommended to be raised on all on
// all the constituent ServiceExports if feasible.
//
//
// Possible reasons for this condition to be true are:
//
// * "PortConflict"
// * "TypeConflict"
// * "SessionAffinityConflict"
// * "SessionAffinityConfigConflict"
// * "AnnotationsConflict"
// * "LabelsConflict"
Comment on lines +206 to +211
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, it's not easy to extend, later we may support other field conflicts.

How about just defining the "ConflictFound" and putting the details in the message?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would prefer to have specific reasons IMO, you are free to use your own reasons in case your use case is not supported as stated in this file.

Copy link

Choose a reason for hiding this comment

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

Having standardized reasons is very helpful for communicating specific known failure scenarios in a predictable way (whereas the message field contents will differ by implementation) and is very helpful in conformance tests too.

We could consider adding an additional general-purpose Conflicted reason for failure scenarios not covered by these reasons, but this field reason is a string type) is extensible by implementations which wish to provide their own implementation-specific reasons as a supplment to the standard reasons.

We should expect (and maybe this is worth adding explicitly in docs here and/or KEP) that additional reasons may be defined by implementations or be added to the set of standard reasons in the future if needed.

Copy link
Member Author

@MrFreezeex MrFreezeex Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We could consider adding an additional general-purpose Conflicted reason for failure scenarios not covered by these reasons, but this field reason is a string type) is extensible by implementations which wish to provide their own implementation-specific reasons as a supplment to the standard reasons.

Adding a generic reason doesn't seems great. I would expect implementation to either use the provided one or make up their own. I don't see why you would use a generic one in this context...

We should expect (and maybe this is worth adding explicitly in docs here and/or KEP) that additional reasons may be defined by implementations or be added to the set of standard reasons in the future if needed.

It's actually already written each time we define some reasons thanks to GW-API "template" :D

Copy link
Contributor

Choose a reason for hiding this comment

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

So will we validate the reason in the conformance tests if we expect the consumer may define their own?

Copy link
Member Author

@MrFreezeex MrFreezeex Jul 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The conformance test generally present targeted conflict with one property conflicting at time so it should be relatively fine to make this into the conformance test. If there are test with multiple conflict we would probably not test the exact reason I would say. Also if you have particular situation that are specific to your implementation it would probably not be in the conformance test to begin with.

//
// When multiple conflicts occurs the above reasons may be combined
// using commas.
//
// Possible reasons for this condition to be False are:
//
// * "NoConflicts"
//
// Controllers may raise this condition with other reasons,
// but should prefer to use the reasons listed above to improve
// interoperability.
ServiceExportConditionConflict ServiceExportConditionType = "Conflict"

// ServiceExportReasonPortConflict is used with the "Conflict" condition
// when the exported service has a conflict related to port configuration.
// This includes when ports on resulting imported services would have
// duplicated names (including unnamed/empty name) or duplicated
// port/protocol pairs.
ServiceExportReasonPortConflict ServiceExportConditionReason = "PortConflict"

// ServiceExportReasonTypeConflict is used with the "Conflict" condition
// when the exported service has a conflict related to the service type
// (eg headless vs non-headless).
ServiceExportReasonTypeConflict ServiceExportConditionReason = "TypeConflict"

// ServiceExportReasonSessionAffinityConflict is used with the "Conflict"
// condition when the exported service has a conflict related to session affinity.
ServiceExportReasonSessionAffinityConflict ServiceExportConditionReason = "SessionAffinityConflict"

// ServiceExportReasonSessionAffinityConfigConflict is used with the
// "Conflict" condition when the exported service has a conflict related
// to session affinity config.
ServiceExportReasonSessionAffinityConfigConflict ServiceExportConditionReason = "SessionAffinityConfigConflict"

// ServiceExportReasonLabelsConflict is used with the "Conflict"
// condition when the ServiceExport has a conflict related to exported
// labels.
ServiceExportReasonLabelsConflict ServiceExportConditionReason = "LabelsConflict"

// ServiceExportReasonAnnotationsConflict is used with the "Conflict"
// condition when the ServiceExport has a conflict related to exported
// annotations.
ServiceExportReasonAnnotationsConflict ServiceExportConditionReason = "AnnotationsConflict"

// ServiceExportReasonNoConflicts is used with the "Conflict" condition
// when the condition is False.
ServiceExportReasonNoConflicts ServiceExportConditionReason = "NoConflicts"
)