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

Use default ssl config fo HiveMQ client #381

Merged

Conversation

Myshkouski
Copy link
Contributor

See #380.

@CLAassistant
Copy link

CLAassistant commented Mar 3, 2024

CLA assistant check
All committers have signed the CLA.

@sdelamo sdelamo requested a review from jeremyg484 April 1, 2024 10:26
@sdelamo sdelamo added the type: improvement A minor improvement to an existing feature label Apr 1, 2024
Copy link
Contributor

@jeremyg484 jeremyg484 left a comment

Choose a reason for hiding this comment

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

@Myshkouski Thanks for submitting!

I think the code looks fine, but this needs a couple of additions before we can approve and merge it:

  1. There needs to be test coverage for this new path.

  2. We should also include an update to the documentation that describes this option and the differences to setting the SSL configuration explicitly.

Would you be able to submit these needed updates to the PR? I'm happy to guide you along as needed.

@jeremyg484 jeremyg484 self-assigned this Apr 2, 2024
@Myshkouski
Copy link
Contributor Author

Myshkouski commented Apr 2, 2024

@jeremyg484 Please review the following changes:

  1. There needs to be test coverage for this new path.

Seems SSL configuration for HiveMQ client is not covered at all. I added 2 test suites:

  • test-suite-hivemq-mqttv5-graal-mtls - test with mqtt.client.ssl.* properties (f6670e3);
  • test-suite-hivemq-mqttv5-graal-no-mtls - test without mqtt.client.ssl.* properties (4b02e52).

Both are based on existing test-suite-mqttv5-graal-ssl test suite.

Now coverage for io.micronaut.mqtt.hivemq.v5.client.Mqtt5ClientFactory#buildTransportConfig looks like this:

image_2024-04-02_13-20-43

@Myshkouski
Copy link
Contributor Author

Myshkouski commented Apr 2, 2024

2. We should also include an update to the documentation that describes this option and the differences to setting the SSL configuration explicitly.

Documentation already provides description for mqtt.client.server-uri option as:

mqtt.client.server-uri | The URI of server to connect to as [schema]://[serverHost]:[serverPort].

We could explicitly add what values are allowed for schema - tcp or ssl.
I think no additional docs about using SSL by default is required (that was the point of the issue).

@jeremyg484
Copy link
Contributor

Seems SSL configuration for HiveMQ client is not covered at all. I added 2 test suites:

  • test-suite-hivemq-mqttv5-graal-mtls - test with mqtt.client.ssl.* properties (f6670e3);
  • test-suite-hivemq-mqttv5-graal-no-mtls - test without mqtt.client.ssl.* properties (4b02e52).

@Myshkouski Excellent, thank you!

@jeremyg484
Copy link
Contributor

@Myshkouski I was more thinking of the SSL Connections section of the docs. Is there anything we should explain differently there?

@jeremyg484 jeremyg484 merged commit 2751e60 into micronaut-projects:master Apr 4, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: improvement A minor improvement to an existing feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants