feat: Implement standard CORS support#303
feat: Implement standard CORS support#303k8s-ci-robot merged 16 commits intokubernetes-sigs:mainfrom
Conversation
|
Welcome @chakravardhan! |
|
Hi @chakravardhan. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
| "k8s.io/apimachinery/pkg/util/validation/field" | ||
| "k8s.io/utils/ptr" | ||
| gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" | ||
| ) | ||
|
|
||
| func corsFeature(_ []networkingv1.Ingress, _ map[types.NamespacedName]map[string]int32, ir *providerir.ProviderIR) field.ErrorList { |
There was a problem hiding this comment.
Sorry, there isn't a lot of precedent for this and I haven't had time to update the docs, but the Cors filter is currently experimental (https://gateway-api.sigs.k8s.io/geps/gep-1767/), so it seems a bit early to add it to ingress2gateway.
There was a problem hiding this comment.
We could implement it now, but the flow would look a little different. You would follow the flow of something like #288 where we create the IR in the provider and then populate the fields in the common emitter. We would also have to add a feature flag/argument to the common emitter --allow-alpha-gw-api
There was a problem hiding this comment.
Also, take a look at this PR: #305 that has the structure I'm looking for. Also, I'm going to ask you to sign yourself up as a codeowner of the GCE emitter if you want to add code there.
There was a problem hiding this comment.
I have implemented the --allow-alpha-gw-api flag in the root command and wired it through to the CommonEmitter.
By default (flag false), the CommonEmitter now actively filters out experimental features like CORS filters from the IR before it reaches the specific emitter.
When --allow-alpha-gw-api is passed, CORS filters are preserved and output.
There was a problem hiding this comment.
Ack. I will check PR #305 and follow up with a separate PR to add myself to the OWNERS file for the GCE emitter components once this infrastructure lands.
There was a problem hiding this comment.
@chakravardhan could you look at #305? The flow right now is add the CORSFilter in the provider and strip it out in the CommonEmitter before it gets sent to the final emitter. But what if the final emitter is Envoy Gateway and they want to implement it using their CORS filter. Also, structuring it like #305 will help us in the future because we will know what was parsed and what wasn't.
There was a problem hiding this comment.
Understood, Adopting a structure like #305. Let me know if this looks good
There was a problem hiding this comment.
@chakravardhan. Thanks, this is almost what I'm looking for, but it would be ideal to not touch ProviderIR. See applyBodySizeToEmitterIR here https://github.com/kubernetes-sigs/ingress2gateway/pull/305/files
There was a problem hiding this comment.
I've modified code similar to the mentioned PR above by adding this method applyCorsToEmitterIR. Kindly check this approach and let us know if this aligns well. Thanks! @Stevenjin8
dba4e03 to
153ea57
Compare
bexxmodd
left a comment
There was a problem hiding this comment.
Left one comment regarding flag value. The rest LGTM
Co-authored-by: Steven Jin <stevenjin8@gmail.com>
1255b61 to
ff083d3
Compare
ff083d3 to
20e35cb
Compare
Fixed now! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bexxmodd, chakravardhan, kkk777-7, Stevenjin8 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
LGTM, thanks! /lgtm |
* feat: Implement PR1 Infrastructure and Standard CORS support * refactor(gce): rename upsell to update and fix duplicate * Address review comments: delete gce_converter, add --allow-alpha-gw-api flag * fix(gce): unconditionally set GatewayClassName to gke-l7-global-external-managed * fix(ingressnginx): improve cors-max-age parsing validation * feat: make GCE GatewayClassName configurable via flag * Update pkg/i2gw/emitter_intermediate/intermediate_representation.go Co-authored-by: Steven Jin <stevenjin8@gmail.com> * Fix build error by removing usage of removed Gce field * feat(gce): implement conditional logic for --gce-gateway-class-name * refactor: Move CORS filtering from common emitter to standard emitter * chore: revert GCE and GatewayClassName changes to focus on CORS * Cleanup: remove implicit emitter flag association * Address PR comments: Refactor CORS to CommonEmitter and cleanup * Revert standard.go to match main branch * Revert conversion.go to match main branch * Fix gosec lint errors from cors changes --------- Co-authored-by: Steven Jin <stevenjin8@gmail.com>
What type of PR is this?
/kind feature
What this PR does / why we need it:
This is the first PR in a stacked series. This PR focuses on the internal infrastructure and providing robust, standard-compliant CORS support. It enables users to migrate complex CORS configurations seamlessly to
HTTPRoutefilters.Supported Annotations & Mappings
This PR adds a conversion mapping for the following Nginx annotations to Gateway API
HTTPRouteFilterCORSfields:HTTPRoute.filters.cors)nginx.ingress.kubernetes.io/enable-cors"true"to enable.cors-allow-originallowOrigins*cors-allow-methodsallowMethodsGET, PUT, POST, DELETE, PATCH, OPTIONScors-allow-headersallowHeaderscors-expose-headersexposeHeaderscors-allow-credentialsallowCredentialstrue(Matches Nginx default)cors-max-agemaxAge1728000(Matches Nginx default)New flags -
Flag: --allow-experimental-gw-api
Purpose: Controls whether Experimental Gateway API fields are included in the output.
Behavior: Defaults to false. Must be explicitly set to generated CORS configuration, as it is considered an advanced/experimental feature in this tool context.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: