v0.7.0 API Review #1923

robscott · 2023-04-06T20:36:52Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR is for the purpose of reviewing the v0.7.0 release. It compares the /apis dir from the main branch to the /apis dir on the release-0.6 branch. This is to avoid distracting the PR with all the docs, GEP, and test changes that have also occurred in the same time. This PR is not intended to merge, instead is meant purely to facilitate API review.

/cc @bowei @shaneutt @youngnick
/assign @khenidak @thockin

k8s-ci-robot · 2023-04-06T20:36:54Z

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

thockin · 2023-04-06T22:20:53Z

apis/v1alpha2/httproute_types.go

-// +kubebuilder:validation:Pattern=`^[A-Za-z0-9!#$%&'*+\-.^_\x60|~]+$`
 // +k8s:deepcopy-gen=false
-type HTTPHeaderName = v1beta1.HTTPHeaderName
+type HTTPHeaderName = v1beta1.HeaderName


Why did we drop HTTP from this?

Looks like because of the above change to make the GRPC variant type GRPCHeaderName v1beta1.HeaderName instead of its own string alias?

Yep, this was trying to ensure that our types and validation were consistent since by definition a GRPC Header name is going to have the same validation and constraints as HTTP.

apis/v1beta1/httproute_types.go

thockin · 2023-04-06T22:35:58Z

apis/v1beta1/httproute_types.go

-// +kubebuilder:validation:Pattern=`^[A-Za-z0-9!#$%&'*+\-.^_\x60|~]+$`
-type HTTPHeaderName string
+//   - "/invalid" - "/ " is an invalid character
+type HTTPHeaderName HeaderName


Curious about this indirection?

This is part a larger effort to ensure that our types and validation are consistent across GRPC and HTTP Routes. Leaving the "HTTPHeaderName" in place for backwards compat.

apis/v1beta1/validation/gateway.go

apis/v1beta1/gateway_types.go

khenidak

Minor comments, otherwise this looks fine.

khenidak · 2023-04-13T19:08:21Z

apis/v1alpha2/tlsroute_types.go

 	//
 	// Support: Core for Kubernetes Service
 	//
+	// Support: Extended for Kubernetes ServiceImport


Nit: While we all understand what ServiceImport is, a new reader may not. So better to call out that these are objects.

Same comment applies in a lot of places.

To clarify, are you saying that "Extended for Kubernetes ServiceImport" should be replaced with "Extended for Kubernetes ServiceImport Objects", would the same be true for "Core for Kubernetes Service Objects"? I'm not that familiar with the proper terminology here, but would "Resource(s)" work as well?

apis/v1alpha2/validation/grpcroute.go

khenidak · 2023-04-13T19:15:21Z

apis/v1alpha2/validation/grpcroute.go

 	for i, m := range matches {
-		if m.Method != nil && m.Method.Service == nil && m.Method.Method == nil {
-			errs = append(errs, field.Required(path.Index(i).Child("method"), "one or both of `service` or `method` must be specified"))
+		if m.Method != nil {


can m.Method == nil if so, is it a valid object?

Yep, it's optional, but there is defaulting involved so it seems unlikely/impossible that this would ever be empty.

We often add the else ... field.Required() to these - it has helped prevent tests that passed whjen they shouldn't.

apis/v1alpha2/validation/grpcroute.go

khenidak · 2023-04-13T19:24:41Z

apis/v1beta1/httproute_types.go

+	// Implementations SHOULD NOT add the port number in the 'Location'
+	// header in the following cases:
+	//
+	// * A Location header that will use HTTP (whether that is determined via


the _and_ construct will not work as expected this file will be viewed by code editors which generally don't do Markdown.

Maybe you meant to comment on a different line, I can't find an _and_ here. In general we're trying to ensure https://gateway-api.sigs.k8s.io/references/spec/ is readable, and in that case basic markdown formatting is effective.

apis/v1beta1/validation/gateway.go

khenidak · 2023-04-13T19:29:59Z

apis/v1beta1/validation/gateway.go

+		protocol := listener.Protocol
+		port := listener.Port
+		hostnameProtocolPort := fmt.Sprintf("%s:%s:%d", *hostname, protocol, port)
+		if hostnameProtocolPortSets.Has(hostnameProtocolPort) {


a map[string]struct{}{} will be better here.

I think the k8s set util that we're using here is roughly doing the same thing but has a nicer interface to work with (https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/sets/set.go#L24). Anything I'm missing?

I found some places still use map instead of k8s set, such as https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1beta1/validation/gateway.go#L129, https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1beta1/validation/httproute.go#L110. I think we can unify them.

khenidak · 2023-05-15T15:18:25Z

/approve
/lgtm

Thanks for this.

k8s-ci-robot · 2023-05-15T15:18:34Z

[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

Details

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/v1beta1/validation/gateway.go

thockin

LGTM, just a few minor nits on re-scan

thockin

LGTM, just a few minor nits on re-scan

thockin · 2023-05-15T16:29:46Z

/lgtm

to make it real

robscott · 2023-05-15T23:14:12Z

Thanks everyone for the reviews! v0.7.0 has been released, closing this out.

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2023

k8s-ci-robot assigned khenidak Apr 6, 2023

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 6, 2023

k8s-ci-robot assigned thockin Apr 6, 2023

k8s-ci-robot requested review from bowei, shaneutt and youngnick April 6, 2023 20:36

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 6, 2023

shaneutt added this to the v0.7.0 milestone Apr 6, 2023

shaneutt added the release-blocker MUST be completed to complete the milestone label Apr 6, 2023

thockin reviewed Apr 6, 2023

View reviewed changes

rainest reviewed Apr 10, 2023

View reviewed changes

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

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

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

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

This was referenced Apr 13, 2023

cleanup: Replace error types when validating unique fields #1933

Merged

cleanup: Touch up some of the doc comments #1934

Merged

khenidak reviewed Apr 13, 2023

View reviewed changes

robscott mentioned this pull request Apr 24, 2023

Validation cleanup from v0.7.0 API Review #1972

Merged

v0.7.0 API Review

a446eb3

robscott force-pushed the review-0.7 branch from 1cfa968 to a446eb3 Compare April 27, 2023 23:44

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

thockin reviewed May 15, 2023

View reviewed changes

apis/v1beta1/validation/gateway.go Show resolved Hide resolved

thockin reviewed May 15, 2023

View reviewed changes

apis/v1beta1/validation/gateway.go Show resolved Hide resolved

thockin reviewed May 15, 2023

View reviewed changes

robscott mentioned this pull request May 15, 2023

Replacing fmt.Sprintln with plain strings #2022

Merged

robscott closed this May 15, 2023

v0.7.0 API Review #1923

v0.7.0 API Review #1923

Uh oh!

Conversation

robscott commented Apr 6, 2023

Uh oh!

k8s-ci-robot commented Apr 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

khenidak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thockin May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khenidak commented May 15, 2023

Uh oh!

k8s-ci-robot commented May 15, 2023

Uh oh!

Uh oh!

Uh oh!

thockin left a comment

Choose a reason for hiding this comment

Uh oh!

thockin left a comment

Choose a reason for hiding this comment

Uh oh!

thockin commented May 15, 2023

Uh oh!

robscott commented May 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

thockin May 15, 2023 •

edited

Loading