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

[redpanda] sasl test for wrong mechanism #2048

Merged
merged 4 commits into from Feb 2, 2024

Conversation

rwaweber
Copy link
Contributor

What does this PR do?

  • Adds a test that ensures that when using a different sasl mechanism from the broker, unsupported error is returned

Why is it important?

Would like to get another PR out to support SASL-PLAIN auth for redpanda's broker component. Figured I'd use this as a starting point for the discussion. Happy to file a separate issue for that if you'd prefer!

In the interest of not breaking existing calls by changing the name of the WithEnableSASL function, I was thinking that the SASL-PLAIN auth mechanism could be added by incorporating a WithEnablePlainSASL function for an option at the same level. What do you all think?

If amenable to that idea, you think adding a WithEnableScramSASL function could help clear up the distinction between them? I certainly don't feel strongly about this approach at all.

How to test this PR

  • Should be able to be picked up by the existing test harness without doing anything differently.

- ensure that when using a different sasl mechanism from the broker,
  unsupported error is returned
@rwaweber rwaweber requested a review from a team as a code owner December 22, 2023 20:04
Copy link

netlify bot commented Dec 22, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 9544f6a
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/65bc137316e4240008d39994
😎 Deploy Preview https://deploy-preview-2048--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@rwaweber rwaweber changed the title [redpanda] sasl test, wrong mechanism [redpanda] sasl test for wrong mechanism Dec 22, 2023
@mdelapenya
Copy link
Collaborator

Hi @rwaweber thanks for creating this PR. I was on Xmas PTO until this week, that's why I did not get to you before.

I've shared this PR, and your proposal for the SALS PLAIN mechanism, with the Redpanda folks, so they eventually jump in here with more insights.

Again, thanks for your time improving the module!

@rwaweber
Copy link
Contributor Author

No worries at all, hope you had a restful PTO!

Sounds great to me, and thank you for doing the hard work of maintaining it! It's certainly saved my bacon a few times, so I'm happy to help where I can.

@mdelapenya mdelapenya self-assigned this Feb 1, 2024
@mdelapenya mdelapenya added the chore Changes that do not impact the existing functionality label Feb 1, 2024
Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this addition! Do you think we could add a warning in the module docs or waiting for the follow-up PR with the PLAIN mechanism is fine?

@mdelapenya mdelapenya merged commit bdab82e into testcontainers:main Feb 2, 2024
80 checks passed
@mdelapenya
Copy link
Collaborator

Thanks for contributing this test! Much appreciated 👏

mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Feb 2, 2024
* main:
  [redpanda] sasl test for wrong mechanism (testcontainers#2048)
mdelapenya added a commit to gene-redpanda/testcontainers-go that referenced this pull request Feb 2, 2024
* main:
  feat(modules.clickhouse): Add zookeeper for clickhouse clusterization (testcontainers#1995)
  redpanda: allow using SASL and TLS together (testcontainers#2140)
  chore: do not panic in testable examples (testcontainers#2193)
  fix: all mounts should contain the testcontainers labels (testcontainers#2191)
  [redpanda] sasl test for wrong mechanism (testcontainers#2048)
  fix: deprecate BindMounts correctly (testcontainers#2190)
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Feb 14, 2024
* main: (33 commits)
  feat (postgres): support for creating and restoring Snapshots (testcontainers#2199)
  fix: apply volume options only to volumes (testcontainers#2201)
  redpanda/test: add admin client call (testcontainers#2200)
  chore(deps): bump cloud.google.com/go/spanner from 1.55.0 to 1.56.0 in /modules/gcloud, cloud.google.com/go/pubsub from 1.35.0 to 1.36.1 in /modules/gcloud, cloud.google.com/go/bigquery from 1.57.1 to 1.58.0 in /modules/gcloud (testcontainers#2197)
  chore(deps): bump github.com/docker/docker from 25.0.1+incompatible to 25.0.2+incompatible (testcontainers#2196)
  fix: go doc reference broken image (testcontainers#2195)
  Add Support for WASM Transforms to Redpanda Module (testcontainers#2170)
  feat(modules.clickhouse): Add zookeeper for clickhouse clusterization (testcontainers#1995)
  redpanda: allow using SASL and TLS together (testcontainers#2140)
  chore: do not panic in testable examples (testcontainers#2193)
  fix: all mounts should contain the testcontainers labels (testcontainers#2191)
  [redpanda] sasl test for wrong mechanism (testcontainers#2048)
  fix: deprecate BindMounts correctly (testcontainers#2190)
  chore(docker_mounts): stop doing misleading logging (testcontainers#2178)
  fix: Add HTTPStrategy WithForcedIPv4LocalHost To Fix Docker Port Map (testcontainers#1775)
  chore(deps): bump github.com/docker/compose/v2 in /modules/compose (testcontainers#2162)
  feat(modules.cockroachdb) Adds cockroachdb module (testcontainers#2131)
  chore(deps): bump golang.org/x/crypto in /modules/minio (testcontainers#2161)
  chore(deps): bump golang.org/x/crypto in /modules/openldap (testcontainers#2165)
  chore(deps): bump github.com/google/uuid from 1.5.0 to 1.6.0 (testcontainers#2169)
  ...
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Feb 14, 2024
* main: (33 commits)
  feat (postgres): support for creating and restoring Snapshots (testcontainers#2199)
  fix: apply volume options only to volumes (testcontainers#2201)
  redpanda/test: add admin client call (testcontainers#2200)
  chore(deps): bump cloud.google.com/go/spanner from 1.55.0 to 1.56.0 in /modules/gcloud, cloud.google.com/go/pubsub from 1.35.0 to 1.36.1 in /modules/gcloud, cloud.google.com/go/bigquery from 1.57.1 to 1.58.0 in /modules/gcloud (testcontainers#2197)
  chore(deps): bump github.com/docker/docker from 25.0.1+incompatible to 25.0.2+incompatible (testcontainers#2196)
  fix: go doc reference broken image (testcontainers#2195)
  Add Support for WASM Transforms to Redpanda Module (testcontainers#2170)
  feat(modules.clickhouse): Add zookeeper for clickhouse clusterization (testcontainers#1995)
  redpanda: allow using SASL and TLS together (testcontainers#2140)
  chore: do not panic in testable examples (testcontainers#2193)
  fix: all mounts should contain the testcontainers labels (testcontainers#2191)
  [redpanda] sasl test for wrong mechanism (testcontainers#2048)
  fix: deprecate BindMounts correctly (testcontainers#2190)
  chore(docker_mounts): stop doing misleading logging (testcontainers#2178)
  fix: Add HTTPStrategy WithForcedIPv4LocalHost To Fix Docker Port Map (testcontainers#1775)
  chore(deps): bump github.com/docker/compose/v2 in /modules/compose (testcontainers#2162)
  feat(modules.cockroachdb) Adds cockroachdb module (testcontainers#2131)
  chore(deps): bump golang.org/x/crypto in /modules/minio (testcontainers#2161)
  chore(deps): bump golang.org/x/crypto in /modules/openldap (testcontainers#2165)
  chore(deps): bump github.com/google/uuid from 1.5.0 to 1.6.0 (testcontainers#2169)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Changes that do not impact the existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants