Cartographer allows non unique resources.

[sfzylad]

I've noticed Cartographer allows the stamped objects to share the same name within the same namespace and within the same supply chain which in case of Runnables causes objects to be stamped indefinitely.

To reproduce the problem:

• create a template stamping a runanble with fixed name, let runnable to kick off some action (I encountered the problem while stamping Tekton's PipelineRuns)
• create a Supply Chain that uses the template from the previous step
• add 2 or more workload objects pointing on the supply chain from the previous step

The image I use: projectcartographer/cartographer@sha256:16c635ebfc498ee9fe096bacc7683dd5762e93cf4ef22be382e1697cef4af2d4

Expected behaviour: I would expect some sort of validation when adding new workloads or at least information about collisions. Instead the following log message is currently emitted:

{"level":"error","ts":1647631525.5404093,"logger":"controller.runnable-service","msg":"Reconciler error","name":"a-pipeline-runnable","namespace":"default","error":"failed to update status for runnable: Operation cannot be fulfilled on runnables.carto.run \"a-pipeline-runnable\": the object has been modified; please apply your changes to the latest version and try again","stacktrace":"sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem\n\tsigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:266\nsigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2\n\tsigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:227"}

[github-actions[bot]]

Hello @sfzylad! Welcome to the Cartographer community and thank you for opening your first issue, we appreciate your contributions

[cirocosta]

hi @sfzylad ! thanks for reporting - indeed, the controller currently does not perform any validation in that form. e.g., a minimal example to illustrate it:

1. create a template that stamps out a configmap named after workload-$(workload.metadata.name)$

apiVersion: carto.run/v1alpha1
kind: ClusterTemplate
metadata:
  name: test-basic
spec:
  params:
    - name: who
      default: no-one
  template:
    apiVersion: v1
    kind: ConfigMap
    metadata:
      name: workload-$(workload.metadata.name)$
    data: $(params)$

2. create a clustersupplychain that makes use of this template twice

apiVersion: carto.run/v1alpha1
kind: ClusterSupplyChain
metadata:
  name: test-basic
spec:
  selector:
    test-basic: test-basic

  resources:
    - name: first
      templateRef:
        kind: ClusterTemplate
        name: test-basic
      params:
        - name: who
          value: first

    - name: second
      templateRef:
        kind: ClusterTemplate
        name: test-basic
      params:
        - name: who
          value: second

3. create a workload that matches that supply chain

apiVersion: carto.run/v1alpha1
kind: Workload
metadata:
  name: test-basic
  labels:
    test-basic: test-basic
spec: {}

once the reconciliation occurs, we can observe that the workload indeed stamps out the same configmap as if there were two different resources

[
  {
    "name": "first",
    "stampedRef": {
      "apiVersion": "v1",
      "kind": "ConfigMap",
      "name": "workload-test-basic",
      "namespace": "default"
    },
    "templateRef": {
      "apiVersion": "carto.run/v1alpha1",
      "kind": "ClusterTemplate",
      "name": "test-basic"
    }
  },
  {
    "name": "second",
    "stampedRef": {
      "apiVersion": "v1",
      "kind": "ConfigMap",
      "name": "workload-test-basic",
      "namespace": "default"
    },
    "templateRef": {
      "apiVersion": "carto.run/v1alpha1",
      "kind": "ClusterTemplate",
      "name": "test-basic"
    }
  }
]

although the last resource is the one that "wins the race":

$ kubectl get cm workload-test-basic -o jsonpath={.data}
{"who":"second"}

[cirocosta]

Expected behaviour: I would expect some sort of validation when adding new workloads or at least information about collisions.

makes sense to me! agree that it's a good validation to have in place - I tried thinking of at least a case against having the validation ... but can't think of anything 👍

[squeedee]

IPM suggestion:

Validation here is quite hard mostly because of ytt, but the suggestion is:

Before applying a resource, if it already exists but belongs to another workload, or the same workload but a different resource-name, then provide and error and a status condition explaining the issue.