Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

-> v1.8.0-RC3 #1687

Merged
merged 3 commits into from Sep 16, 2021
Merged

-> v1.8.0-RC3 #1687

merged 3 commits into from Sep 16, 2021

Conversation

mhowlett
Copy link
Contributor

No description provided.

Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs some minor release notes fixes, but apart from that it looks fantastic!

CHANGELOG.md Outdated

## Fixes

- **Breaking Change:** Updated the message framing format used by the Protobuf serdes to be compatible with the Java Protobuf serdes (message
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs more context, e.g., what type of clients (classes) that this applies to, and a proper mention of Schema-Registry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically, this doesn't necessarily need to have anything to do with Confluent Schema registry. I'm not sure the change log is the place to write a tutorial on how all this works, but i'll add a note anyway.

- **Breaking Change:** Updated the message framing format used by the Protobuf serdes to be compatible with the Java Protobuf serdes (message
indices now use zigzag encoding). To disable, set the `UseDeprecatedFormat` configuration property to `true`. ([rayokota](https://github.com/rayokota)).

## Security
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So empty.
Mention the fixed zlib CVEs:

Upgrade bundled zlib version from 1.2.8 to 1.2.11 in the librdkafka.redist NuGet package. The updated zlib version fixes CVEs: CVE-2016-9840, CVE-2016-9841, CVE-2016-9842, CVE-2016-9843 See https://github.com/edenhill/librdkafka/issues/2934 for more information.

@wuhkuh
Copy link

wuhkuh commented Sep 16, 2021

Hey, not sure if this is the right place to ask, but could you update the external dependencies in this branch?
I know for one that NJsonSchema broke binary compatibility (but not API compatibility) in a minor version, and recompiling the Confluent.Kafka-assemblies with the latest minor versions of dependencies would resolve this.

@mhowlett
Copy link
Contributor Author

thanks for the heads up @wuhkuh - i'll do this.

Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great to me

@mhowlett
Copy link
Contributor Author

@wuhkuh - i'm wondering if you understand / can explain what the issue is in more depth and how bumping the dependency version helps. My understanding is that by specifying an exact version as a dependency as we do, what we're actually doing is specifying a minimum version ( https://docs.microsoft.com/en-us/nuget/concepts/package-versioning#version-ranges ). So if a higher version is specified anywhere else in your project, or in a dependency of your project, that should be the version of NJsonSchema used by Confluent.SchemaRegistry.Serdes.Json as well.

There is a possibly related problem with the 4.5.0 System.Memory reference in Confluent.Kafka. People sometimes complain about that (and I'm not completely sure why).

Note: I don't think bumping would be considered backwards incompatible. It would be a breaking change if some other dependency of a project using Confluent.SchemaRegistry.Serdes.Json specifies an upper version limit on NJsonSchema that is lower than what we bump to, but i'm unsure if that is considered breaking semver or not ...

I most likely think we should do the bump, I just wanted to defer until 1.8.1 due to the above uncertainty. We want to get 1.8.0 out quickly because of the CVEs. We can do 1.8.1 quickly after.

@mhowlett mhowlett merged commit eaa450f into confluentinc:1.8.x Sep 16, 2021
@wuhkuh
Copy link

wuhkuh commented Sep 16, 2021

@mhowlett Sure thing!

JsonSchemaValidator.Validate has been augmented with an optional parameter with a default value in this commit. This is fully compatible API-wise, but this is handled at the call site in IL.

I compiled two versions of Confluent.SchemaRegistry.Serdes.Json with both current NJsonSchema (as in the .csproj file), and the latest version.
Using ildasm, we can make the binary incompatibility visible:

Confluent.SchemaRegistry.Serdes.Json + NJsonSchema 10.1.5:

        IL_0063:  callvirt   instance class [netstandard]System.Collections.Generic.ICollection`1<class [NJsonSchema]NJsonSchema.Validation.ValidationError>[NJsonSchema]NJsonSchema.Validation.JsonSchemaValidator::Validate(string, class [NJsonSchema]NJsonSchema.JsonSchema)

Confluent.SchemaRegistry.Serdes.Json + NJsonSchema 10.5.2:

        IL_0063:  ldc.i4.0
        IL_0064:  callvirt   instance class [netstandard]System.Collections.Generic.ICollection`1<class [NJsonSchema]NJsonSchema.Validation.ValidationError>[NJsonSchema]NJsonSchema.Validation.JsonSchemaValidator::Validate(string, class [NJsonSchema]NJsonSchema.JsonSchema, valuetype [NJsonSchema]NJsonSchema.SchemaType)

This means that by depending on a newer version of NJsonSchema, the runtime will throw a MissingMethodException when used in conjunction with the current version of Confluent.SchemaRegistry.Serdes.Json.

@mhowlett
Copy link
Contributor Author

ok, i was aware of this... but now I understand the scenario and how bumping the dependency helps. thanks.

@mhowlett
Copy link
Contributor Author

#1688

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants