Requiressync annotation allows GVKs with multiple versions

[anlandu]

Is the schema for allowing multiple versions listed in the same GVK (example) set in stone? I think it makes unmarshaling the json into the gvk type a little less convenient. I'm happy to open a PR to change the templates in the library if this change is agreed upon. But I'm also happy to implement custom unmarshaling logic for my use case if the existing schema is a necessity.

[anlandu]

@apeabody

[apeabody]

Hi @anlandu! - Can you share an example of the potential change you have in mind?

[anlandu]

Sure! Just switching it so that each version is its own GVK, like so:

      "[
        [
          {
            "groups":["extensions"],
            "versions": ["v1beta1"],
            "kinds": ["Ingress"]
          },
          {
            "group": ["networking.k8s.io"],
            "version": ["v1beta1"],
            "kind": ["Ingress"]
          },
          {
            "group": ["networking.k8s.io"],
            "version": ["v1"],
            "kind": ["Ingress"]
          }
        ]
      ]"

[sozercan]

it looks like we are naming it both singular and plural (for example, groups vs group, and the same for others). Can we align on one of these?

[anlandu]

Good point! Singular aligns better with the type definition, I can include that in my fix of this issue if @apeabody or others have no objections to either change?

[apeabody]

Thanks for the sample @anlandu!

@ekitson @maxsmythe - Thoughts on the denormalized format?

[ekitson]

I think my initial preference was for a format like this, but I think Max preferred the compact format. My preference is not strong either way so I'll defer to him. If we do update the schema to singular fields, we should also drop the brackets.

[apeabody]

Thanks @ekitson! Good call on the dropping the brackets if moving to singular, should make unmarshaling even easier.

@anlandu I'm also comfortable with the changes, unless @maxsmythe has a concern. Sounds like the final version woudl be something like this?

  "[
    [
      {
        "group": "extensions",
        "version": "v1beta1",
        "kind": "Ingress"
      },
      {
        "group": "networking.k8s.io",
        "version": "v1beta1",
        "kind": "Ingress"
      },
      {
        "group": "networking.k8s.io",
        "version": "v1",
        "kind": "Ingress"
      }
    ]
  ]"

[maxsmythe]

I'm not sure we want to trade in size efficiency for marshaling convenience here.

The current annotation format is consistent with the match schema for GVK as well as RBAC resource matching and other K8s primitives.

[maxsmythe]

To be clear, my main objection is that this shifts complexity from writing code to interpret the annotation (which we can do one time, and write a library function for), to authors of constraint templates, requiring more boiler plate than necessary.

There is also the maximum annotation size to consider, though it's fairly large IIRC

[anlandu]

Thank you for your response, @maxsmythe, that's great to know! Is the plural vs singular key still a thing we would want to converge on (sounds like plural would make more sense now)? Additionally, as I start thinking about implementing the unmarshaling, is there a doc I (and CT/constraint authors) could refer to on best practices/rules for this schema? I'm happy to start writing it as well if it doesn't exist yet. For instance, I'm assuming there can be plurals in any/all of the fields and I should treat every combination as valid. If so, when is keeping objects separate best practice and when should they be combined? For example (let me know if this isn't the RBAC resource match schema you were talking about), the constraint here has separate items for ClusterRoleBinding and RoleBinding, rather than combining them like so:

      - apiGroups: ["rbac.authorization.k8s.io"]
        kinds: ["ClusterRoleBinding", "RoleBinding"]

Or, in the example discussed here, having multiple versions definitely seems more intuitive than multiple groups, but could I also expect to see multiple groups + treat it as equivalent? i.e.

      "[
        [
          {
            "groups": ["extensions", "networking.k8s.io"],
            "versions": ["v1beta1"],
            "kinds": ["Ingress"]
          },
          {
            "groups": ["networking.k8s.io"],
            "versions": ["v1"],
            "kinds": ["Ingress"]
          }
        ]
      ]"

Thank you so much and sorry this comment is long!!

[maxsmythe]

No worries, thank you for engaging!

The nerdiest (but briefest) explanation is that for each object, the cross product of all fields is the set of GVKs that can be required. That's a bit dense though, so for example:

[
  [
     {
       "groups": ["g1", "g2"],
       "versions": ["v1", "v2"],
       "kinds": ["k1", "k2"],
     }
  ]
]

Would expand to: "must have one of [k1.g1/v1, k1.g1/v2, k1.g2/v1, k1.g2/v2, k2.g1/v1, k2.g1/v2, k2.g2/v1, k2.g2/v2] to run this constraint" (that was a lot to type!).

Of course, it doesn't make sense to always take the cross-product, which is where the inner-list comes in:

[
  [
     {
       "groups": [""],
       "versions": ["v1beta1"],
       "kinds": ["NetworkPolicy"],
     },
     {
       "groups": ["networking.k8s.io"],
       "versions": ["v1beta1", v1],
       "kinds": ["NetworkPolicy"],
     },
  ]
]

Means: "must have one of [NetworkPolicy/v1beta, NetworkPolicy.networking.k8s.io/v1beta1, NetworkPolicy.networking.k8s.io/v1beta1] to run this constraint" (note there is no NetworkPolicy/v1, historically when resources have changed groups, not all groups provided all versions)

Finally, sometimes constraints require multiple resources to run, this is where the outer list comes in:

[
  [
     {
       "groups": [""],
       "versions": ["v1beta1"],
       "kinds": ["NetworkPolicy"],
     },
     {
       "groups": ["networking.k8s.io"],
       "versions": ["v1beta1", v1],
       "kinds": ["NetworkPolicy"],
     },
  ],
  [
     {
       "groups": [""],
       "versions": ["v1"],
       "kinds": ["Namespace"],
     }
  ]
]

Which means: "must have one of [NetworkPolicy/v1beta, NetworkPolicy.networking.k8s.io/v1beta1, NetworkPolicy.networking.k8s.io/v1beta1] AND one of [Namespace/v1] to run this constraint"

I agree it's not super human-readable unless you have documentation on how to interpret the fields, so if we're open to changing it and there is a change that makes this more self-documenting (especially the nested lists), I'd be for it.

The main intent behind this annotation is to let users express sync requirements for their constraints in a way that provides enough information to know when the constraint should run, and also that provides alternative equivalent resources, since sometimes different k8s versions have different GVKs available. With this AND/OR structure, there should be enough information for an automation system to use the discovery API to set up syncs.

[maxsmythe]

Best practice would be to use the breakdown that is legible and doesn't produce invalid GVK combos (I have no strong opinions on deduplication, just don't want to force a lot of copy/paste).

WRT Ingress example... exactly. I tried to show that with the 2nd example in my above reply.

[anlandu]

That makes sense! I'll PR a doc for that. It sounds like once GK implements validation that the config was properly updated this unmarshaling logic will end up in your lib, but for the time being I'll just put it in the Azure policy addon repo

[anlandu]

I'm assuming I'm still good to PR the change to make the keys plural everywhere?

[apeabody]

I'm assuming I'm still good to PR the change to make the keys plural everywhere?

Hi @anlandu! - I would agree with standardizing on plurals, I probably just missed updating the second set in https://github.com/open-policy-agent/gatekeeper-library/blob/master/library/general/uniqueingresshost/template.yaml#L17.

A future idea would be to validate the key names in the annotation as part of the require-sync check: https://github.com/open-policy-agent/gatekeeper-library/blob/master/scripts/require-sync/main.go#L98

[anlandu]

Thanks all!

[ritazh]

We don’t have requiresync enforced on GK yet, but with the bug in 1.0.1, once we do introduce it, whats the experience there since group won’t be correct? How to make sure its fault tolerant? and how do we mark a policy version as bad or deprecated? @maxsmythe @sozercan thoughts?