[BUG] Updating the field `spec.ingress.sql.tls.secretName` is not reflected in the sql ingress object

[hoyhbx]

What version of operator are you using? commit 561cf47d783c368fd8795acb82a5a39099a35984 (HEAD -> master)

What operating system and processor architecture are you using (kubectl version)? Ubuntu. 20.04

kubectl version Output

$ kubectl version
Client Version: version.Info{Major:"1", Minor:"24", GitVersion:"v1.24.0", GitCommit:"4ce5a8954017644c5420bae81d72b09b735c21f0", GitTreeState:"clean", BuildDate:"2022-05-03T13:46:05Z", GoVersion:"go1.18.1", Compiler:"gc", Platform:"darwin/amd64"}
Kustomize Version: v4.5.4
Server Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.9", GitCommit:"6df4433e288edc9c40c2e344eb336f63fad45cd2", GitTreeState:"clean", BuildDate:"2022-05-19T19:53:08Z", GoVersion:"go1.16.15", Compiler:"gc", Platform:"linux/amd64"}
WARNING: version difference between client (1.24) and server (1.22) exceeds the supported minor version skew of +/-1

What did you do?

We found that when we updated the field spec.ingress.sql.tls.secretName in the CR, the change is not reflected in the sql ingress resource. The field spec.ingress.sql.tls.secretName is only effective when initially creating the sql ingress resource.

Reproduce

1. We first applied the operator yaml file and crd yaml file to deploy the operator and used the following cr.yaml file to deploy the cockroachdb cluster by using kubectl apply -f cr.yaml -n cockroach-operator-system cr.yaml:

apiVersion: crdb.cockroachlabs.com/v1alpha1
kind: CrdbCluster
metadata:
  name: test-cluster
spec:
  additionalLabels:
    crdb: is-cool
  dataStore:
    pvc:
      spec:
        accessModes:
        - ReadWriteOnce
        resources:
          requests:
            storage: 1Gi
        volumeMode: Filesystem
  image:
    name: cockroachdb/cockroach:v21.2.10
  ingress:
    sql:
      host: MyHost
      tls:
      - secretName: MySecretName
  nodes: 3
  resources:
    limits:
      cpu: 2
      memory: 2Gi
    requests:
      cpu: 100m
      memory: 1Gi
  tlsEnabled: true

2. Then we updated the field spec.ingress.sql.tls.secretName by deploying the following yaml file:

apiVersion: crdb.cockroachlabs.com/v1alpha1
kind: CrdbCluster
metadata:
  name: test-cluster
spec:
  additionalLabels:
    crdb: is-cool
  dataStore:
    pvc:
      spec:
        accessModes:
        - ReadWriteOnce
        resources:
          requests:
            storage: 1Gi
        volumeMode: Filesystem
  image:
    name: cockroachdb/cockroach:v21.2.10
  ingress:
    sql:
      host: MyHost
      tls:
      - secretName: MyAnotherSecretName
  nodes: 3
  resources:
    limits:
      cpu: 2
      memory: 2Gi
    requests:
      cpu: 100m
      memory: 1Gi
  tlsEnabled: true

What did you see?

spec.tls.0.secret_name under ingress was not updated in the cluster. We expected that spec.tls.0.secret_name under ingress would be changed from MySecretName to MyAnotherSecretName, but there was no such change.

Possible root cause We searched through cockroach operator code based and it seemed that the cockroach operator does not have code update spec.ingress.sql.tls.secretName. So no matter what new value is assigned to spec.ingress.sql.tls.secretName, nothing will be reflected in the cluster.

There is a similar feature in the spec.ingress.ui, and it is implemented here

This is a bug, since secretName cannot be updated and there were no messages indicating that our input got rejected.

Possible fix We also ran an experiment after adding these lines to pkg > resource > sql_ingress.go > BuildV1Ingress at line 92 (right before the function 's final return nil):

for i := range ingressConfig.SQL.TLS {
		ingress.Spec.TLS = append(ingress.Spec.TLS, v1.IngressTLS{
			Hosts:      ingressConfig.SQL.TLS[i].Hosts,
			SecretName: ingressConfig.SQL.TLS[i].SecretName,
		})
	}

And after remaking the image and used the image we made to deploy the operator, the change in spec.ingress.sql.tls.secretName showed up in the cluster.

[chrisseto]

Hey @hoyhbx, can you tell us a bit more about your setup?

You're correct that spec.tls is not set for the SQL ingress object. This is because CockroachDB serves SQL traffic over TLS when tlsEnabled is set to true.

Do you terminate TLS at your ingress controller and then re-establish it between the ingress controller and CockroachDB?

I don't think we had considered this as a use case previously.

[hoyhbx]

Hi @chrisseto, thanks for your reply

Do you terminate TLS at your ingress controller and then re-establish it between the ingress controller and CockroachDB?

yeah I think this could be one of the use cases to change the spec.ingress.sql.tls.secretName after setting the TLS connection up. Do you think the linked PR could solve this problem?

[hoyhbx]

@chrisseto , if the TLS is not used, we can add a validation in the webhook to explicitly let the users know that the spec.sql.tls is not being used. How do you think of this fix?