failure to detect drift in created kubernetes resources

[Kolossi]

What happened?

Kubernetes Deployment created from provider-kubernetes object is not monitored for drift.

How can we reproduce it?

Create the following object:

apiVersion: kubernetes.crossplane.io/v1alpha1
kind: Object
metadata:
  name: whoami
  namespace: default
spec:
  forProvider:
    manifest:
      apiVersion: apps/v1
      kind: Deployment
      metadata:
        namespace: default
      spec:
        metadata:
          name: whoami
          namespace: default
        replicas: 2
        selector:
          matchLabels:
            app: whoami
        template:
          metadata:
            labels:
              app: whoami
          spec:
            containers:
            - name: whoami
              image: docker.io/containous/whoami:v1.5.0
  providerConfigRef:
    name: kubernetes-provider

Edit the created deployment with kubectl edit deployments.apps whoami updating spec.replicas: 3

Expected: deployment drift detected and updated back to spec.replicas: 2.
Actual: Deployment continues to have 3 pods.

Editing the object forProvider.manifest.spec.replicas: 4: Expected: object drift detected and updated back to forProvider.manifest.spec.replicas: 2 and deployment and pods updated. Actual: object replicas, deployment replicas and pod count remains at 4

Note that if the object is created using composition, it behaves as above on deployment edit, BUT then if the object is updated forProvider.manifest.spec.replicas: 4 then the drift of object is somehow detected, the replicas reset to 2 and the deployment and pod count similarly fixed up back to 2.

apiVersion: apiextensions.crossplane.io/v1
kind: CompositeResourceDefinition
metadata:
  name: xminapps.example.com
spec:
  group: example.com
  names:
    kind: XMinApp
    plural: xminapps
  claimNames:
    kind: MinAppClaim
    plural: minappclaims
  versions:
  - name: v1alpha1
    served: true
    referenceable: true
    schema:
      openAPIV3Schema:
        type: object
        properties:
          spec:
            type: object
            properties:
              id:
                type: string
              parameters:
                type: object
                properties:
                  imageName:
                    type: string
                  replicas:
                    type: integer
                    default: 1
                required:
                - imageName
            required:
            - id
            - parameters
---
apiVersion: apiextensions.crossplane.io/v1
kind: Composition
metadata:
  name: minapp
  labels:
    crossplane.io/xrd: xminapp.example.com
    provider: kubernetes
spec:
  compositeTypeRef:
    apiVersion: example.com/v1alpha1
    kind: XMinApp
  resources:
  - name: deployment
    base:
      apiVersion: kubernetes.crossplane.io/v1alpha1
      kind: Object
      spec:
        forProvider:
          manifest:
            apiVersion: apps/v1
            kind: Deployment
            metadata:
              namespace: default
            spec:
              replicas: 1
              template:
                spec:
                  containers:
                  - name: frontend
        providerConfigRef:
          name: kubernetes-provider
    patches:
    - fromFieldPath: spec.id
      toFieldPath: metadata.name
    - fromFieldPath: spec.id
      toFieldPath: spec.forProvider.manifest.metadata.name
    - fromFieldPath: spec.id
      toFieldPath: spec.forProvider.manifest.metadata.labels.app
    - fromFieldPath: spec.id
      toFieldPath: spec.forProvider.manifest.spec.selector.matchLabels.app
    - fromFieldPath: spec.id
      toFieldPath: spec.forProvider.manifest.spec.template.metadata.labels.app
    - fromFieldPath: spec.id
      toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[0].name
    - fromFieldPath: spec.parameters.imageName
      toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[0].image
    - fromFieldPath: spec.parameters.replicas
      toFieldPath: spec.forProvider.manifest.spec.replicas
---
apiVersion: example.com/v1alpha1
kind: MinAppClaim
metadata:
  name: my-whoami
spec:
  id: whoami
  parameters:
    imageName: docker.io/containous/whoami:v1.5.0
    replicas: 2

What environment did it happen in?

Crossplane version: v1.7.0 (helm install) kubernetes version v1.22.5 (windows docker desktop kubernetes environment) Win 10 21H2 (19044.1645) accessing through through WSL2 Ubuntu 20.04.4 LTS (Focal Fossa) using client kubectl v1.21.5

[Kolossi]

Update: I've looked into the source and see that the observation of the created manifest is by looking at the last-applied-configuration.

This means that it will not detect the update to the deployment with kubectl edit as given in the repro in my initial post, but if it's done with apply such as:

kubectl get deployments.apps whoami -oyaml | sed '0,/^  replicas/{s/^  replicas:.*/  replicas: 4/}' | kubectl apply -f -

It does get detected and reverts to 2 replicas as per the spec in the Object/XRD.

This seems like a bug that it will only detect drift where it's been done "nicely", is there a reason it can't be fixed to look at actual observed state rather than "last-applied-configuration"?

[Kolossi]

A small extra request due to my faultfinding - the "sync-period" (I've read the source help but still don't really get what that is) is output in the debug logs, could the "poll-interval" (drift detection rate) also be output for faultfinding?

[turkenh]

@Kolossi you're right.

We are using last applied configuration since for some k8s resources, API Server fills some fields after you create the resource hence once you get it back you would always see it is different than what you want to apply. Service a good example of this behavior:

For example, you declare this:

apiVersion: v1
kind: Service
metadata:
  name: test
spec:
  ports:
  - name: https
    port: 443
    protocol: TCP
    targetPort: https
  selector:
    app.kubernetes.io/component: test
  type: ClusterIP

If you get it back after creation, you will get something like:

apiVersion: v1
kind: Service
metadata:
  name: test
spec:
  clusterIP: 10.178.253.253
  clusterIPs:
  - 10.178.253.253
  ipFamilies:
  - IPv4
  ipFamilyPolicy: SingleStack
  ports:
  - name: https
    port: 443
    protocol: TCP
    targetPort: https
  selector:
    app.kubernetes.io/component: test
  sessionAffinity: None
  type: ClusterIP
status:
  loadBalancer: {}

So, it is not straightforward to figure out the drift from the desired state without a workaround like last applied configuration.

The current implementation opts for simplicity by not introducing resource-specific and/or complex merging logic. An ideal solution could be to leverage Server Side Apply feature.

[Kolossi]

OK so I understand the logic of the choosing the simple (likely more because it was "easy") option grabbing last applied configuration.

But I don't feel like this is really doing drift detection as in-cluster hacks arent necessarily (likely) to use kubectl apply and play nice.

I feel like although complex merge logic would be ideal, a next level "easy" option is to look at the declared manifest, and only for those fields check whether there is a mismatch in the actual manifest. The logic being if the field really wanted to be fixed, it "should" have been specified in the declared manifest, anything not in the declaration is essentially "wildcard" and anything should be allowed.

That shouldn't be too complicated to implement?

[Kolossi]

But yeah api equiv of kubectl-diff (rather than server side apply) might be a more comprehensive version - if anything has changed, reapply config from declared.

Hmm no I'm wrong there on the last bit, that would just detect if actual was different to last-applied. But that might also be different to declared which needs to be picked up. So doing the kubectl diff then the current approach would maybe be the way to go?

[AbrohamLincoln]

Could the resourceVersion of created/updated objects be tracked and used as an indicator of drift?

[erikgb]

I wonder if using server-side apply could improve this. SSA will probably become the new default in kubectl eventually.