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

Fix SASL and TLS issues reported in #56 and #84 #86

Merged
merged 6 commits into from
Jun 13, 2022

Conversation

mostafa
Copy link
Owner

@mostafa mostafa commented Jun 12, 2022

In this PR, I tried to fix the issue reported in the following issues.

The PR includes changes, fixes, and features as follows:

  • Fixes to how SASL and TLS configs work:
    While refactoring the extension on several occasions over the past few months, I tried to improve the extension. Still, I somehow broke SASL and TLS configuration for the unauthenticated client (dialer) with the default TLS configuration (and introduced a regression). As I mentioned in xk6-kafka supports SASL_SSL authentication to confluent cloud? #56 (comment) and xk6-kafka supports SASL_SSL authentication to confluent cloud? #56 (comment), the problem manifested itself while a default TLS configuration was needed, but the extension was looking for certificate and keys to be passed explicitly. This is now fixed.
  • Automatic topic creation:
    I also introduced a new option to the produce function to allow automatic topic creation on the first message.
  • Passing values directly from JS to Go in functions as JSON:
    I also experimented with passing struct fields directly from JS to Go based on the json field in the Go struct, and it was successful 🎉, which means that I'll refactor the rest of the API to accept JSON, instead of stringified JSON. This was an effort started by @iamelevich in Update APIs to accept JSON object instead of stringified JSON object #20, but I intervened and made the PR a mess. 🤦 So, to make amends, I'll refactor all the APIs to receive JSON to make things easier, both for developers and users/testers.
  • Update SASL constants:
    Changes can be seen here and are reflected in the test_sasl_auth.js script. Eventually, all of them will be exported at the module level: Export all constants to JS #70.
  • Separated SASL and TLS configs:
    An example is available here.
  • Fix a bug on multiple occasions:
    Almost all the scripts contained a bug that prevented the correct message to be printed after the topic is deleted, which is now fixed.
  • Proper JS API documentation: 📖
    Since the API is changing rapidly, I introduced the index.d.ts file and the mechanism to generate JS API docs in the docs directory in Add API docs with typedoc #87.
  • Test against Confluent Cloud and kafka-docker-playground:
    I tried Confluent Cloud with SASL Plain and TLS v1.2 and it worked perfectly and SASL Plain (with no TLS) with kafka-docker-playgrounds's sasl-plain environment.

@fdahunsibread @thanapat-sk @anjosanap @Momotoculteur @kusumkappdirect
Please test this PR, so I can make sure it works for you as well. I'd be happy to have your review as well if you're into Go.

@sonarcloud
Copy link

sonarcloud bot commented Jun 12, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 8 Code Smells

No Coverage information No Coverage information
2.5% 2.5% Duplication

@mostafa mostafa marked this pull request as ready for review June 12, 2022 16:45
@kusumkappdirect
Copy link

What is image version here ?

@mostafa
Copy link
Owner Author

mostafa commented Jun 13, 2022

@kusumkappdirect
There are no images, you need to build the source yourself, as I explained here. However, in this specific case, you also need to switch the branch. Provided that you have Go installed, run these commands:

git clone git@github.com:mostafa/xk6-kafka.git && cd xk6-kafka
git checkout fix-sasl-and-tls-config
xk6 build --with github.com/mostafa/xk6-kafka@latest=.

@kusumkappdirect
Copy link

kusumkappdirect commented Jun 13, 2022

Is this the correct way to install

FROM golang:1.18-alpine as builder
RUN apk add git
RUN apk add openssh-client
RUN mkdir /app
WORKDIR /app
RUN go install go.k6.io/xk6/cmd/xk6@latest
RUN git clone git@github.com:mostafa/xk6-kafka.git && cd xk6-kafka
RUN git checkout fix-sasl-and-tls-config
RUN xk6 build --with github.com/mostafa/xk6-kafka@latest=.

I am getting error ssh: Could not resolve hostname github.com: Name does not resolve

@kusumkappdirect
Copy link

I have to run these git command in dockerfile
I am getting error ssh: Could not resolve hostname github.com: Name does not resolve

@mostafa
Copy link
Owner Author

mostafa commented Jun 13, 2022

@kusumkappdirect This means that your docker build cannot resolve the address inside the build container due to DNS resolve issue. Try the git clone command locally (on your host) to see if it works. If it does, then use another base image for your Go image. Or set the /etc/resolv.conf to nameserver 8.8.8.8 in your Dockerfile.

@anjosanap
Copy link

In this PR, I tried to fix the issue reported in the following issues.

The PR includes changes, fixes, and features as follows:

@fdahunsibread @thanapat-sk @anjosanap @Momotoculteur @kusumkappdirect Please test this PR, so I can make sure it works for you as well. I'd be happy to have your review as well if you're into Go.

Hi, I cloned the repo and did some tests on the fix-sasl-and-tls-config branch and worked perfectly 🥳

Now I’m validating other scenarios that I have here, but with a simple scenario using the SASL_SSL and TLS configuration worked without problems (same scenario that didn’t work versions later than v0.8) ✅

Thanks for the speed and precision in dealing with this problem 🤝✨

@mostafa
Copy link
Owner Author

mostafa commented Jun 13, 2022

@anjosanap I'm really glad to hear that, and thank you for the feedback! 🙏
Since I got a confirmation from you, I'll merge this PR, so it can be tested on the main branch. If others still want to test it, follow the instructions on the README.

@kusumkappdirect
Copy link

kusumkappdirect commented Jun 14, 2022

I am not getting "No TLS config provided. ,cannot create a kafka writer with a nil address this error now , but i am getting level=error msg="Unable to unmarshal credentials, OriginalError: %!w(*json.SyntaxError=&{invalid character 'o' looking for beginning of value 2})" error="Unable to unmarshal credentials, OriginalError: %!w(*json.SyntaxError=&{invalid character 'o' looking for beginning of value 2})"

const username = [__ENV.KAFKA_USERNAME];
const password = [__ENV.KAFKA_PASSWORD];

console.log("Username"+username);
console.log("Password"+password);

const saslConfig = {
    "username": username,
    "password": password,
    "algorithm": "sasl_ssl"
};

const tlsConfig = {
    "enableTLS": true,
    "insecureSkipTLSVerify": true,
    "minVersion": "TLSv1.2"
};
const [producer,_writerError] = writer( bootstrapServers, topic, saslConfig, tlsConfig);

@mostafa
Copy link
Owner Author

mostafa commented Jun 14, 2022

@kusumkappdirect Are you building and running the code on the latest main branch? The message you're getting is from the older version, v0.10.0.

@kusumkappdirect
Copy link

kusumkappdirect commented Jun 14, 2022

I am using xk6-kafka with @latest tag ..i hope that fetch the latest ?
RUN xk6 build --with github.com/mostafa/xk6-kafka@latest

@mostafa
Copy link
Owner Author

mostafa commented Jun 14, 2022

@kusumkappdirect That fetches the latest tagged version, which is v0.10.0. Hence the git clone and =. at the end of the xk6 command.

@mostafa
Copy link
Owner Author

mostafa commented Jun 20, 2022

Released in v0.11.0.

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.

3 participants