k8ssandra-operator icon indicating copy to clipboard operation
k8ssandra-operator copied to clipboard

Operator is not resilient to missing spec.cassandra.serverVersion

Open Miles-Garnsey opened this issue 2 years ago • 7 comments

What happened?

The operator is not resilient to cases where the spec.cassandra.serverVersion is nil.

Did you expect to see something different?

Even malformed CRs should not cause crashes. This could lead to a DOS issue in multi-tenant clusters.

How to reproduce it (as minimally and precisely as possible):

When creating a K8ssandraCluster, it appears that I can crash the operator by submitting the following CR:

apiVersion: k8ssandra.io/v1alpha1
kind: K8ssandraCluster
metadata:
  name: test
  namespace: k8ssandra-operator
spec:
  cassandra:
    datacenters:
      - metadata:
          name: dc1
        size: 1
        storageConfig:
          cassandraDataVolumeClaimSpec:
            storageClassName: standard
            accessModes:
              - ReadWriteOnce
            resources:
              requests:
                storage: 5Gi
        config:
          jvmOptions:
            heapSize: 384Mi

┆Issue is synchronized with this Jira Story by Unito

Miles-Garnsey avatar Jan 10 '23 02:01 Miles-Garnsey

I suggest we should vet this at the webhook layer and then again via the controllers.

Miles-Garnsey avatar Jan 10 '23 02:01 Miles-Garnsey

Can't we make ServerVersion mandatory to make this simpler, by using // +kubebuilder:validation:Required?

adejanovski avatar Jan 10 '23 06:01 adejanovski

That does validation at the OpenAPI layer right? That's probably even better than the webhook, agreed.

Miles-Garnsey avatar Jan 10 '23 06:01 Miles-Garnsey

Or just have the regexp there also? This is what cass-operator does:

	// +kubebuilder:validation:Pattern=(6\.8\.\d+)|(3\.11\.\d+)|(4\.\d+\.\d+)
	ServerVersion string `json:"serverVersion"`

burmanm avatar Jan 11 '23 15:01 burmanm

We have it already: https://github.com/k8ssandra/k8ssandra-operator/blob/main/apis/k8ssandra/v1alpha1/k8ssandracluster_types.go#L310

adejanovski avatar Jan 11 '23 16:01 adejanovski

Right, but there's "omitempty", which allows there to be a nil.

burmanm avatar Jan 11 '23 17:01 burmanm

good point 👍 that's what we need to fix then.

adejanovski avatar Jan 11 '23 17:01 adejanovski