[security] Allow config client dns bind addr and port#13390
Conversation
266069d to
b6ba69e
Compare
|
/pulsarbot run-failure-checks |
|
@codelipenghui @lhotari @michaeljmarshall @merlimat @315157973 @hangc0276 PTAL, thanks |
There was a problem hiding this comment.
I am +1 on this change, but I think a few things need to be addressed first.
@shoothzj - Have you tested out this change? Your PR description doesn't mention test coverage.
|
|
||
| @ApiModelProperty( | ||
| name = "dnsLookupBindAddress", | ||
| value = "The Pulsar client dns lookup bind address." |
There was a problem hiding this comment.
Can you add the default value and that it means the client will bind to 0.0.0.0?
There was a problem hiding this comment.
Can you add the default value and that it means the client will bind to
0.0.0.0?
@michaeljmarshall the default behavior is similar, but it's not bind to 0.0.0.0.
if (localAddress == null) {
future = b.register();
} else {
future = b.bind(localAddress);
}There was a problem hiding this comment.
I think I might be missing something. Since the default behavior isn't changed by this PR, doesn't that mean we will still bind to 0.0.0.0? My only request here is that we update this annotation with a little more documentation about the default behavior.
There was a problem hiding this comment.
@michaeljmarshall I misunderstood your meaning, added the defautl behavior notion
There was a problem hiding this comment.
No worries, thanks for updating it.
b6ba69e to
70f4a33
Compare
|
@michaeljmarshall Currently, I test it locally, and have some configuration read, write test. It's hard to write unit test for this pr, need a mock dns server and ensure the test machine has different network interface. |
|
/pulsarbot run-failure-checks |
70f4a33 to
ea0ef42
Compare
|
@michaeljmarshall ping |
michaeljmarshall
left a comment
There was a problem hiding this comment.
@shoothzj - thanks for updating the test and confirming that you tested locally.
I have one last question about documentation, and then I'll approve this PR.
|
|
||
| @ApiModelProperty( | ||
| name = "dnsLookupBindAddress", | ||
| value = "The Pulsar client dns lookup bind address." |
There was a problem hiding this comment.
I think I might be missing something. Since the default behavior isn't changed by this PR, doesn't that mean we will still bind to 0.0.0.0? My only request here is that we update this annotation with a little more documentation about the default behavior.
### Motivation By default, the pulsar client dns start listen on `0.0.0.0`, which could be a security risk ### Modifications - Allow config client dns bind addr and port - And default behavior is not changed
Motivation
By default, the pulsar client dns start listen on
0.0.0.0, which could be a security riskModifications
Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
docIMO, this change to code will generated doc automatically.