feat(gateway-api): customize gateway deployments via class annotations

[svrakitin]

Closes #46594

This PR enhances custom GatewayClass'es and allows a per-class customization. Currently we have templates and service types hardcoded for each controller type instead.

• Support inject.istio.io/templates annotation on GatewayClass to pick a custom template from the istiod ConfigMap.
  • Uses controller's default if unset.
  • Supports just a single template for now. No template chaining like for sidecars.
• Support networking.istio.io/service-type annotation on GatewayClass to override controller's default. This annotation is already supported for Gateway to perform a per-gateway override.
  • Uses controller's default if unset.

[istio-policy-bot]

😊 Welcome @svrakitin! This is either your first contribution to the Istio istio repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

[istio-testing]

Hi @svrakitin. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

[keithmattix]

This looks reasonable to me
/ok-to-test

[svrakitin]

/retest

EDIT: There were some flaky tests related to Ambient.

[howardjohn]

sorry I didn't notice this until now, will take a look soon

[howardjohn]

So I like the idea, I have some concerns on the implementation.

First is GatewayClass in general. I personally don't like the idea of GatewayClass at all and have pushed in the API to remove it and/or move things way from it. But its still there and going GA so my opinion there probably doesn't matter much. In particular, I hate parametersRef - but, again, its there and going GA. parametersRef is the "right" way to do this in the API. But I have some cognitive dissonance here, as the API is bad IMO... not sure how to rationalize those

On the injection template: if we allow it in the class, we should probably allow it per-Gateway as well. Should be easy to add that.

However, I am more concerned about the idea of providing arbitrary templates. Unlike classic sidecar pod injection, this form of templating has a subtle but critical security risk I call "YAML injection". Basically, if you write a template like type: {{ index .Annotations "networking.istio.io/service-type" }} (obviously as part of a large template), this may look innocent and fine. But if a user makes the annotation something like

networking.istio.io/service-type: |
  ClusterIP
  ---
  kind: ClusterRole
  name: give-everyone-admin-permission
  ---

You are going to have a bad time. Sidecar injection mitigates this since its always generating a Patch on a Pod object, so its impossible to create new objects. Technically, this is the template-writers' (who is the mesh admin) fault -- but its really easy to make the mistake, and the results are pretty bad.
Perhaps we add some mitigation that it can only create certain objects or something?
This is why we have quote everywhere, by the way.

The other concern is around stability. There is (slow) progress to make some of this upstream (kubernetes-sigs/gateway-api#1757), and the API for templates may change over time (for example, .DeploymentName -- we could decide it needs to be removed for some reason or other, or changed, etc). If we do this I would want to add some caveat that it is experimental.

WDYT?

[svrakitin]

Thanks for the detailed explanation @howardjohn

Sounds good, you definitely have much more context than I do! 😄

We can avoid touching GatewayClass at all and support override per-gateway only.

Do we want to introduce something like gateway.istio.io/pod-template instead, which would be applied on top of the Deployment's existing spec.template. This should be safer and cover most of the use cases?

I would then have something like:

gateway.istio.io/pod-template: |
  spec:
    nodeSelector: {}
    affinity: {}
    tolerations: []

This would use a strategic merge and hide kube-gateway template as an implementation detail.

Or we have a new CRD like GatewayConfig, which would have these attributes and use label selector similar to ProxyConfig. This is better as we can limit what users can actually do with that.

[howardjohn]

pod-template is kind of nice since then you could also use the same templates for sidecars. GatewayConfig is nice but will be a big effort to get in - the project is very conservative around adding new resources.

Let me solicit some feedback from others and get back to you

[svrakitin]

@howardjohn Thank you!

[istio-policy-bot]

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2023-08-23. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

[ofirshaya]

any progress on this? related to this
it is impossible for us to use istio-gateway-api, we are using a global template that injects .Values.global.proxy.lifecycle and we get field not declared in schema
I thought about using a custom template but as it is stated in this issue- not possible because of the nature of the kube-gateway file.
@svrakitin @howardjohn