apis: add GatewayClassConditionReason Unsupported #3048

mikemorris · 2024-05-02T21:36:29Z

...and clarifies the description for when GatewayClassConditionReason InvalidParameters should be preferred.

I considered adding something like GatewayClassConditionType ResolvedRefs with GatewayClassConditionType InvalidParametersRef as a more spec-consistent way to handle an invalid parametersRef reference as distinct from malformed content, but that felt like too big a change given that the documentation for the parametersRef field currently states:

If the referent cannot be found, the GatewayClass’s “InvalidParameters” status condition will be true.

EDIT: Although now that I read that, that sentence needs to be reworded to be accurate.

What type of PR is this?
/kind documentation
/area conformance

What this PR does / why we need it:
Adds an appropriate reason for implementations (such as those of cloud vendors) which provide their own fixed GatewayClass offerings and do not support manually created GatewayClass resources.

Which issue(s) this PR fixes:
Provides an appropriate status condition reason to set for the GatewayClassObservedGeneration test currently skipped and blocking Azure Application Gateway for Containers full conformance in #3021

Does this PR introduce a user-facing change?:

Adds the GatewayClassConditionReason "Unsupported" and clarifies when GatewayClassConditionReason "InvalidParameters" should be preferred.

/cc @robscott @youngnick @snehachhabria

k8s-ci-robot · 2024-05-02T21:36:33Z

@mikemorris: The label(s) kind/conformance cannot be applied, because the repository doesn't have them.

In response to this:

...and clarifies the description for when GatewayClassConditionReason InvalidParameters should be preferred.

I considered adding something like GatewayClassConditionType ResolvedRefs with GatewayClassConditionType InvalidParametersRef as a more spec-consistent way to handle an invalid parametersRef reference as distinct from malformed content, but that felt like too big a change given that the documentation for the parametersRef field currently states:

If the referent cannot be found, the GatewayClass’s “InvalidParameters” status condition will be true.

What type of PR is this?
/kind documentation
/kind conformance

What this PR does / why we need it:
Adds an appropriate reason for implementations (such as those of cloud vendors) which provide their own fixed GatewayClass offerings and do not support manually created GatewayClass resources.

Which issue(s) this PR fixes:
Provides an appropriate status condition reason to set for the GatewayClassObservedGeneration test currently skipped and blocking Azure Application Gateway for Containers full conformance in #3021

Does this PR introduce a user-facing change?:
Adds the GatewayClassConditionReason "Invalid" and clarifies when GatewayClassConditionReason "InvalidParameters" should be preferred.
/cc @robscott @youngnick @snehachhabria

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.

k8s-ci-robot · 2024-05-02T21:36:33Z

@mikemorris: GitHub didn't allow me to request PR reviews from the following users: snehachhabria.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

...and clarifies the description for when GatewayClassConditionReason InvalidParameters should be preferred.

I considered adding something like GatewayClassConditionType ResolvedRefs with GatewayClassConditionType InvalidParametersRef as a more spec-consistent way to handle an invalid parametersRef reference as distinct from malformed content, but that felt like too big a change given that the documentation for the parametersRef field currently states:

If the referent cannot be found, the GatewayClass’s “InvalidParameters” status condition will be true.

What type of PR is this?
/kind documentation
/kind conformance

What this PR does / why we need it:
Adds an appropriate reason for implementations (such as those of cloud vendors) which provide their own fixed GatewayClass offerings and do not support manually created GatewayClass resources.

Which issue(s) this PR fixes:
Provides an appropriate status condition reason to set for the GatewayClassObservedGeneration test currently skipped and blocking Azure Application Gateway for Containers full conformance in #3021

Does this PR introduce a user-facing change?:
Adds the GatewayClassConditionReason "Invalid" and clarifies when GatewayClassConditionReason "InvalidParameters" should be preferred.
/cc @robscott @youngnick @snehachhabria

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.

robscott

Thanks @mikemorris!

robscott · 2024-05-02T22:12:50Z

apis/v1/gatewayclass_types.go

+	// GatewayClass was not accepted, with more detail in the message.
+	GatewayClassReasonInvalid GatewayClassConditionReason = "Invalid"


Since there's not really much to a GatewayClass, it may be slightly more accurate to use "Unsupported" here if custom GatewayClasses just aren't supported. I think either are probably fine here, interested in what others think.

I'd be happy with Unsupported instead if folks would prefer that!

I feel Unsupported is a better than Invalid as its a more accurate reason even if a message is or is not provided. Invalid can have a wider scope.

apis/v1/gatewayclass_types.go

Clarify description for when GatewayClassConditionReason InvalidParameters should be preferred.

…metersRef

mikemorris · 2024-05-03T20:02:40Z

/retest

Is this Docker builder removal timeout issue with CI documented anywhere as needing a fix yet?

robscott · 2024-05-03T21:17:04Z

@mikemorris would it be ok if we held off on merging this until post-v1.1? I'm completely on board with the change, but hate to make any changes to CRDs/API Spec between RC2 and final release. Maybe implementations could just manually add this condition until it was formally added here?

robscott · 2024-05-03T21:18:56Z

Is this Docker builder removal timeout issue with CI documented anywhere as needing a fix yet?

Nope, I think I asked about in Slack once, but really unsure who could help us figure this one out. As a short term fix, it might be worth splitting out our docker build verification into a separate task that doesn't run with the rest of our verification. We'd just need to rework our Makefile a bit and add a new task here: https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes-sigs/gateway-api/gateway-api-config.yaml.

mikemorris · 2024-05-06T16:16:38Z

@mikemorris would it be ok if we held off on merging this until post-v1.1?

Sure, no objections to that!

robscott · 2024-05-10T00:45:42Z

Thanks @mikemorris!

/lgtm
/approve

k8s-ci-robot · 2024-05-10T00:45:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikemorris, robscott

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~~ [robscott]

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

* apis: add GatewayClassConditionReason Invalid Clarify description for when GatewayClassConditionReason InvalidParameters should be preferred. * doc: clarify expected behavior for bad reference in GatewayClass ParametersRef * docs: GatewayClass SHOULD be rejected with InvalidParameters reason * apis: switch from Invalid reason to Unsupported * fixup codegen and yamllint --------- Co-authored-by: Mike Morris <1149913+mikemorris@users.noreply.github.com>

k8s-ci-robot requested a review from robscott May 2, 2024 21:36

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/documentation Categorizes issue or PR as related to documentation. labels May 2, 2024

k8s-ci-robot requested a review from youngnick May 2, 2024 21:36

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 2, 2024

robscott reviewed May 2, 2024

View reviewed changes

mikemorris commented May 2, 2024

View reviewed changes

apis/v1/gatewayclass_types.go Outdated Show resolved Hide resolved

apis: add GatewayClassConditionReason Invalid

a2f3dd7

Clarify description for when GatewayClassConditionReason InvalidParameters should be preferred.

mikemorris force-pushed the gatewayclass-condition-reason-invalid branch from 1fe4585 to e08357c Compare May 2, 2024 22:25

doc: clarify expected behavior for bad reference in GatewayClass Para…

1debac4

…metersRef

mikemorris force-pushed the gatewayclass-condition-reason-invalid branch from e08357c to 1debac4 Compare May 2, 2024 22:27

mikemorris and others added 2 commits May 2, 2024 19:01

docs: GatewayClass SHOULD be rejected with InvalidParameters reason

896ad6d

apis: switch from Invalid reason to Unsupported

4c83e43

mikemorris force-pushed the gatewayclass-condition-reason-invalid branch from a0728d1 to 4c83e43 Compare May 2, 2024 23:02

mikemorris changed the title ~~apis: add GatewayClassConditionReason Invalid~~ apis: add GatewayClassConditionReason Unsupported May 2, 2024

fixup codegen and yamllint

bcaef32

k8s-ci-robot assigned robscott May 10, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2024

k8s-ci-robot merged commit 22c1c0e into kubernetes-sigs:main May 10, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apis: add GatewayClassConditionReason Unsupported #3048

apis: add GatewayClassConditionReason Unsupported #3048

mikemorris commented May 2, 2024 •

edited

Loading

k8s-ci-robot commented May 2, 2024

k8s-ci-robot commented May 2, 2024

robscott left a comment

robscott May 2, 2024

mikemorris May 2, 2024 •

edited

Loading

snehachhabria May 2, 2024 •

edited

Loading

mikemorris commented May 3, 2024 •

edited

Loading

robscott commented May 3, 2024

robscott commented May 3, 2024

mikemorris commented May 6, 2024

robscott commented May 10, 2024

k8s-ci-robot commented May 10, 2024

		// GatewayClass was not accepted, with more detail in the message.
		GatewayClassReasonInvalid GatewayClassConditionReason = "Invalid"

apis: add GatewayClassConditionReason Unsupported #3048

apis: add GatewayClassConditionReason Unsupported #3048

Conversation

mikemorris commented May 2, 2024 • edited Loading

k8s-ci-robot commented May 2, 2024

k8s-ci-robot commented May 2, 2024

robscott left a comment

Choose a reason for hiding this comment

robscott May 2, 2024

Choose a reason for hiding this comment

mikemorris May 2, 2024 • edited Loading

Choose a reason for hiding this comment

snehachhabria May 2, 2024 • edited Loading

Choose a reason for hiding this comment

mikemorris commented May 3, 2024 • edited Loading

robscott commented May 3, 2024

robscott commented May 3, 2024

mikemorris commented May 6, 2024

robscott commented May 10, 2024

k8s-ci-robot commented May 10, 2024

mikemorris commented May 2, 2024 •

edited

Loading

mikemorris May 2, 2024 •

edited

Loading

snehachhabria May 2, 2024 •

edited

Loading

mikemorris commented May 3, 2024 •

edited

Loading