Default to empty username & password

varas commented 4 years ago

Description of the changes

Default values for username & password should be empty.

Args do not distinguish between empty and no value, so when values are not provided default values are used. Sane default values should be no username, nor password. Otherwise connection might fail.

PR Review Checklist

Author

[x] the PR should focus on a single subject. Change only relevant files to the problem you’re working on Clean and format the code
[ ] add unit tests for your changes and make sure all unit tests are passing
[x] add a risk label after carefully considering the "blast radius" of your changes
[x] describe the intent of your changes in the description. don't just rewrite your code in prose
[x] assign at least one reviewer
[ ] address the feedback

Reviewer

[ ] review code for readability
[ ] verify that high risk behavior changes are well tested
[ ] check license for any new external dependency
[ ] ask questions about anything that isn't clear and obvious
[ ] approve the PR when you consider it's good to merge

CLAassistant commented 4 years ago

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Juan Hernandez seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

camdencheek commented 4 years ago

@varas If I remember correctly, it was implemented this way because the SDK would fail if username and password were empty, but in most cases JMX wouldn't complain if user/pass were set if auth wasn't enabled. Have you tested this against an instance with no auth?

varas commented 4 years ago

Yes I tested this against a JMX server without authentication. Also, sdk args package enforces default value when there's an empty value. Therefore without args package update there's no way to provide no username/pass if default values aren't empty :(

varas commented 4 years ago

I tested running nri-jmx agains an endpoint with no auth and it fails because of this admin/admin enforcement. And it works just fine with this update.

newrelic / nri-jmx