prune cluster scoped resources does not work when specify namespace in app config

[kevin-namba]

What happened: prune cluster scoped resources (such as clusterrole) does not work when specify namespace in app config

What you expected to happen:

How to reproduce it:

Environment:

• piped version:
• control-plane version:
• Others:

[ffjlabo]

Reproduced on QuickSync like this↓ (name: pod-reader)

[result]

before

% k get clusterrole                                                                          (git)-[delete-cluster-role]
NAME                                                                   CREATED AT
admin                                                                  2024-03-25T07:29:57Z
cluster-admin                                                          2024-03-25T07:29:56Z
edit                                                                   2024-03-25T07:29:57Z
kindnet                                                                2024-03-25T07:30:01Z
kubeadm:get-nodes                                                      2024-03-25T07:29:58Z
local-path-provisioner-role                                            2024-03-25T07:30:02Z
pod-reader                                                             2024-03-28T09:37:08Z
system:aggregate-to-admin                                              2024-03-25T07:29:57Z

after

% k get clusterrole                                                                                 (git)-[delete-cluster-role]
NAME                                                                   CREATED AT
admin                                                                  2024-03-25T07:29:57Z
cluster-admin                                                          2024-03-25T07:29:56Z
edit                                                                   2024-03-25T07:29:57Z
kindnet                                                                2024-03-25T07:30:01Z
kubeadm:get-nodes                                                      2024-03-25T07:29:58Z
local-path-provisioner-role                                            2024-03-25T07:30:02Z
pod-reader                                                             2024-03-28T09:37:08Z
system:aggregate-to-admin                                              2024-03-25T07:29:57Z

[sample]

apiVersion: pipecd.dev/v1beta1
kind: KubernetesApp
spec:
  name: clusterrole-test
  input:
    namespace: test
  labels:
    env: clusterrole-test
    team: product
  quickSync:
    prune: true
  description: |
    This is a test app for clusterrole. Especially for testing the prune on QuickSync.

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: pod-reader
  annotations:
    pipecd.dev/order: "1"
rules:
- apiGroups: [""] # "" indicates the core API group
  resources: ["pods"]
  verbs: ["get", "watch", "list"]

apiVersion: v1
kind: Namespace
metadata:
  name: test
  labels:
    name: test
  annotations:
    pipecd.dev/order: "0"

[ffjlabo]

This might happen because the resource key in the live resource differs from the resource used when pruning.

• The actual resource key: rbac.authorization.k8s.io/v1:ClusterRole:test:pod-reader
• The one used when pruning: rbac.authorization.k8s.io/v1:ClusterRole:default:pod-reader (estimated by the code below)
  • https://github.com/pipe-cd/pipecd/blob/ad7d699df1c453888bdffe7ded63017aecec4a47/pkg/app/piped/executor/kubernetes/kubernetes.go#L293-L297
  • https://github.com/pipe-cd/pipecd/blob/ad7d699df1c453888bdffe7ded63017aecec4a47/pkg/app/piped/platformprovider/kubernetes/resourcekey.go#L103-L105

[ffjlabo]

As this PR https://github.com/pipe-cd/pipecd/pull/3991, all of the resources for the application belong to the namespace set on the spec.input.namespace in app.pipecd.yaml. So in PipeCD, the ClusterRole resource is also treated as the one which belong the namespace.

[ffjlabo]

Root cause:

the resource key stored as livestate and the one on the annotation of actual k8s resource are different values, but actually, they are compared as if they are equal.

The Code:

https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/platformprovider/kubernetes/applier.go#L161-L163

Details

The resource key stored as livestate

• created by the livestatestre.reflector after applying
• based on the actual k8s resource on the cluster with MakeResourceKey https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/platformprovider/kubernetes/resourcekey.go#L249-L260

More details

• applier.Delete(ctx, k) https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/executor/kubernetes/kubernetes.go#L287
• removeKeys are determined by findRemoveKeys using manifests (get from git repo) and liveResources (get from AppLiveResourceLister.ListKubernetesResources) https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/executor/kubernetes/sync.go#L119
  • first, determine values from git except for the namespace
  • then, determine the namespace from the livestate's one
  • => The namespace of the resourceKey depends on livestate's one
  • ListKubernetesResources
    • https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/controller/controller.go#L725-L727
    • originally, get from the livestate store
      • -> ListKubernetesAppLiveResources https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/livestatestore/livestatestore.go#L219-L225
      • -> s.store.GetAppLiveManifests https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/livestatestore/kubernetes/store.go#L302-L316
      • -> getManagingNodes https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/livestatestore/kubernetes/appnodes.go#L164-L169
• the resourceKey is stored by reflector based on the resource applied in the cluster.
  • using MakeResourceKey https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/platformprovider/kubernetes/resourcekey.go#L249-L260
  • treated the namespace as default if the obj.namespace is empty, ---code reading memo---
  • set the handlers (onAdd, onUpdate) https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/livestatestore/kubernetes/kubernetes.go#L102-L103
  • start informers with them to detect the changes https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/livestatestore/kubernetes/reflector.go#L219-L224
  • -> onObjectAdd, onObjectUpdate
    • https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/livestatestore/kubernetes/reflector.go#L251-L252
    • https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/livestatestore/kubernetes/reflector.go#L282-L286
  • -> onAddResource, onUpdateResource
    • https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/livestatestore/kubernetes/store.go#L170
    • https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/livestatestore/kubernetes/store.go#L185-L186
  • -> addResource https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/livestatestore/kubernetes/store.go#L156-L157
  • -> addManagingResource https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/livestatestore/kubernetes/appnodes.go#L66-L67

The resource key on the annotation of actual k8s resource

• created by before applying
• based on the k8s resource manifest, then fixed by app.pipecd.yaml (spec.input.namespace)

More details

• the annotation pipecd.dev/resource-key is set based on the resource loaded from the git repo before applying
  • loadManifests https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/executor/kubernetes/sync.go#L28-L35
  • addBuiltinAnnotations https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/executor/kubernetes/kubernetes.go#L223
• When loading manifests, piped executor determines the resourceKey based on the manifest and app.pipecd.yaml
  • first, determine the resourceKey using MakeResourceKey
    • -> ParseManifests (all of helm, kustomize, default) https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/platformprovider/kubernetes/manifest.- go#L243-L246
  • finally, fix it based on spec.input.namespace on app.pipecd.yaml
    • https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/platformprovider/kubernetes/loader.go#L90

Example:

Consider the case to apply ClusterRole

• spec.input.namespace exists (namespace: test)
  • livestate: rbac.authorization.k8s.io/v1:ClusterRole:default:pod-reader
  • annotation: rbac.authorization.k8s.io/v1:ClusterRole:test:pod-reader
• doesn't exist
  • livestate: rbac.authorization.k8s.io/v1:ClusterRole:default:pod-reader
  • annotation: rbac.authorization.k8s.io/v1:ClusterRole:default:pod-reader

[ffjlabo]

Solution candidates

The point is whether to fix or not as if they are equal.

1. fix not to compare them. (they are not equal)
2. fix to use of annotation when creating the resourceKey on the livestate. (they are equal)

In both cases, there is no user effect. Considering the effective scope for the fix, 1 is safer. But I think it is confusing that they are not equal.

[ffjlabo]

Other stages

Baseline

load manifests in the same way with K8S_SYNC stage indirectly. (by using loadRunningManifests) https://github.com/pipe-cd/pipecd/blob/7b1103d027fcdbc2468af60deb5da758fcc8f6ae/pkg/app/piped/executor/kubernetes/baseline.go#L46

So the resourceKey is based on the k8s resource manifest, then fixed by app.pipecd.yaml (spec.input.namespace).

then, store it and use it on the BASELINE_CLEAN

[ffjlabo]

📝 the namespace of the resourceKey is spec.input.namespace > original namespace https://github.com/pipe-cd/pipecd/blob/7b1103d027fcdbc2468af60deb5da758fcc8f6ae/pkg/app/piped/platformprovider/kubernetes/loader.go#L90-L91

[ffjlabo]

There are two problems

1. We can't delete the cluster scoped resources if we set the namespace to the app.pipecd.yaml.

• This is because the resource key stored as livestate and the one on the annotation of actual k8s resource are different. values.

2. The resourceKey rule of cluster scoped resources is undefined.

• Currently, if we set the namespace with app.pipecd.yaml (spec.input.namespace), resourceKey is also affected by it.

So I'll resolve 1 in this issue, and later will try 2.

[ffjlabo]

📝 Prepared the issue for 2 https://github.com/pipe-cd/pipecd/issues/4861

[ffjlabo]

Previous context: https://github.com/pipe-cd/pipecd/issues/4269#issuecomment-2031079364

After some investigations, I decided to fix the logic for the resource key as the ideal state. I think this problem is the incorrect namespace in the resource key created by MakeResourceKey.

https://github.com/pipe-cd/pipecd/blob/7b471d7a9f2eb09ccbc0a9222b6e7adb08ea2edf/pkg/app/piped/platformprovider/kubernetes/resourcekey.go#L249-L260

livestate side

• use default when the resource obj doesn't have the namespace.
• use the namespace when the resource obj has the namespace.

When reading the manifests from git

• use spec.input.namespace in app.pipecd.yaml if it is set.
• use the namespace on the manifest from git if it is set.
• use default when the namespace on the manifest is "".

So for example, cluster-scoped resource don't have any namespace, but if we set the spec.input.namespace in the app.pipecd.yaml, it sets the value as the namespace to the resource key.

Here is the ideal state of the resource key.

livestate side

• use "" for the cluster-scoped resource
• use the namespace for the namespace scoped resource

When reading the manifests from git

• use "" for the cluster-scoped resource
• for the namespace scoped resource,
• use spec.input.namespace in app.pipecd.yaml if it is set.
• use the namespace on the manifest from git if it is set.
• use default if both of spec.input.namespace and the namespace are not set

So I investigated the scope of the usage of MakeResourceKey.

At first, I drew the call graph.

This shows that the fix affects 4 components.

• livestate store
• executor
• detector
• planner

Next, I categorized each functions by the usecases. The resource key is usually created in the three cases below

1. read the resource from the cluster (livestate)
2. read the resource stored as manifests in git repo
3. create the temporary resource for the progressive delivery (e.g. canary pod and so on) after 2

I think the 1st and 3rd case don't need MakeResourceKey below

• the 1st case is just reading the correct resource from the cluster. So it should make the resource key just by reading it.
• the 3rd case is after 2nd case. So it would be good if the resource reading from git is correct.

So the problem is 2nd case. I think it can be solved by reading the resource from the git repo with the proper namespace.

So, it would be nice to unify the responsibility to the loader.LoadManifests. Especially, add function to the loader like this.

func (l *loader) refineNamespace(m Manifest) string {
	clusterScoped := true // TODO: determine cluster scoped
	if clusterScoped {
		return ""
	}

	if l.input.Namespace != "" {
		return l.input.Namespace
	}

	ns := m.GetNamespace()
	if ns == "" {
		return "default"
	}

	return ns
}

Finally, fixing the manifest's namespace only in the loader.Manifests would be nice.

TODO

• [x] fix to define the correct namespace when reading manifests from git repo
  • fix to refine the resource namespace in the loader.LoadManifests
  • ~fix not to use the MakeResourceKey on the livestate store~
  • ~fix not to use the MakeResourceKey on the executor~
  • remove the logic in MakeResourceKey, just use the namespace from the obj
• [ ] change the checking logic without using the annotation pipecd.dev/resource-key
• [ ] (option) fix not to use MakeResourceKey
• [ ] (option) change Manifest.Key as private variable to avoid changing other logic without loader.LoadManifests