Further changes to Policy Attachment

[youngnick]

In a dedicated meeting today about Policy Attachment, some community members discussed where Policy Attachment is at (as of writing, #2813 is still in progress, but we hope to merge this in its current state soon, and then to make further changes based on the content of this discussion). Meeting notes are available at https://docs.google.com/document/d/1hF0JuhL-XNCqgmZ_MGP4tgneFxVWEz44niVhM0skBjg/edit?usp=sharing

#2813 splits GEP-713 into a Memorandum GEP, and then separate GEPs for Direct Policy Attachment and Inherited Policy Attachment.

Direct Policy Attachment is a strict subset of Policies that meet a set of rules (defined in GEP-2648 in the PR), and is intended to make us able to graduate restricted Policies included in the API (currently BackendTLSPolicy, but GEP-1619 is proposing to also introduce a BackendLoadBalancerPolicy, which also meets the Direct criteria).

Additionally, the requirements for Inherited Policy have been loosened such that defaults or overrides stanzas are no longer required. Instead, Policy owners can define how fields should work.

In our discussion, we agreed that we, the community, would like to consider the following four things (as suggested by @robscott, thanks Rob!):

• if defaults/overrides are not specified, defaulting is assumed
• allow plural 'targetRefs' instead of targetRefs with maxLen of ~16?
• copy kuadrant's concept of merge strategies - what we currently refer to as "direct" sets this to "none". Implementations can specify impl-specific strategies and maybe add that to a GW API library that powers tools like gwctl
• direct and inherited are no longer different forms of policy attachment, just different merge strategies

I'll make four separate comments outlining my thoughts on these four options, that we can use as starting points for further discussion, so we can keep all of this in the same place.

We also agreed that #2813 will have GEP-2648 and GEP-2649 changed to Provisional, since it seems likely from today's discussion that the distinction may be less relevant in the future.

[youngnick]

Discussion thread for:

If defaults or overrides are not specified, defaulting is assumed.

This means that, in the absence of structs named defaults or overrides, fields in a Policy should be assumed to be default values rather than override ones.

[youngnick]

This seems like a straightforward change that could be merged into the existing GEPs very quickly. +1 from me.

[robscott]

+1

[youngnick]

Discussion thread for:

Allow plural targetRefs instead of targetRef with maxLen of ~16?

[youngnick]

Helpfully, the API changes docs document a process for pluralizing a field like this: https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md#making-a-singular-field-plural

There's a pretty well-understood process for working through this, that I'd encourage Policy designers who want to use this to read.

However, the bigger issue here is that any Policy status will now also need to be namespaced by targetRef in addition to controllerName (or some other string to identify the controller instance).

This is very doable - we can use ListType: map for a list of PolicyAncestorStatus or similar struct, and then set the ListMapKeys to multiple fields in Kubebuilder and other tools. This will set the x-kubernetes-list-map-type and x-kubernetes-list-map-keys properties in the CRD's OpenAPI spec, which will have the apiserver treat the status correctly.

It seems likely that this may actually help a bit with number of apiserver status updates required, because some updates to the same object may be able to be coalesced more easily. We'll have to see how it turns out practically though, I think.

In any case, I think I'm a +1 on this overall, with the tradeoffs I can see:

• less updates and objects to think about when similar policy needs to be attached to multiple places
• greater blast radius for policy object changes
• May make it harder to build a conceptual model of what policy is doing what. Hard to tell for now though.

[robscott]

Yep, I think the big detail to get right here is status, but agree with your proposed approach for that, +1 on this direction.

[mikemorris]

For a specific example of how this is helpful even for the narrow scope of BackendTLSPolicy, consider how a typical blue/green deployment pattern with HTTPRoute typically involves splitting traffic off to a differently-named foo-v2 Service temporarily - to avoid disruption, this new Service would likely also need the same policy applied so the Gateway client knows to continue using TLS to connect.

This would allow temporarily adding an additional targetRef to the existing policy to enable this rather than creating a duplicate policy and risking config sprawl/zombie policies after consolidating back to the original name following a successful rollout.

I think the question of whether PolicyAncestorStatus is an appropriate way to represent the status of affected resources is an intersecting but separate consideration that should be discussed at more length over in #2923

[candita]

#2755 came to my attention recently. Another issue to consider with muitiple targetRefs.

[youngnick]

Discussion thread for:

Copy Kuadrant's concept of merge strategies - what we currently refer to as "direct" sets this to "none". Implementations can specify impl-specific strategies and maybe add that to a GW API library that powers tools like gwctl

and

direct and inherited are no longer different forms of policy attachment, just different merge strategies

[howardjohn]

That is already state of the art for kubernetes objects and has a variety of tooling (kistomize, kubectl, etc). What about gateway requires a different solution?

[howardjohn]

Also should we be using the CRD directly or using the API server openapi discovery?

Last time I suggested reading CRDs for something api-machinery folks recommended against it

[robscott]

That is already state of the art for kubernetes objects and has a variety of tooling

@howardjohn What are you referring to in this context?

[howardjohn]

Putting merging method indicators into OpenAPI schemas and consuming them in tooling like kubectl/kustomize/etc

[mikemorris]

+1 to the merge strategy being a status field to communicate to a user or generic tool such as gwctl how this policy should be expected to resolve, and typically configured by the policy implementation author in the CRD directly.

If some implementations allow selection of merge strategies on certain policies, they could express the default behavior in this same way, and additionally allow an end user to override the default strategy in something like a spec.policyMeta.strategy field, which would then be reflected in the status.

[mikemorris]

Spinning out my comment from #2813 (comment), I'd like to propose that targeting both a resource as a whole and a subsection of the same resource by sectionName be disallowed for Direct Policies (or policies with merge strategy none if that proposal advances), and only supported when a compositional merge strategy is defined (either by the policy implementer in the CRD, or by an end-user author in the policy spec if supported).

Simultaneously targeting separate non-overlapping subsections of a resource should be allowed for Direct Policies (or with merge strategy none), but overlapping targets which would require some sort of inheritance logic should instead be rejected and set a Conflicted status condition.