confluentinc / terraform-provider-confluent

Terraform Provider for Confluent
Apache License 2.0
31 stars 64 forks source link

Schema Validation changes after upgrading to v2.7.0 from v1.27.9 #485

Open gautam-goudar opened 1 week ago

gautam-goudar commented 1 week ago

Have subsequent upgrades to the Terraform provider changed the schema validation mechanism?

When using v1.27.0, we are able to upgrade the schema with BACKWARD compatibility by

Example

v1
-----------
message MyProtoSchema_v1 {
  google.protobuf.StringValue id = 1;
  google.protobuf.Timestamp issuedAt = 2;
}

v2
-----------
message MyProtoSchema_v1 {
  google.protobuf.StringValue id = 1;
  google.protobuf.Timestamp issuedAt = 2;
  TemporaryOperatingPermitCalculation calculation = 3;
}

message TemporaryOperatingPermitCalculation {
...........................
}

With v1.27.0, we were able to deploy v2 with the above changes by adding the field calculation of type TemporaryOperatingPermitCalculation.

Now after upgrading to v2.7.0, the same pipeline returns the below error (without any changes)

Error: error validating Schema: error validating a schema: [{errorType:"MESSAGE_REMOVED", description:"The new schema is missing a MESSAGE type at path '#/TemporaryOperatingPermitCalculation' in the old schema"} {oldSchemaVersion: 2} {oldSchema: 'syntax = "proto3";

What is the solution to this error? Please help.

Thanks

gautam-goudar commented 1 week ago

If this helps, below is the line in the source code (like 459) that prints out the error we are seeing in the logs https://github.com/confluentinc/terraform-provider-confluent/blob/2b0d56e24dff0a4eb2f48e069ee0096284eb0463/internal/provider/resource_schema.go#L459

The function “schemaValidateCheck” was added to the code base on May 31st, 2023

cryoshida commented 4 days ago

Hi @gautam-goudar !

Forward and full compatibility are not recommended for Protobuf. The compatibility matrix remained the same across SR releases.

Note that best practice for Protobuf is to use BACKWARD_TRANSITIVE, as adding new message types is not forward compatible.

Here are some links that might be helpful: https://docs.confluent.io/platform/current/schema-registry/fundamentals/schema-evolution.html#summary and https://www.confluent.io/blog/best-practices-for-confluent-schema-registry/ .

That being said, I'm double checking with our team to see if there is anything else that we might have missed.

gautam-goudar commented 4 days ago

Thanks.

We are using "BACKWARD" compatibility for out protobuf schema. For this very same reason, we are not sure why we are seeing the error when a new message is added to evolve schema.

linouk23 commented 3 days ago

@gautam-goudar, it's a bit surprising to see that it worked in the first place, considering the validation fails. 🤔

Could you try setting skip_validation_during_plan = true and see whether terraform plan work (assuming the goal is to fix terraform plan for already deployed schemas)?

Given that this flag was likely introduced after we added validation to terraform plan, and your terraform plan fails already, you might have to add skip_validation_during_plan = true. Additionally, you may need to patch the TF state file manually to include this line. Otherwise, your update from skip_validation_during_plan = false (default) to true may not go through. This is because terraform apply will implicitly call terraform plan (that fails already), which will cause the apply to fail as well.