🐛 [BUG] - Resources gets created when plugin is disabled

[ibakshay]

Priority

(Medium) I'm annoyed but I'll live

Description

The resources defined in the PluginDefinition helm chart gets created in the remote cluster even when the plugin.spec.disabled is set to true.

Reproduction steps

1. Add a new plugin with `spec.disabled = true`
2. Monitor if the resources are being created in the remote cluster
3. Ideally, the resources should not be created in the remote cluster

Manifests

apiVersion: greenhouse.sap/v1alpha1
kind: Plugin
metadata:
  creationTimestamp: "2024-06-21T13:19:34Z"
  finalizers:
    - greenhouse.sap/helm
  generation: 6
  labels:
    greenhouse.sap/cluster: obs-eu-de-1
    greenhouse.sap/plugindefinition: kube-monitoring
    greenhouse.sap/pluginpreset: kube-monitoring
  name: kube-monitoring-obs-eu-de-1
  namespace: test-org
spec:
  clusterName: remote-cluster-1-control-plane
  disabled: true
  displayName: kube monitoring obs eu de 1
  optionValues:
    - name: alerts.enabled
      value: false
    - name: kubeMonitoring.defaultRules.create
      value: false
    - name: kubeMonitoring.prometheus.ingress.enabled
      value: false
    - name: kubeMonitoring.prometheus.prometheusSpec.externalLabels
      value:
        cluster: "{{ .Values.global.greenhouse.clusterName }}"
        cluster_type: '{{ (split "-" .Values.global.greenhouse.clusterName)._0 }}'
        organization: ccloud
        region: "{{ .Values.global.greenhouse.clusterName | trunc -7 }}"
    - name: kubeMonitoring.prometheus.prometheusSpec.externalUrl
      value:
        https://kube-monitoring-{{ .Values.global.greenhouse.clusterName }}-prometheus--ccloud--{{
        .Values.global.greenhouse.clusterName }}.ccloud.greenhouse-qa.eu-nl-1.cloud.sap
    - name: kubeMonitoring.prometheus.prometheusSpec.retention
      value: 30d
    - name: kubeMonitoring.prometheus.prometheusSpec.storageSpec.volumeClaimTemplate.spec.resources.requests.storage
      value: 100Gi
    - name: kubeMonitoring.prometheus.service.additionalPorts
      value:
        - name: grpc
          port: 10901
          protocol: TCP
          targetPort: grpc
    - name: global.greenhouse.clusterNames
      value:
        - obs-eu-de-1
        - obs-eu-nl-1
    - name: global.greenhouse.teamNames
      value:
        - baremetal
        - compute-storage-api
        - containers
        - core
        - email
        - foundation
        - identity
        - image
        - infraautomation
        - network-api
        - observability
        - solutiondevelopment
        - storage
    - name: global.greenhouse.organizationName
      value: ccloud
    - name: global.greenhouse.clusterName
      value: obs-eu-de-1
  pluginDefinition: kube-monitoring
  releaseNamespace: kube-monitoring

Screenshots

![DESCRIPTION](LINK.png)

[viennaa]

Good catch.

Was running in this too, when testing this scenario:

• Disabling the plugin in the pluginpreset
• Enabling them with clusterOverrides for certain clusters

Not sure if the latter is even possible, but the disabled plugin shows as disabled correctly, however all resources are deployed.

[IvoGoman]

I found the original commit that introduced the disabled field almost 1.5 years ago. But the functionality behind the field was never implemented.

There are two parts to this:

1. Disabling the Helm Chart leads to helm uninstall and skip reconciliation
2. Skip the UI for this Plugin (if the PluginDefinition contains a uiApplication)

[ibakshay]

I found the original commit that introduced the disabled field almost 1.5 years ago. But the functionality behind the field was never implemented.

There are two parts to this:

1. Disabling the Helm Chart leads to helm uninstall and skip reconciliation
2. Skip the UI for this Plugin (if the PluginDefinition contains a uiApplication)

@IvoGoman Thank you for giving the insights.

Do you think it also makes sense to add a condition to the plugin.status.statusConditions.conditions?

statusConditions:
  conditions:
    - lastTransitionTime: "2024-08-26T17:06:42Z"
      message: "Plugin is disabled successfully"
      status: "True|false"
      type: PluginDisabled
    - lastTransitionTime: "2024-09-04T13:08:48Z"
      message: Helm Chart Test is successful
      status: "True"
      type: NoHelmChartTestFailures

[IvoGoman]

I am not sure we should actually implement this, or deprecate and remove the field. It looks like there was no real use-case for it since the field was added 1,5 years ago.

The scenario which @viennaa is describing sounds like a feature for the PluginPreset, as to use a 'clusterSelector' but also provide a blocklist to remove clusters by name?

[ibakshay]

I am not sure we should actually implement this, or deprecate and remove the field. It looks like there was no real use-case for it since the field was added 1,5 years ago.

The scenario which @viennaa is describing sounds like a feature for the PluginPreset, as to use a 'clusterSelector' but also provide a blocklist to remove clusters by name?

okay, cool. Shall we discuss this topic in the next Greenhouse weekly meeting?
@IvoGoman