-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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][fn] JavaInstanceStarter --tls_allow_insecure default to false #20267
[fix][fn] JavaInstanceStarter --tls_allow_insecure default to false #20267
Conversation
/pulsarbot rerun-failure-checks |
/pulsarbot rerun-failure-checks |
Codecov Report
@@ Coverage Diff @@
## master #20267 +/- ##
============================================
- Coverage 72.91% 72.64% -0.27%
+ Complexity 31913 31859 -54
============================================
Files 1867 1867
Lines 138534 139993 +1459
Branches 15200 15507 +307
============================================
+ Hits 101009 101697 +688
- Misses 29517 30211 +694
- Partials 8008 8085 +77
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
public String tlsAllowInsecureConnection = Boolean.TRUE.toString(); | ||
public String tlsAllowInsecureConnection = Boolean.FALSE.toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lhotari - I finally understand why this defaults to arity 1, not 0. It's because the field is a String
, not a Boolean
or a boolean
. Therefore, this change was correct.
Motivation
Pulsar's defaults for allowing insecure connections are always
false
except for in this one case in theJavaInstanceStarter
. A more secure default is to set it tofalse
, so I propose we change this default.In this case, we have a default that is likely never used unless someone manually uses the
JavaInstanceStarter
. As such, I propose we update this default without requiring a PIP.Modifications
--tls_allow_insecure
in theJavaInstanceStarter
classfalse
.Verifying this change
I will verify that tests pass.
Does this pull request potentially affect one of the following parts:
Documentation
doc-not-needed
These docs are generated.
Matching PR in forked repository
PR in forked repository: michaeljmarshall#43