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.

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

[WIP] conformance: require that GatewayClass controller name is immutable #1534

Closed

Conversation

markmc
Copy link
Contributor

@markmc markmc commented Nov 14, 2022

Relates to #1514

This is a proof-of-concept conformance test that checks for the validation behavior provided by the admission webhook. In this case we're just checking that an update to a GatewayClass controller name is rejected.

We use a temporary GatewayClass rather than the one specified by the --gateway-class, to avoid messing up installations that do allow this update through.

This does raise an interesting question though - isn't it strange that we're saying an implementation is required to validate objects that it should otherwise ignore? In this case, we would would otherwise say an implementation should ingore this object because it doesn't recognize the controller name.

Failure looks like this:

=== RUN   TestConformance/GatewayClassAdmissionValidation/GatewayClass_controllerName_is_immutable
    gatewayclass-admission-validation.go:62:
        	Error Trace:	gatewayclass-admission-validation.go:62
        	Error:      	Should be in error chain:
        	            	expected: %!q(**errors.StatusError=0xc000014008)
        	            	in chain:
        	Test:       	TestConformance/GatewayClassAdmissionValidation/GatewayClass_controllerName_is_immutable
        	Messages:   	updating gatewayclass-immutable GatewayClass.Spec.ControllerName should not be permitted

Relates to kubernetes-sigs#1514

This is a proof-of-concept conformance test that checks for the
validation behavior provided by the admission webhook. In this case
we're just checking that an update to a GatewayClass controller name
is rejected.

We use a temporary GatewayClass rather than the one specified by the
--gateway-class, to avoid messing up installations that do allow this
update through.

This does raise an interesting question though - isn't it strange that
we're saying an implementation is required to validate objects that it
should otherwise ignore? In this case, we would would otherwise say an
implementation should ingore this object because it doesn't recognize
the controller name.

Failure looks like this:

```
=== RUN   TestConformance/GatewayClassAdmissionValidation/GatewayClass_controllerName_is_immutable
    gatewayclass-admission-validation.go:62:
        	Error Trace:	gatewayclass-admission-validation.go:62
        	Error:      	Should be in error chain:
        	            	expected: %!q(**errors.StatusError=0xc000014008)
        	            	in chain:
        	Test:       	TestConformance/GatewayClassAdmissionValidation/GatewayClass_controllerName_is_immutable
        	Messages:   	updating gatewayclass-immutable GatewayClass.Spec.ControllerName should not be permitted
```
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 14, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: markmc
Once this PR has been reviewed and has the lgtm label, please assign bowei for approval by writing /assign @bowei in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 14, 2022
@shaneutt
Copy link
Member

This is marked as a WIP, so I'm converting it to draft for organizational reasons. If this is now ready for review, please convert it back from draft and remove the WIP and we'll take a look, thank you! 🖖

@shaneutt shaneutt marked this pull request as draft January 31, 2023 14:40
@shaneutt
Copy link
Member

shaneutt commented Mar 8, 2023

Hey @markmc, it's been a while since we heard back from you on this one. Do you still have time for this one? Do you need any help?

@markmc markmc closed this May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants