[security] Allow config client dns bind addr and port #13390
Conversation
github-actions
bot
added
the
doc
shoothzj
force-pushed
the
security-allow-config-dns-bind-addr-port
branch
2 times, most recently
from
266069d
to
b6ba69e
Compare
|
/pulsarbot run-failure-checks |
codelipenghui
requested review from
michaeljmarshall,
Technoboy-,
315157973,
BewareMyPower,
gaoran10 and
merlimat
|
@codelipenghui @lhotari @michaeljmarshall @merlimat @315157973 @hangc0276 PTAL, thanks |
pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ClientConfigurationData.java
Outdated
Show resolved
Hide resolved
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.
pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ClientConfigurationData.java
Outdated
Show resolved
Hide resolved
pulsar-client/src/main/java/org/apache/pulsar/client/impl/conf/ClientConfigurationData.java
Show resolved
Hide resolved
| @@ -295,6 +295,19 @@ | |||
| @JsonIgnore | |||
| private Clock clock = Clock.systemDefaultZone(); | |||
|
|
|||
| @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.
shoothzj
force-pushed
the
security-allow-config-dns-bind-addr-port
branch
from
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 |
shoothzj
force-pushed
the
security-allow-config-dns-bind-addr-port
branch
from
70f4a33
to
ea0ef42
Compare
|
@michaeljmarshall ping |
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.
| @@ -295,6 +295,19 @@ | |||
| @JsonIgnore | |||
| private Clock clock = Clock.systemDefaultZone(); | |||
|
|
|||
| @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.