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

Confluent Cloud support #134

Merged
merged 3 commits into from
Mar 9, 2022
Merged

Conversation

iMacTia
Copy link
Contributor

@iMacTia iMacTia commented Mar 4, 2022

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.

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Add unit tests
  • Test with private Rails application using Confluent Cloud

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added a line in the CHANGELOG describing this change, under the UNRELEASED heading
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@dorner
Copy link
Member

dorner commented Mar 4, 2022

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

@iMacTia
Copy link
Contributor Author

iMacTia commented Mar 4, 2022

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

@iMacTia iMacTia marked this pull request as ready for review March 4, 2022 15:18
@iMacTia iMacTia changed the title Confluent Cloud support [WIP] Confluent Cloud support Mar 4, 2022
@dorner
Copy link
Member

dorner commented Mar 4, 2022

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
Copy link
Contributor Author

iMacTia commented Mar 4, 2022

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
Copy link
Member

dorner commented Mar 4, 2022

@iMacTia I have a fix for this here: #135 I'm going to wait for Derek to come back from vacation to review it, which should be mid next week.

@iMacTia
Copy link
Contributor Author

iMacTia commented Mar 4, 2022

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

@iMacTia
Copy link
Contributor Author

iMacTia commented Mar 5, 2022

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
Copy link
Member

dorner commented Mar 6, 2022

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
Copy link
Contributor Author

iMacTia commented Mar 6, 2022

Happy to give it a try 🙌! I'll open a PR to fix both 👌

@dorner
Copy link
Member

dorner commented Mar 8, 2022

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

@iMacTia iMacTia force-pushed the confluent-cloud-support branch 2 times, most recently from c9eb5a6 to 4eb3781 Compare March 9, 2022 17:40
@iMacTia iMacTia changed the title [WIP] Confluent Cloud support Confluent Cloud support Mar 9, 2022
@iMacTia
Copy link
Contributor Author

iMacTia commented Mar 9, 2022

@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
Copy link
Member

dorner commented Mar 9, 2022

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

@dorner dorner merged commit 45bc901 into flipp-oss:master Mar 9, 2022
@iMacTia iMacTia deleted the confluent-cloud-support branch March 9, 2022 18:11
@dorner
Copy link
Member

dorner commented Mar 9, 2022

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

@iMacTia
Copy link
Contributor Author

iMacTia commented Mar 9, 2022

Thanks for the support and the quick review 🙌 !

@tjsiron
Copy link

tjsiron commented Mar 31, 2022

@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
Copy link
Contributor Author

iMacTia commented Mar 31, 2022

@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!

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.

Confluent Cloud authentication
3 participants