feat: add gcp affinity annotation parsing#343
feat: add gcp affinity annotation parsing#343chakravardhan wants to merge 2 commits intokubernetes-sigs:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chakravardhan The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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. |
| // affinityFeature parses the ingress-nginx affinity annotations and populates | ||
| // the ProviderSpecificServiceIR.Gce.SessionAffinity. | ||
| func affinityFeature(_ []networkingv1.Ingress, _ map[types.NamespacedName]map[string]int32, ir *providerir.ProviderIR) field.ErrorList { | ||
| var errs field.ErrorList |
There was a problem hiding this comment.
I think I might havesaid this in another PR, but can we avoid using the ProviderSpecificServiceIR? Its only there for backcompat reasons. For example, what if another emitter also wants to implement sessionaffinity? Would they have to read ProviderSpecificServiceIR.Gce?
There was a problem hiding this comment.
Agreed @Stevenjin8, Should we move the SessionAffinity outside ProviderSpecificServiceIR into a common ServiceContext variable? wdyt?
b25dd60 to
31d25fb
Compare
31d25fb to
7233168
Compare
|
/ok-to-test |
| // has a dedicated field for each provider to specify their extension features | ||
| // on Service. | ||
| type ServiceContext struct { | ||
| ProviderSpecificIR ProviderSpecificServiceIR |
There was a problem hiding this comment.
Can we avoid having provider specific fields in the IR. The whole point is that it isn't provider specific
|
PR needs rebase. 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. |
|
@chakravardhan: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
DamianSawicki
left a comment
There was a problem hiding this comment.
- Thanks for taking care of it! Since 1.0 is now official, it would be great to get this merged asap.
- It may be outside the scope of this particular PR, but
nginx.ingress.kubernetes.io/session-cookie-namecan be translated with GCPHTTP_COOKIEsession affinity type instead ofGENERATED_COOKIE. It requires usingGCPTrafficDistributionPolicyinstead ofGCPBackendPolicyto configure session affinity (https://docs.cloud.google.com/kubernetes-engine/docs/how-to/configure-gateway-resources#expanded-session-affinity).
Actually, is this still relevant or was it superseded by #394? |
Yes, the current PR is superseded by #394. Its not relevant anymore unless we pivot the PR to produce |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces support for parsing Nginx Ingress session affinity annotations and translating them to GKE Gateway API resources.
Specifically, this PR:
nginx.ingress.kubernetes.io/affinityandnginx.ingress.kubernetes.io/session-cookie-max-agein theingress-nginxprovider.GCPBackendPolicywithSessionAffinityConfigset toGENERATED_COOKIEwhenaffinity: "cookie"is detected and the tool is run with--emitters=gce.session-cookie-max-agetoCookieTTLSecwith strict bound-checking[0, 1209600](14 days) to align with GCP's API constraints, surfacing validation errors early for invalid configurations.Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: