API Review for v0.6.0 release #1538

robscott · 2022-11-16T23:43:41Z

What type of PR is this?
/kind api-change

What this PR does / why we need it:
THIS PR WILL NOT MERGE, THIS IS INTENDED FOR REVIEW ONLY.

This is a draft PR to facilitate the review of the v0.6.0 release of Gateway API. This compares the current state of /apis from the main branch with the state in v0.5.

Does this PR introduce a user-facing change?:
Changelog will be computed separately.

/cc @bowei @shaneutt @youngnick @howardjohn @keithmattix @mikemorris
/assign @khenidak @thockin

k8s-ci-robot · 2022-11-16T23:43:42Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

robscott · 2022-11-16T23:43:56Z

/hold forever

thockin

I admit I only skimmed validation code.

Overall this seems OK. The prep-meeting helped a lot.

apis/v1alpha2/gateway_types.go

apis/v1alpha2/grpcroute_types.go

thockin · 2022-11-18T19:43:41Z

apis/v1alpha2/grpcroute_types.go

+	// non-empty, then the implementation MUST accept exactly one of these two
+	// routes, determined by the following criteria, in order:
+	//
+	// * The oldest Route based on creation timestamp.


This is very specific - do we need it to be so specific? What if the implementation intends to prefer GRPC - not allowed?

I'd rather start with more specific/stringent requirements. Wherever possible, I'd like to have consistent and portable behavior. If in the future an implementation wants to prefer GRPC we could make a carve out for how to do that here (likely something along the lines of "any deviation from this approach must be clearly documented").

More detail on this decision is in the GEP for GRPCRoute. Originally we wanted to allow a GRPCRoute and HTTPRoute sharing a hostname to coexist in this scenario and to merge their individual rules very similarly to how two HTTPRoutes would do so.

But after investigating what it would take to enable this behavior in a couple of prospective implementations (Traffic Director and Istio), we realized that it would be difficult to accomplish this. In the interest of starting with something simple that we can evolve from, we changed to the resolution mechanism here, which is the same as for two HTTPRoutes with rules with tied precedence. It would be a backward-compatible change to enable this merge behavior in the future.

apis/v1alpha2/validation/grpcroute.go

thockin · 2022-11-18T20:23:29Z

apis/v1beta1/referencegrant_types.go

+	// Spec defines the desired state of ReferenceGrant.
+	Spec ReferenceGrantSpec `json:"spec,omitempty"`
+
+	// Note that `Status` sub-resource has been excluded at the


I'm still skeptical of this :)

I think the most likely use of this would be for implementations to publish the references this grant is actually allowing. I think that would be very helpful, but I'm less sure if that would actually scale. As we've found in previous cases, it's much safer to leave room for it than to leave it out altogether.

thockin · 2022-11-18T20:25:31Z

apis/v1beta1/referencegrant_types.go

+	// * TCPRoute
+	// * TLSRoute
+	// * UDPRoute
+	Kind Kind `json:"kind"`


Required or optional? Can I grant a whole namespace access to a thing? Wouldn't that be the norm if this went upstream?

I don't think we should make this optional. The purpose of ReferenceGrant is to enable very specific kinds of references, not broad access. I'd be worried that allowing ReferenceGrant to be used to enable such broad access could have unintended consequences. For example, someone might use the broad access with the intent of allowing access for all kinds they currently know about, but in the future a new kind may be added that results in a significant change to the access that an open ReferenceGrant like that would enable.

apis/v1beta1/validation/common.go

khenidak

This review covers types /apis/v1alpha2 directory. Rest to follow.

apis/v1alpha2/gateway_types.go

apis/v1alpha2/grpcroute_types.go

apis/v1alpha2/httproute_types.go

apis/v1alpha2/shared_types.go

apis/v1alpha2/referencegrant_types.go

apis/v1alpha2/util/translator/httproute.go

apis/v1alpha2/util/translator/shared_types.go

apis/v1alpha2/validation/common.go

apis/v1alpha2/validation/grpcroute_test.go

khenidak

That is everything i have. Thank you for this effort. I really appreciate it.

apis/v1beta1/util/validation/common.go

apis/v1beta1/util/validation/common_test.go

apis/v1alpha2/validation/tlsroute_test.go

apis/v1alpha2/validation/udproute_test.go

khenidak · 2022-11-22T19:36:44Z

apis/v1beta1/object_reference_types.go

-	// Group is the group of the referent. For example, "networking.k8s.io".
-	// When unspecified (empty string), core API group is inferred.
+	// Group is the group of the referent. For example, "gateway.networking.k8s.io".
+	// When unspecified or empty string, core API group is inferred.


This is going to be interesting to see in patch updates. Consider this

create w/ Group: "G"
update other fields w/ Group field value: "G"
Update other fields w/Group field value:"" // which will be defaulted to "" removing "G" which is definitly not the intention. Can we double check the behavior?

I tried this out with the following transition:

tls: certificateRefs: - kind: Secret group: "g" name: foo-example-com-cert

tls: certificateRefs: - kind: Secret name: foo-example-com-cert

As you expected, after applying step 2, the group became "" instead of "g". With that said, I'm not sure why you would remove a group from a manifest, or if you were trying to just update a subset, why you wouldn't use patch. This is likely something we need to more clearly document, but I'm hoping it doesn't represent a typical workflow.

apis/v1beta1/util/translator/httproute.go

…updates chore: cleanup for #1538

apis/v1beta1/gateway_types.go

apis/v1alpha2/grpcroute_types.go

apis/v1alpha2/httproute_types.go

apis/v1alpha2/shared_types.go

k8s-ci-robot · 2022-12-03T01:00:18Z

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

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

In response to this:

Any suggestions for how to rework this? I think this is just talking about HTTP/2 Cleartext which would mean no cipher.

/cc @gnossen

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.

…kubernetes-sigs#1580

apis/v1alpha2/gateway_types.go

robscott · 2022-12-20T19:09:22Z

Thanks for the reviews @khenidak and @thockin! Can you formally approve these changes for v0.6.0 release?

khenidak · 2022-12-20T20:16:09Z

/approve /lgtm

k8s-ci-robot · 2022-12-20T20:16:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: khenidak, 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

thockin · 2022-12-21T00:38:54Z

Kal - each bot cmd goes on its own line :)

/lgtm

See? :)

robscott · 2022-12-27T18:19:13Z

Thanks to everyone for the reviews! We've released v0.6.0 and linked to this as where the release was approved. Will close this out.

API Review for v0.6.0 release

a5c0a50

k8s-ci-robot assigned khenidak Nov 16, 2022

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Nov 16, 2022

k8s-ci-robot assigned thockin Nov 16, 2022

k8s-ci-robot requested review from bowei, howardjohn, keithmattix, mikemorris, shaneutt and youngnick November 16, 2022 23:43

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 16, 2022

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2022

robscott mentioned this pull request Nov 17, 2022

Some small cleanup from v0.6.0 review #1539

Merged

thockin reviewed Nov 18, 2022

View reviewed changes

khenidak reviewed Nov 21, 2022

View reviewed changes

khenidak reviewed Nov 22, 2022

View reviewed changes

shaneutt mentioned this pull request Dec 1, 2022

chore: cleanup for #1538 #1569

Merged

k8s-ci-robot added a commit that referenced this pull request Dec 2, 2022

Merge pull request #1569 from shaneutt/shaneutt/referencegrant-godoc-…

223094b

…updates chore: cleanup for #1538

robscott mentioned this pull request Dec 3, 2022

v0.6.0 review cleanup #1576

Merged

robscott commented Dec 3, 2022

View reviewed changes

Including changes from kubernetes-sigs#1576, kubernetes-sigs#1569, and …

3ffa2bc

…kubernetes-sigs#1580

This was referenced Dec 7, 2022

Replacing util/translate with generic func, misc cleanup from API review #1585

Merged

Tightening GRPCRoute validation, fixing gaps in HTTPRoute and Gateway webhook validation #1599

Merged

shaneutt mentioned this pull request Dec 14, 2022

v0.6.0 review updates #1601

Merged

sunjayBhatia reviewed Dec 14, 2022

View reviewed changes

apis/v1alpha2/gateway_types.go Outdated Show resolved Hide resolved

youngnick mentioned this pull request Dec 15, 2022

Update types to fix API review feedback #1604

Merged

Final changes for v0.6.0

90829ed

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 21, 2022

robscott closed this Dec 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Review for v0.6.0 release #1538

API Review for v0.6.0 release #1538

robscott commented Nov 16, 2022

k8s-ci-robot commented Nov 16, 2022

robscott commented Nov 16, 2022

thockin left a comment

thockin Nov 18, 2022

robscott Dec 3, 2022

gnossen Dec 5, 2022

thockin Nov 18, 2022

robscott Dec 7, 2022 •

edited

Loading

thockin Nov 18, 2022

robscott Dec 3, 2022

khenidak left a comment

khenidak left a comment

khenidak Nov 22, 2022

robscott Dec 20, 2022

k8s-ci-robot commented Dec 3, 2022

robscott commented Dec 20, 2022

khenidak commented Dec 20, 2022

k8s-ci-robot commented Dec 20, 2022

thockin commented Dec 21, 2022

robscott commented Dec 27, 2022

API Review for v0.6.0 release #1538

API Review for v0.6.0 release #1538

Conversation

robscott commented Nov 16, 2022

k8s-ci-robot commented Nov 16, 2022

robscott commented Nov 16, 2022

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khenidak left a comment

Choose a reason for hiding this comment

khenidak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 3, 2022

robscott commented Dec 20, 2022

khenidak commented Dec 20, 2022

k8s-ci-robot commented Dec 20, 2022

thockin commented Dec 21, 2022

robscott commented Dec 27, 2022

robscott Dec 7, 2022 •

edited

Loading