flipp-oss / deimos

Framework to work with Kafka, Avro and ActiveRecord
Other
59 stars 22 forks source link

Confluent Cloud support #134

Closed iMacTia closed 2 years ago

iMacTia commented 2 years ago

Summary

Add support for Confluent Cloud, by adding SASL authentication. Fixes #133

Description

This PR solves 2 authentication issues with Confluent Cloud:

  1. Provide SASL authentication when connecting to a Confluent Cloud Kafka cluster
  2. Provide Basic authentication when connecting to a Confluent Cloud Schema Registry

The issue at the moment is that the latter change relies on a change in avro_turf that was introduced here, and only released as part of v1.3.0. However deimos currently depends on avro_turf ~> 0.11, and when I tried relaxing the dependency to allow for avro_turf 1.x, I got an error with the schema classes generator.

I managed to pinpoint the issue to a breaking change that was released with avro_turf 1.0. There were even plans to bring support for it since 2020 (#67), but I see no progress has been made since then.

@dorner any guidance on this would be welcome. I haven't spent enough time with Deimos to feel confident fixing this compatibility issue and I'm not sure I understand your proposal about a "singleton encoder/decoder". If this is too complicated or lengthy to implement, I might opt for temporarily monkey-patching avro_turf by cherry picking the basic auth change commit.

Type of change

Please delete options that are not relevant.

How Has This Been Tested?

Checklist:

dorner commented 2 years ago

@iMacTia the PR looks really good so far! Can you elaborate on the issue you were seeing with the schema classes generator?

iMacTia commented 2 years ago

Oh, I was hoping my PR would run the CI and you could see it as well, is that because it's a Draft? I'll mark it ready for review šŸ‘

But in short, it's because updating avro_turf causes the "secondary classes" not to be generated (nested schemas). Example below:

Deimos::Generators::SchemaClassGenerator with a mix of Consumer and Producer Schemas should generate a schema class for my_nested_schema
     Failure/Error: expect(File.read(generated_path)).to eq(File.read(expected_path))

       expected: "# frozen_string_literal: true\n\n# This file is autogenerated by Deimos, Do NOT modify\nmodule Schem...o_h,\n        'some_optional_record' => @some_optional_record&.to_h\n      }\n    end\n  end\nend\n"
            got: "# frozen_string_literal: true\n\n# This file is autogenerated by Deimos, Do NOT modify\nmodule Schem...o_h,\n        'some_optional_record' => @some_optional_record&.to_h\n      }\n    end\n  end\nend\n"

       (compared using ==)

       Diff:
       @@ -2,52 +2,6 @@

        # This file is autogenerated by Deimos, Do NOT modify
        module Schemas
       -  ### Secondary Schema Classes ###
       -  # Autogenerated Schema for Record at com.my-namespace.MyNestedRecord
       -  class MyNestedRecord < Deimos::SchemaClass::Record
       -    ### Attribute Accessors ###
       -    # @param value [Integer]
       -    attr_accessor :some_int
       -    # @param value [Float]
       -    attr_accessor :some_float
       -    # @param value [String]
       -    attr_accessor :some_string
       -    # @param value [nil, Integer]
       -    attr_accessor :some_optional_int
       -
       -    # @override
       -    def initialize(some_int: nil,
       -                   some_float: nil,
       -                   some_string: nil,
       -                   some_optional_int: nil)
       -      super
       -      self.some_int = some_int
       -      self.some_float = some_float
       -      self.some_string = some_string
       -      self.some_optional_int = some_optional_int
       -    end
       -
       -    # @override
       -    def schema
       -      'MyNestedRecord'
       -    end
       -
       -    # @override
       -    def namespace
       -      'com.my-namespace'
       -    end
       -
       -    # @override
       -    def to_h
       -      {
       -        'some_int' => @some_int,
       -        'some_float' => @some_float,
       -        'some_string' => @some_string,
       -        'some_optional_int' => @some_optional_int
       -      }
       -    end
       -  end
       -
          ### Primary Schema Class ###
          # Autogenerated Schema for Record at com.my-namespace.MyNestedSchema
          class MyNestedSchema < Deimos::SchemaClass::Record
dorner commented 2 years ago

Our CI needs a lot of love unfortunately - I haven't had time to go back to fix it up. :(

Thanks for the note here - I'll try to look into this over the next couple of days and see if I can get some help from other team members as well.

iMacTia commented 2 years ago

Thank you @dorner šŸ™! (I can see you have a .circleci folder, but for some reason, the CI didn't kick-off for my PRs šŸ¤”)

dorner commented 2 years ago

@iMacTia I have a fix for this here: https://github.com/flipp-oss/deimos/pull/135 I'm going to wait for Derek to come back from vacation to review it, which should be mid next week.

iMacTia commented 2 years ago

Thanks for the quick turnaround! Ping me if I can help in any way!

iMacTia commented 2 years ago

Nice work on #135, as soon as that's merged next week I'll rebase this PR and hopefully that should make tests pass. Still, I'm unsure as to why CI isn't running here. If you'd like, I can help migrating over from CircleCI to GitHub Actions? It's completely free for OSS and I'm enjoying it quite a lot in my gems. Let me know!

dorner commented 2 years ago

That was definitely the plan! It just kept getting bumped down on my list of priorities. Would be amazing if you would be willing to take a stab at that! I know that lint fails, I should raise an issue to fix the lint stuff as well. :)

iMacTia commented 2 years ago

Happy to give it a try šŸ™Œ! I'll open a PR to fix both šŸ‘Œ

dorner commented 2 years ago

@iMacTia The other PR has been merged! Please fix up / resolve this one (and add an entry to the CHANGELOG under UNRELEASED).

iMacTia commented 2 years ago

@dorner this is now ready for review, I was able to successfully produce a message against our Confluent Cloud cluster today šŸŽ‰ ! I've also updated CHANGELOG.md and CONFIGURATION.md with the new settings šŸ‘

dorner commented 2 years ago

Looks good! I'm going to merge and run a local test before cutting a release.

dorner commented 2 years ago

Version has been bumped! Thanks so much for the contributions!

iMacTia commented 2 years ago

Thanks for the support and the quick review šŸ™Œ !

tjsiron commented 2 years ago

@iMacTia Would you be able to post a sanitized version of your configuration object for deimos sasl relating to Confluent Cloud? We're in a similar boat trying to get this to work and want to make sure we're setting the same things as you did in your test.

iMacTia commented 2 years ago

@tjsiron Of course! I think the relevant parts are these:

kafka do
  seed_brokers ENV['KAFKA_BROKERS']&.split(',')

  ssl do
    enabled true
    ca_certs_from_system true
  end

  sasl do
    enabled true
    plain_username ENV['KAFKA_SASL_USER']
    plain_password ENV['KAFKA_SASL_PASS']
    enforce_ssl true
  end
end

schema do
  backend :avro_schema_registry
  use_schema_classes true
  registry_url ENV['KAFKA_SCHEMA_REGISTRY_URL']
  user ENV['KAFKA_SCHEMA_REGISTRY_USER']
  password ENV['KAFKA_SCHEMA_REGISTRY_PASS']
end

One thing that surprised my at the beginning, was the need for ssl.ca_certs_from_system, that might be one of the missing pieces in your config.

If you have any more specific questions (or errors to share, I have seen plenty šŸ˜‚), please do let me know!