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 can't authenticate by username and password when use Nacos #9941

Merged
merged 1 commit into from Jun 14, 2022

Conversation

xxxcrel
Copy link
Contributor

@xxxcrel xxxcrel commented Apr 19, 2022

What is the purpose of the change

This PR inspired by issue #9910, when nacos enabled authenticate,
RegistryConfig and ConfigCenterConfig setUsername and setPassword is not working

Brief changelog

org.apache.dubbo.registry.nacos.util.NacosNamingServiceUtils
org.apache.dubbo.configcenter.support.nacos.NacosDynamicConfiguration

Verifying this change

Checklist

  • Make sure there is a GitHub_issue field for the change (usually before you start working on it). Trivial changes like typos do not require a GitHub issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Check if is necessary to patch to Dubbo 3 if you are work on Dubbo 2.7
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
  • Add some description to dubbo-website project if you are requesting to add a feature.
  • GitHub Actions works fine on your own branch.
  • If this contribution is large, please follow the Software Donation Guide.

@xxxcrel
Copy link
Contributor Author

xxxcrel commented Apr 19, 2022

I tested three case:

config.setUsername("nacos");
config.setPassword("nacos");
config.setAddress("nacos://nacos:nacos@127.0.0.1:8848")
config.setAddress("nacos://nacos:nacos@127.0.0.1:8848?username=nacos&password=nacos")

only case 3 success, maybe it‘s just a trick🤔

@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2022

Codecov Report

Merging #9941 (042710b) into master (3a8e5e6) will decrease coverage by 0.85%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #9941      +/-   ##
============================================
- Coverage     60.92%   60.07%   -0.86%     
+ Complexity      449      445       -4     
============================================
  Files          1100     1100              
  Lines         44557    44565       +8     
  Branches       6486     6490       +4     
============================================
- Hits          27145    26771     -374     
- Misses        14446    14837     +391     
+ Partials       2966     2957       -9     
Impacted Files Coverage Δ
...enter/support/nacos/NacosDynamicConfiguration.java 0.00% <0.00%> (ø)
...o/registry/nacos/util/NacosNamingServiceUtils.java 41.42% <0.00%> (-2.52%) ⬇️
...ookeeper/ZookeeperDynamicConfigurationFactory.java 0.00% <0.00%> (-66.67%) ⬇️
...gcenter/wrapper/CompositeDynamicConfiguration.java 0.00% <0.00%> (-63.34%) ⬇️
...pport/zookeeper/ZookeeperDynamicConfiguration.java 0.00% <0.00%> (-60.00%) ⬇️
...fig/configcenter/TreePathDynamicConfiguration.java 31.42% <0.00%> (-54.29%) ⬇️
...a/org/apache/dubbo/rpc/filter/AccessLogFilter.java 28.35% <0.00%> (-50.75%) ⬇️
.../dubbo/metadata/report/MetadataReportInstance.java 12.50% <0.00%> (-40.63%) ⬇️
...va/org/apache/dubbo/rpc/support/AccessLogData.java 53.16% <0.00%> (-37.98%) ⬇️
.../configcenter/support/zookeeper/CacheListener.java 0.00% <0.00%> (-32.44%) ⬇️
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a8e5e6...042710b. Read the comment docs.

@AlbumenJ
Copy link
Member

Would you please add some uts for this?

@xxxcrel
Copy link
Contributor Author

xxxcrel commented Apr 22, 2022

em, It seems that connectivity to the nacos server cannot be unit tested

@chickenlj chickenlj added this to the 3.0.9 milestone Jun 14, 2022
@chickenlj chickenlj merged commit 9efe21a into apache:master Jun 14, 2022
@baymax55
Copy link

baymax55 commented Jul 7, 2022

before using it:

dubbo.registry.address=nacos://${nacos.url}?username=${spring.cloud.nacos.username}&password=${spring.cloud.nacos.password}&namespace=${spring.cloud.nacos.discovery.namespace}

after:

dubbo.registry.address=nacos://${nacos.url}?namespace=${spring.cloud.nacos.discovery.namespace}
dubbo.registry.username=username
dubbo.registr.password=password

@xxxcrel
Copy link
Contributor Author

xxxcrel commented Jul 7, 2022

before using it:

dubbo.registry.address=nacos://${nacos.url}?username=${spring.cloud.nacos.username}&password=${spring.cloud.nacos.password}&namespace=${spring.cloud.nacos.discovery.namespace}

after:

dubbo.registry.address=nacos://${nacos.url}?namespace=${spring.cloud.nacos.discovery.namespace}
dubbo.registry.username=username
dubbo.registr.password=password

You can still use the previous method

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.

None yet

5 participants