-
Notifications
You must be signed in to change notification settings - Fork 585
DO NOT MERGE: initial v1.4.0 feedback #4042
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
base: release-1.3
Are you sure you want to change the base?
DO NOT MERGE: initial v1.4.0 feedback #4042
Conversation
…s#3729) * add mesh confomance tests structure and first test * move mesh tests to mesh folder
kubernetes-sigs#3777) * add redirect-port and redirect scheme mesh tests * mesh tests for path host and status redirect
Signed-off-by: Norwin Schnyder <[email protected]>
* Add conformance report for Contour 1.31.0 Supports gateway API 1.2.1 Signed-off-by: Sunjay Bhatia <[email protected]> * update implementations.md Signed-off-by: Sunjay Bhatia <[email protected]> --------- Signed-off-by: Sunjay Bhatia <[email protected]>
…3781) * add csm mesh to implementations.md * fix * fix1 * Update implementations.md Co-authored-by: Rob Scott <[email protected]> --------- Co-authored-by: Rob Scott <[email protected]>
…igs#3564) Add feature name processing to remove redundant prefixes and improve readability in conformance comparison tables. Changes include: - Remove "HTTPRoute" and "Gateway" prefixes from feature names - Split camelCase words into space-separated words - Add process_feature_name() function for consistent text processing - Update generate_profiles_report() to use processed feature names This makes the conformance reports easier to read
Signed-off-by: bitliu <[email protected]>
The report is generated by Cilium GHA CI https://github.com/cilium/cilium/actions/runs/15111523465 Signed-off-by: Tam Mach <[email protected]>
…igs#3809) * conformance: Add Airlock Microgateway report for v1.3.0 Signed-off-by: Norwin Schnyder <[email protected]> * add trailing new lines Signed-off-by: Norwin Schnyder <[email protected]> --------- Signed-off-by: Norwin Schnyder <[email protected]>
…#3811) Signed-off-by: Norwin Schnyder <[email protected]>
* conformance: add Hook in ConformanceTestSuite * update comment Signed-off-by: zirain <[email protected]> * add Hook in ConformanceOptions Signed-off-by: zirain <[email protected]> --------- Signed-off-by: zirain <[email protected]>
* add mesh conformance for request header modifier * address feedback
* docs: Add v1.3 conformance report table Signed-off-by: Norwin Schnyder <[email protected]> * Automatically update conformance table files Signed-off-by: Norwin Schnyder <[email protected]> --------- Signed-off-by: Norwin Schnyder <[email protected]>
Signed-off-by: Shane Utt <[email protected]>
…-sigs#3827) * add httproute weights mesh conformance tests * gofmt
…#2712) * draft dns configuration for gateway API GEP-2627 minor tweaks Update geps/gep-2627/index.md Co-authored-by: Candace Holman <[email protected]> Update geps/gep-2627/index.md Co-authored-by: Candace Holman <[email protected]> Update geps/gep-2627/index.md Co-authored-by: Candace Holman <[email protected]> * changes post review * rewording * Update geps/gep-2627/index.md Co-authored-by: Nick Young <[email protected]> * Update geps/gep-2627/index.md Co-authored-by: Nick Young <[email protected]> * Update geps/gep-2627/index.md Co-authored-by: Nick Young <[email protected]> * Update geps/gep-2627/index.md Co-authored-by: Nick Young <[email protected]> * Update geps/gep-2627/index.md Co-authored-by: Shane Utt <[email protected]> * Update geps/gep-2627/index.md Co-authored-by: Shane Utt <[email protected]> * Update geps/gep-2627/index.md Co-authored-by: Shane Utt <[email protected]> * Update geps/gep-2627/index.md Co-authored-by: Shane Utt <[email protected]> * Update geps/gep-2627/index.md Co-authored-by: Shane Utt <[email protected]> * Update geps/gep-2627/index.md Co-authored-by: Shane Utt <[email protected]> * Update geps/gep-2627/index.md Co-authored-by: Shane Utt <[email protected]> * Update geps/gep-2627/index.md Co-authored-by: Shane Utt <[email protected]> * minor tweaks to the text and link to kuadrant as an example of a DNSPolicy type API * fix new line * move kuadrant to a reference --------- Co-authored-by: Candace Holman <[email protected]> Co-authored-by: Nick Young <[email protected]> Co-authored-by: Shane Utt <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
* Kubevernor conformance report * Adding Kubvernor section to implementations section
Adding conformance report for the v2.0 NGINX Gateway Fabric release. Now supporting Gateway API v1.3.
* Update naming * fix failures
Skipping CI for Draft Pull Request. |
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-sigs/prow repository. |
Keywords which can automatically close issues and at(@) or hashtag(#) mentions are not allowed in commit messages. The list of commits with invalid commit messages:
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-sigs/prow repository. I understand the commands that are listed here. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
// +kubebuilder:validation:Minimum=1 | ||
// +kubebuilder:validation:Maximum=65535 | ||
// <gateway:experimental> | ||
Port PortNumber `json:"port"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we had this questions in the review of GIE, right now everything is HTTP1 and HTTP2 that are TCP , but eventually we'll get to http3 that is UDP :/ and this will require to discriminate by port ... and then the key need to be Port+proto ... just commenting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handling HTTP3 correctly will require a lot of changes , I guess we should add this to the pile. Thanks!
// FrontendTLSConfig specifies frontend tls configuration for gateway. | ||
type FrontendTLSConfig struct { | ||
// Default specifies the default client certificate validation configuration | ||
// for all Listeners handling HTTPS traffic, unless a per-port configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for my understanding, this field is required, so the unless a per-port configuration
means if a Port configuration exists it overrides the default, but is not that this is a kind of oneOf
API, you always need to define a default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @kl52752
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I don't think we defined what happens if you don't set a default and do set a per-port config. We either need to say you can't do that, or define what happens when you do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aojea yes :)
After all review rounds we agreeded that per port override rules might be confusing. Therefore, a default option is required and will apply to all Listeners with HTTPS protocol. If someone needs to divers frontend TLS config then it can be overridden for a specific port if needed.
// | ||
// References to a resource in a different namespace are invalid UNLESS there | ||
// is a ReferenceGrant in the target namespace that allows the certificate | ||
// to be attached. If a ReferenceGrant does not allow this reference, the | ||
// "ResolvedRefs" condition MUST be set to False for this listener with the | ||
// "RefNotPermitted" reason. | ||
// | ||
// +required | ||
// +listType=atomic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL default is atomic kubernetes/kubernetes#121759 , I'm educating myself here, is that right @thockin ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep - kube-api-linter is requiring us to specify in all cases, even though this is the default. It's easy to forget to set this and accidentally end up with atomic
, so I think this is a net positive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, explicit > implicit :)
// if relevant.) | ||
// | ||
// </gateway:util:excludeFromCRD> | ||
// | ||
// +listType=map | ||
// +listMapKey=type | ||
// +kubebuilder:validation:MaxItems=8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question, I see in some places you require one item
gateway-api/apis/v1/shared_types.go
Lines 510 to 515 in 8ec8a83
// +listType=map | |
// +listMapKey=type | |
// +kubebuilder:validation:MinItems=1 | |
// +kubebuilder:validation:MaxItems=8 | |
// +required | |
Conditions []metav1.Condition `json:"conditions,omitempty"` |
is this allowed to have empty or you always want to have at least 1?
// +kubebuilder:validation:MinItems=1
// If this list is empty, then the following headers must be sent: | ||
// | ||
// - `Authorization` | ||
// - `Location` | ||
// - `Proxy-Authenticate` | ||
// - `Set-Cookie` | ||
// - `WWW-Authenticate` | ||
// | ||
// If the list has entries, only those entries must be sent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not default the list to these if empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This brings up, to me, an ambiguity in the spec.
If the list is empty, does the proxy need to send exactly these 5 headers and nothing else? Or at least these 5, and the rest is implementation specific?
If the former, how can I say "send all headers", which is (IIUC) the most common usage with grpc?
If the latter, why not just default to all headers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @htuch @youngnick
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I based this on the protos at https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/ext_authz/v3/ext_authz.proto#envoy-v3-api-msg-extensions-filters-http-ext-authz-v3-extauthz, but on rereading, them, I've made a mistake.
The gRPC header config should be as @howardjohn says - if unspecified or empty, then all headers should be sent. Those five headers are relevant for the HTTP external servers, not the gRPC one.
I'll make a PR to fix this.
This is a PR to gather some early initial feedback on the upcoming changes for #3756. It should not be merged.