-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
allow to set https as the default scheme #8729
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8729 +/- ##
============================================
+ Coverage 69.66% 69.74% +0.08%
+ Complexity 4616 4614 -2
============================================
Files 1730 1730
Lines 90341 90325 -16
Branches 13442 13425 -17
============================================
+ Hits 62932 63001 +69
+ Misses 23045 22954 -91
- Partials 4364 4370 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Consider making it more explicit (as boolean)?
- controller.swagger.use.https
- pinot.broker.swagger.use.https
- pinot.server.swagger.use.https
- pinot.minion.swagger.use.https
@@ -89,7 +91,11 @@ private void setupSwagger() { | |||
beanConfig.setDescription("APIs for accessing Pinot broker information"); | |||
beanConfig.setContact("https://github.com/apache/pinot"); | |||
beanConfig.setVersion("1.0"); | |||
beanConfig.setSchemes(new String[]{CommonConstants.HTTP_PROTOCOL, CommonConstants.HTTPS_PROTOCOL}); | |||
if (CommonConstants.HTTPS_PROTOCOL.equalsIgnoreCase(_swaggerDefaultProtocol)) { | |||
beanConfig.setSchemes(new String[]{CommonConstants.HTTPS_PROTOCOL, CommonConstants.HTTP_PROTOCOL}); |
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.
Should we still set HTTP?
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.
yeah, still needed at least for those quickstart examples, and perhaps for users not enabling TLS.
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.
oh, got your point. Taking the new configs suggested by you above, we ca save http here.
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.
hmm... on a second thought, I'd prefer to leave http
there, in case someone wants to debug issues like when TLS doesn't work and he/she wants to switch to http for some quick checks.
@@ -63,6 +63,7 @@ public class ControllerConf extends PinotConfiguration { | |||
public static final String HELIX_CLUSTER_NAME = "controller.helix.cluster.name"; | |||
public static final String CLUSTER_TENANT_ISOLATION_ENABLE = "cluster.tenant.isolation.enable"; | |||
public static final String CONSOLE_WEBAPP_ROOT_PATH = "controller.query.console"; | |||
public static final String CONSOLE_WEBAPP_DEFAULT_PROTOCOL = "controller.query.console.default.protocol"; |
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.
Why is controller key different from others? I don't think it applies to the query console
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.
I think this is some legacy naming convention, so I just kept it for consistency.
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.
but I'll take your controller.swagger.use.https
This PR adds a switch to set 'https' as the default scheme for Swagger UI, so that in TLS enabled env, requests issued from Swagger UI will use https automatically.
Release notes
New configs are added for each Pinot component and followed the existing naming convention. By default,
http
is used as before.