Conversation
|
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. |
|
|
||
| type EmitterConf struct { | ||
| // TODO: add fields as needed. | ||
| ProviderSpecificFlags map[string]map[string]string |
There was a problem hiding this comment.
I realize that a lot of the flags weren't built with emitters in mind, but is there any way to not have ProviderSpecificFlags in the Emitter configuration.
There was a problem hiding this comment.
I've completely removed ProviderSpecificFlags from the EmitterConf.
To solve the --gce-gateway-class-name issue, I moved the flag registration from the GCE Emitter to the GCE Provider. The provider now extracts this flag directly from its own configuration (ProviderConf) and applies the GatewayClass override into the Intermediate Representation (IR) during the initial convertToIR translation phase instead.
This keeps the Emitters perfectly clean and completely agnostic of any provider-specific CLI flags
0f71686 to
a6c5cb0
Compare
| var gatewayClassName string | ||
| if c.conf != nil && c.conf.ProviderSpecificFlags != nil { | ||
| if flags, ok := c.conf.ProviderSpecificFlags["gce"]; ok { | ||
| if val, ok := flags[GatewayClassNameFlag]; ok && val != "" { |
There was a problem hiding this comment.
So if no value is supplied for the flag it will be a no-op?
There was a problem hiding this comment.
I agree with Beka, can we do some validation here to make sure that the flag value is valid?
If the value is non-nil, it should be one of the valid class names here: https://docs.cloud.google.com/kubernetes-engine/docs/how-to/gatewayclass-capabilities (not an empty string either).
I'm not totally sure if we should be doing validation further up the stack for ProviderSpecificFlags, but it doesn't look like any other provider is doing that right now.
There was a problem hiding this comment.
Agreed, Added a validation logic in ir_converter.go right where we first extract the flag payload from the ProviderSpecificFlags map.
| var gatewayClassName string | ||
| if c.conf != nil && c.conf.ProviderSpecificFlags != nil { | ||
| if flags, ok := c.conf.ProviderSpecificFlags["gce"]; ok { | ||
| if val, ok := flags[GatewayClassNameFlag]; ok && val != "" { |
There was a problem hiding this comment.
I agree with Beka, can we do some validation here to make sure that the flag value is valid?
If the value is non-nil, it should be one of the valid class names here: https://docs.cloud.google.com/kubernetes-engine/docs/how-to/gatewayclass-capabilities (not an empty string either).
I'm not totally sure if we should be doing validation further up the stack for ProviderSpecificFlags, but it doesn't look like any other provider is doing that right now.
|
/lgtm |
b07644a to
82fda4f
Compare
|
/lgtm |
@LiorLieberman , Can I get approval on this PR? Thanks! |
|
/approve |
|
/ok-to-test |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bexxmodd, chakravardhan, spencerhance, 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 |
* feat(gce): implement GatewayClassName override logic * style: gofmt changes * refactor: Move GatewayClassNameFlag from Emitter to Provider * Add test case for gatewayclassname flag * feat: Add validation for gce-gateway-class-name flag * fix: remove duplicate validation block * refactor: simplify gateway-class-name flag validation block * fix(gce): fixing converter test according to upstream changes
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR adds support for a new provider-specific flag
--gce-gateway-class-nameto the GCE emitter.Currently, the GCE provider defaults to standard GatewayClasses (e.g.,
gke-l7-global-external-managedorgke-l7-rilb) based on the input Ingress class. This change allows users to override the GatewayClassName field in all generated Gateways when using thegceemitter, which is useful for migration testing or custom Gateway deployments.If the flag is unset, the emitter preserves the default classes determined by the provider.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: