Conflict: Gatekeeper allows constraint templates of same kind with different names

[RamyasreeChakka]

I was able to install below two constraint templates of different names but with same kind.
https://raw.githubusercontent.com/RamyasreeChakka/RegoPolicy/master/GateKeeperV3/KindTest/AllowedReposOne_template.yaml
https://raw.githubusercontent.com/RamyasreeChakka/RegoPolicy/master/GateKeeperV3/KindTest/AllowedReposTwo_template.yaml

As a constraint refers to constraint template kind and not the name, allowing above behavior is a conflict as Gatekeeper doesn't know which template to use.

During my testing, when I tried to install below constraint, Gatekeeper used the constraint template in AllowedReposOne_template.yaml

https://raw.githubusercontent.com/open-policy-agent/gatekeeper/master/demo/agilebank/constraints/prod_repo_is_openpolicyagent.yaml

[maxsmythe]

Thanks for this!

This is a tough one as K8s requires CRD names to equal the plural name + the group. This means CRDs potentially have the same issue. It appears they solve the problem by letting the resource get created and then scan for name conflicts:

apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"apiextensions.k8s.io/v1beta1","kind":"CustomResourceDefinition","metadata":{"annotations":{},"name":"thingsother.random.sh"},"spec":{"group":"random.sh","names":{"kind":"Thing","listKind":"ThingList","plural":"thingsother","singular":"thing"},"scope":"Cluster","version":"v1alpha1"}}
  creationTimestamp: "2019-06-28T04:15:42Z"
  generation: 1
  name: thingsother.random.sh
  resourceVersion: "28025"
  selfLink: /apis/apiextensions.k8s.io/v1beta1/customresourcedefinitions/thingsother.random.sh
  uid: 64ef3db9-995b-11e9-b058-42010af0014a
spec:
  additionalPrinterColumns:
  - JSONPath: .metadata.creationTimestamp
    description: |-
      CreationTimestamp is a timestamp representing the server time when this object was created. It is not guaranteed to be set in happens-before order across separate operations. Clients may not set this value. It is represented in RFC3339 form and is in UTC.

      Populated by the system. Read-only. Null for lists. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#metadata
    name: Age
    type: date
  group: random.sh
  names:
    kind: Thing
    listKind: ThingList
    plural: thingsother
    singular: thing
  scope: Cluster
  version: v1alpha1
  versions:
  - name: v1alpha1
    served: true
    storage: true
status:
  acceptedNames:
    kind: ""
    plural: thingsother
  conditions:
  - lastTransitionTime: "2019-06-28T04:15:42Z"
    message: '"ThingList" is already in use'
    reason: ListKindConflict
    status: "False"
    type: NamesAccepted
  - lastTransitionTime: "2019-06-28T04:15:42Z"
    message: not all names are accepted
    reason: NotAccepted
    status: "False"
    type: Established
  storedVersions:
  - v1alpha1

Because we can't know the relationship between every singular form and plural form, it is hard to force a singleton. Either we require the name to be equal to the plural, risking the Kind collision cited in this bug, or we require the name to be equal to the lower-spaced kind, risking CRDs being unable to be created due to conflicting plurals.

Either approach has a problem with an associated race condition.

Associating a Constraint with a ConstraintTemplate name is not a possibility, as CRDs define the schema for constraints.

In the near term it may be up to the admin to avoid collisions in the near term. This is a strong argument for sub-groups of constraints (e.g. myconstraint.mygroup.constraints.gatekeeper.sh) as they would make collisions less likely.

Longer term we can think about the failure symptoms of this scenario. It'd be good to have something that:

1. Ensures consistency during a conflict: that the active Rego and CRD are always equal.
2. Surfaces the problems this causes closer to the user. Status field at a minimum, prometheus metrics for alerts also good.

Ultimately this may be a good issue to raise with sig-api-machinery, though I'm not sure there's a logical solution to the one object, four names problem.

[maxsmythe]

One advantage of requiring CT name == CT Kind + group is that at least we know only one CT will make it to the API server. This makes it less ambiguous which version of the code is running.

I think we should do this, even if it diverges from the choices CRDs made.

[ritazh]

requiring CT name == CT Kind + group

Since versioning of CT and constraints is embedded in its name, adding this validation would mean when creating a new version of a CT, users will need to update the CT Kind and propagate that Kind change for all the constraints using the previous version of that CT.

[RamyasreeChakka]

I agree with @maxsmythe that we should make CT name == CT Kind + group to ensure a singleton constraint template in the cluster. If user doesn't want to edit the existing constraint template and want to create a new version of CT, then as @ritazh mentioned, user can edit the existing constraints to point the new one.

[maxsmythe]

I am a bit confused by @ritazh's comment. Versioning was always meant to be embedded in the name and the kind. Under the assumption that plural == singular == lowercase kind due to the version suffix (not spelled out anywhere, I had just assumed that's how things would work) this would always have been a limitation.

The versioning doc puts the version as part of the Kind, as that's how Constraints are able to differentiate which CT version they are associated with.

If we want to change how CT names are validated, we should do it before moving to beta, as it is a breaking change.

[ritazh]

Versioning was always meant to be embedded in the name and the kind. Under the assumption that plural == singular == lowercase kind due to the version suffix (not spelled out anywhere, I had just assumed that's how things would work) this would always have been a limitation.

My comment was to explicitly call out versioning of CT is embedded in the name and the kind. and the implications as this is currently not obvious in the versioning doc and should be explicitly called out. The doc states:

Constraint Templates have a strong need for a versioning solution. Ultimately the best solution seems to be to embed the version into the name.`

[ritazh]

+1 on adding this validation now

[maxsmythe]

@ritazh I recall you saying you were looking at this? Is that the case? If not, I can address.

[maxsmythe]

This should not have been closed until a new version of the constraint framework is included as a GK dependency.

[maxsmythe]

#169 would do this

[maxsmythe]

Fix published.