v1.1 API Review #2997

robscott · 2024-04-19T16:44:24Z

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

What this PR does / why we need it:
This PR is a diff of /apis from v1.1 (main branch) to v1.0 (release-1.0 branch). I've made one change - excluding the added generated applyconfigurations because they would add a lot of noise.

Note: This PR is purely to facilitate review, it is not intended to merge.

Does this PR introduce a user-facing change?:
See #2967

/assign @aojea @danwinship @thockin

k8s-ci-robot · 2024-04-19T16:44:27Z

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

k8s-ci-robot · 2024-04-19T16:44:27Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

shaneutt · 2024-04-19T20:21:03Z

/cc

k8s-ci-robot · 2024-04-23T10:02:24Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,shaneutt]

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

aojea · 2024-04-24T11:10:17Z

apis/v1/object_reference_types.go

+	// Support: Core
+	//
+	// +optional
+	Namespace *Namespace `json:"namespace,omitempty"`


a namespace with an empty name, namespace = "" is an error or is not even possible?

We use type aliases heavily for common validation, including in this case. So nil is valid (pointer type) but "" is not.

gateway-api/apis/v1/shared_types.go

Lines 598 to 601 in 12d50fe

// +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`

// +kubebuilder:validation:MinLength=1

// +kubebuilder:validation:MaxLength=63

type Namespace string

aojea · 2024-04-24T11:24:22Z

apis/v1/gateway_types.go

@@ -186,8 +187,8 @@ type GatewaySpec struct {
 	// +listMapKey=name
 	// +kubebuilder:validation:MinItems=1
 	// +kubebuilder:validation:MaxItems=64
-	// +kubebuilder:validation:XValidation:message="tls must be specified for protocols ['HTTPS', 'TLS']",rule="self.all(l, l.protocol in ['HTTPS', 'TLS'] ? has(l.tls) : true)"


is this no longer required?

I see is explained here #2721

aojea · 2024-04-24T12:10:14Z

apis/v1/shared_types.go

+	// Support: Extended
+	//
+	// +optional
+	AbsoluteTimeout *Duration `json:"absoluteTimeout,omitempty"`


is 0s allowed? it should not (I guess)

Yep, it's a common way to disable timeouts in proxy implementations - x-ref #3012

apis/v1alpha3/backendtlspolicy_types.go

aojea · 2024-04-24T12:43:11Z

apis/v1/grpcroute_types.go

+	//
+	// +optional
+	// <gateway:experimental>
+	SessionPersistence *SessionPersistence `json:"sessionPersistence"`


I think this is related to this https://github.com/grpc/proposal/blob/master/A55-xds-stateful-session-affinity.md 🧑‍🎓

Yep, Sanjay and Gina from gRPC team were helping with this Gateway API proposal.

aojea · 2024-04-24T15:07:02Z

apis/v1alpha2/policy_types.go

+// in different namespaces. For more information on how this policy attachment
+// model works, and a sample Policy resource, refer to the policy attachment
+// documentation for Gateway API.
+type NamespacedPolicyTargetReference struct {


AFAIK this is not referenced elsewhere , only in this gwctl tool, is that correct?

Yep, it's part of the types that we encourage policy extensions to use, but not used internally.

We've discussed that this type may be useful for mesh policies in the future, such as applying client-side policy for outbound connections from a pod to a destination in another namespace.

aojea · 2024-04-24T15:15:54Z

apis/v1alpha3/backendtlspolicy_types.go

 	//
 	// +optional
-	WellKnownCACerts *WellKnownCACertType `json:"wellKnownCACerts,omitempty"`
+	WellKnownCACertificates *WellKnownCACertificatesType `json:"wellKnownCACertificates,omitempty"`


Who defines this list of WellKnownCA certiticates? is it implementation specific?

My specific question is if 2 different implementations that set this field to System can have different lists

yep, implementation specific, we should probably make this more clear. In many/most cases, this will depend on something like https://packages.debian.org/sid/ca-certificates being used by the Gateway, but there are lots of variations of what is trusted, or some may even choose to inject their own well known CA certs to a controller container to have them considered as "system", all of this is inherently implementation specific though.

ok, is clarified in the documentation

There may be some variation in which system certificates are used by each implementation. Refer to documentation from your implementation of choice for more information.

aojea · 2024-04-24T16:23:04Z

apis/v1alpha2/backendlbpolicy_types.go

+	// Support: Extended
+	//
+	// +optional
+	SessionPersistence *SessionPersistence `json:"sessionPersistence"`


talked offline with Rob, LB and SessionPersistence can mean a lot of things , specially depending at what layer of the LB we are talking ...

robscott · 2024-05-06T21:55:48Z

Thanks to everyone (@aojea, @danwinship, @thockin) for the feedback on this both on the PR and in our review meeting. We've released RC1 and RC2 and are planning to move forward with this release on Wednesday, May 9. Please get any remaining feedback in ASAP. We'll plan on closing out this PR and the formal review process ~24 hours from now.

robscott · 2024-05-08T20:16:31Z

Formally closing this one out, thanks for the feedback on this! We'll be releasing v1.1 in another ~3 hours.

v1.1 API Review

56be7a7

k8s-ci-robot assigned aojea Apr 19, 2024

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 19, 2024

k8s-ci-robot assigned danwinship and thockin Apr 19, 2024

k8s-ci-robot requested review from bowei and thockin April 19, 2024 16:44

k8s-ci-robot requested a review from shaneutt April 19, 2024 20:21

shaneutt approved these changes Apr 23, 2024

View reviewed changes

aojea reviewed Apr 24, 2024

View reviewed changes

apis/v1alpha3/backendtlspolicy_types.go Show resolved Hide resolved

aojea reviewed Apr 24, 2024

View reviewed changes

robscott mentioned this pull request Apr 24, 2024

Updating one last caCertRefs reference to caCertificateRefs #3018

Merged

aojea reviewed Apr 24, 2024

View reviewed changes

robscott closed this May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v1.1 API Review #2997

v1.1 API Review #2997

robscott commented Apr 19, 2024

k8s-ci-robot commented Apr 19, 2024

k8s-ci-robot commented Apr 19, 2024

shaneutt commented Apr 19, 2024

k8s-ci-robot commented Apr 23, 2024

aojea Apr 24, 2024 •

edited

Loading

robscott Apr 24, 2024

aojea Apr 24, 2024

aojea Apr 24, 2024

aojea Apr 24, 2024 •

edited

Loading

robscott Apr 24, 2024

aojea Apr 24, 2024

robscott Apr 24, 2024

aojea Apr 24, 2024

robscott Apr 24, 2024

mikemorris Apr 24, 2024

aojea Apr 24, 2024 •

edited

Loading

robscott Apr 24, 2024

aojea Apr 24, 2024

aojea Apr 24, 2024

robscott commented May 6, 2024

robscott commented May 8, 2024

	// +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`
	// +kubebuilder:validation:MinLength=1
	// +kubebuilder:validation:MaxLength=63
	type Namespace string

v1.1 API Review #2997

v1.1 API Review #2997

Conversation

robscott commented Apr 19, 2024

k8s-ci-robot commented Apr 19, 2024

k8s-ci-robot commented Apr 19, 2024

shaneutt commented Apr 19, 2024

k8s-ci-robot commented Apr 23, 2024

aojea Apr 24, 2024 • edited Loading

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

aojea Apr 24, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea Apr 24, 2024 • edited Loading

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 commented May 6, 2024

robscott commented May 8, 2024

aojea Apr 24, 2024 •

edited

Loading

aojea Apr 24, 2024 •

edited

Loading

aojea Apr 24, 2024 •

edited

Loading