Option values for storage breaks after update

[Demonian]

HI,

I have encountered strange behavior if options for storage are set in two ways (flat structure with dot as a key and normal yaml structure) in the same Jaeger manifest. E.g.:

apiVersion: jaegertracing.io/v1
kind: Jaeger
metadata:
  name: jaeger-tracing
  namespace: istio-system
spec:
  ingress:
    enabled: false
  allInOne:
    options:
      query:
        base-path: /jaeger
  strategy: allInOne
  storage:
    type: cassandra
    cassandraCreateSchema:
      enabled: false
    options:
      # should set this key like this because it is impossible to enable `tls` if yaml structure for
      # for options is used
      cassandra.tls: true  
      cassandra:
        servers: cassandra.default.svc.cluster.local
        port: 9042
        keyspace: zipkin_traces
        tls:
          ca: {some value}
          cert: {some value}
          key: {some value}
          server-name: jaeger
          verify-host: false

STR:

1. Apply manifest with structure above.
2. Wait until operator will create jaeger-tracing deployment
3. Investigate applied Jaeger manifest with kubectl get jaeger jaeger-tracing -o yaml --export you will see smth like this:

   apiVersion: jaegertracing.io/v1
   kind: Jaeger
   metadata:
   annotations:
   {many other keys here}
   options:
     cassandra.keyspace: zipkin_traces
     cassandra.port: 9042
     cassandra.servers: cassandra.default.svc.cluster.local
     cassandra.tls: true
     cassandra.tls.ca: {some value}
     cassandra.tls.cert: {some value}
     cassandra.tls.key: {some value}
     cassandra.tls.server-name: jaeger
     cassandra.tls.verify-host: false

4. At this point everything looks fine. Now lets change this value:

   options:
    cassandra:
      tls:
        verify-host: true

5. Apply new manifest with changed value of verify-host
6. Investigate changed Jaeger manifest with kubectl get jaeger jaeger-tracing -o yaml --export, now we will see that the structure is broken and there it is inconsistent now:

   options:
     cassandra:
       keyspace: zipkin_traces
       port: 9042
       servers: cassandra.default.svc.cluster.local
       tls:
         ca: {some value}
         cert: {some value}
         key: {some value}
         server-name: jaeger
         verify-host: true
     cassandra.keyspace: zipkin_traces
     cassandra.port: "9042"
     cassandra.servers: cassandra.default.svc.cluster.local
     cassandra.tls: true
     cassandra.tls.ca: {some value}
     cassandra.tls.cert: {some value}
     cassandra.tls.key: {some value}
     cassandra.tls.server-name: jaeger
     cassandra.tls.verify-host: "false"

The only possible way to fix this is to delete manifest and reapply it again.

Workaround: If all keys will be written in flat structure (with dot) there will be no problem.

Expected Result There should be ability to enable tls with normal yaml structure of keys.

Proposal

1. If yaml structure of keys is used we need to set tls to true if this key exists in manifest.
2. Do not change the structure of keys in Jaeger manifest after operator parsing it.
3. Merge old and new values (the hardest way, because it will be hard to analyze what value is new)

[jpkrohling]

The only possible way to fix this is to delete manifest and reapply it again.

I wonder what happens when both are set in the same original CR. @kevinearls, @jkandasa, perhaps it's something for you to test?

If all keys will be written in flat structure (with dot) there will be no problem.

~Which version of the Operator are you using?~ This should have been fixed by #523 (cc @rubenvp8510), in that the operator will not change the values that were initially provided by your CR.

EDIT: it doesn't matter which version of the operator you are using, this fix wasn't released yet. Would you be open to try the image from the current master?

[Demonian]

@jpkrohling I was using jaeger-operator with version 1.13.1 so the latest one, I could try with master. Will comment on my results one done.

[jpkrohling]

@Demonian did you have a chance to test a newer operator?

[Demonian]

@jpkrohling We are using workaround for now, but I will try to verify, the latest version. Thanks.

[Demonian]

Could you please link the commit with possible fix?

[jpkrohling]

523 is the PR that should have fixed this.

[jpkrohling]

@Demonian, have you had the chance to verify this? Do you think it's safe to close the issue?

[stale[bot]]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.