streamthoughts / jikkou

The Open source Resource as Code framework for Apache Kafka
https://www.jikkou.io
Apache License 2.0
165 stars 20 forks source link

Erroneous schema registry subject comparison #431

Closed minionOfZuul closed 7 hours ago

minionOfZuul commented 1 month ago

Describe the bug Jikkou schema registry provider wants to change Avro schemas that don't need to change. This stems from the fact that it appears to do a string comparison between expected and actual schemas, instead of comparing by schema fingerprint.

To Reproduce schemasubject.yml

apiVersion: "schemaregistry.jikkou.io/v1beta2"
kind: "SchemaRegistrySubject"
metadata:
  name: test-schema
spec:
  schemaType: AVRO
  schema:
    $ref: test-schema.avsc

test-schema.avsc

{
  "type": "record",
  "name": "TestObj",
  "namespace": "test.namespace",
  "fields": [
    {
      "name": "field_1",
      "type": "string"
    },
    {
      "name": "field_2",
      "type": {
        "name": "code",
        "namespace": "test.namespace",
        "type": "enum",
        "symbols": [
          "one",
          "two"
        ]
      }
    }
  ]
}

First, create the subject. jikkou apply -f schemasubject.yaml Output:

TASK [CREATE] Create subject 'test-schema' (type=CREATE, compatibilityLevel=<global>) - CHANGED *********
{
  "end" : "2024-05-02T15:24:06.454388Z",
  "status" : "CHANGED",
  "description" : "Create subject 'test-schema' (type=CREATE, compatibilityLevel=<global>)",
  "changed" : true,
  "failed" : false,
  "change" : {
    "apiVersion" : "schemaregistry.jikkou.io/v1beta2",
    "kind" : "SchemaRegistrySubjectChange",
    "metadata" : {
      "name" : "test-schema",
      "labels" : { },
      "annotations" : {
        "jikkou.io/managed-by-location" : "schemasubject.yaml"
      }
    },
    "spec" : {
      "changes" : [ {
        "name" : "schema",
        "op" : "CREATE",
        "after" : "{\"fields\":[{\"name\":\"field_1\",\"type\":\"string\"},{\"name\":\"field_2\",\"type\":{\"name\":\"code\",\"namespace\":\"test.namespace\",\"symbols\":[\"one\",\"two\"],\"type\":\"enum\"}}],\"name\":\"TestObj\",\"namespace\":\"test.namespace\",\"type\":\"record\"}"
      }, {
        "name" : "schemaType",
        "op" : "CREATE",
        "after" : "AVRO"
      }, {
        "name" : "references",
        "op" : "CREATE",
        "after" : [ ]
      } ],
      "op" : "CREATE",
      "data" : {
        "normalizeSchema" : false,
        "permanentDelete" : false
      }
    }
  }
}
EXECUTION in 1s 822ms
ok : 0, created : 1, altered : 0, deleted : 0 failed : 0

Next, run another apply, but with --dry-run. Note that jikkou wants to change the schema, even though it should result in ok : 0, not altered : 1. jikkou apply -f schemasubject.yaml --dry-run

TASK [UPDATE] Update subject 'test-schema' (type=UPDATE, compatibilityLevel=<global>) - CHANGED *********
{
  "end" : "2024-05-02T15:25:01.854724Z",
  "status" : "CHANGED",
  "description" : "Update subject 'test-schema' (type=UPDATE, compatibilityLevel=<global>)",
  "changed" : true,
  "failed" : false,
  "change" : {
    "apiVersion" : "schemaregistry.jikkou.io/v1beta2",
    "kind" : "SchemaRegistrySubjectChange",
    "metadata" : {
      "name" : "test-schema",
      "labels" : { },
      "annotations" : {
        "jikkou.io/managed-by-location" : "schemasubject.yaml"
      }
    },
    "spec" : {
      "changes" : [ {
        "name" : "compatibilityLevel",
        "op" : "NONE"
      }, {
        "name" : "schema",
        "op" : "UPDATE",
        "before" : "{\"fields\":[{\"name\":\"field_1\",\"type\":\"string\"},{\"name\":\"field_2\",\"type\":{\"name\":\"code\",\"symbols\":[\"one\",\"two\"],\"type\":\"enum\"}}],\"name\":\"TestObj\",\"namespace\":\"test.namespace\",\"type\":\"record\"}",
        "after" : "{\"fields\":[{\"name\":\"field_1\",\"type\":\"string\"},{\"name\":\"field_2\",\"type\":{\"name\":\"code\",\"namespace\":\"test.namespace\",\"symbols\":[\"one\",\"two\"],\"type\":\"enum\"}}],\"name\":\"TestObj\",\"namespace\":\"test.namespace\",\"type\":\"record\"}"
      }, {
        "name" : "schemaType",
        "op" : "NONE",
        "before" : "AVRO",
        "after" : "AVRO"
      }, {
        "name" : "references",
        "op" : "NONE",
        "before" : [ ],
        "after" : [ ]
      } ],
      "op" : "UPDATE",
      "data" : {
        "normalizeSchema" : false,
        "permanentDelete" : false
      }
    }
  }
}
EXECUTION in 1s 819ms
ok : 0, created : 0, altered : 1, deleted : 0 failed : 0

Note that the schema stored in schema registry has had the namespace property removed from the nested enum. A namespace on a nested enum is OK according to the avro spec, but is removed during "canonicalization". The semantic meaning of the schema has not changed, but this is enough to trick Jikkou into thinking it needs to create an UPDATE Operation instead of a NONE.

Expected behavior After the initial subject creation, subsequent invocations should not result in an update operation.

Runtime environment

Additional context Here's what I believe is happening. On schema registration, the Confluent schema registry converts the schema to "canonical" form so that it can compare the new incoming subject version to previous versions to see if they have, in fact, changed. It has its own logic to compute the "fingerprint" of the canonical form of the schema and uses that to see if the schema already exists in its storage.

Jikkou, on the other hand, seems to do a simple string comparison to determine which operation it needs to perform to go from actual state to expected state. See here. If we could teach jikkou to ignore the schema part of the spec and instead teach it to consider a fingerprint (can be computed using avro builtin functions) when determining which operation to build, that will solve the problem.

fhussonnois commented 4 weeks ago

Hi @minionOfZuul, thank you very much for your PR (#432) I'm OK with that fact that it should be possible to use the canonical representation of schemas to detect changes.

However, this cannot be the default behavior as this would be a breaking change in regards to previous versions.

In addition, the current use of a simple string comparaison is also the current strategy used by the Confluent Schema Registry (see: https://github.com/confluentinc/schema-registry/issues/1698). One of the main reason, is that an Avro schema can be enriched with any custom properties that are excluded by the canonical form. For example, some projects include metadata such as the version, the owner, tags, directly in the schema.

I would propose to update your PR to support both strategies. For doing this, we could use a dedicated new annotation (e.g., schemaregistry.jikkou.io/use-canonical-fingerprint=true) with the default value being false if no annotation is present in order to not introduce any breaking change. So, depending on their project needs, users can switch from one strategy to another.