Fix endpoint scheme detection for TLS origination to PD #977
Conversation
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
|
Thanks for the contribution! |
|
The change looks reasonable. I've tried TLS configuration manually and it did fix the issue. |
|
@bb7133: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
LGTM, but I'm not a reviewer :) |
|
PTAL @xhebox and @breeswish |
There was a problem hiding this comment.
Thanks for your contribution @LINKIWI ! Changes LGTM.
For testing, it seems we must test the config logic as a whole since the order is significant. May with suggestion of @shhdgit & @breeswish we can push a follow up to add such test. I'd prefer avoid expanding a lot this PR since there is no test yet.
|
@tisonkun: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
@xhebox: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
LGTM! Sorry for the lack of tests. Additional tests on config initialization are welcome and may require some code restructuring. Improving test coverage is already in our roadmap. |
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: 25e28e9
|
* feat(ui): add timezone information (#974) * Fix endpoint scheme detection for TLS origination to PD (#977) * build(deps): bump path-parse from 1.0.6 to 1.0.7 in /ui (#978) Bumps [path-parse](https://github.com/jbgutierrez/path-parse) from 1.0.6 to 1.0.7. - [Release notes](https://github.com/jbgutierrez/path-parse/releases) - [Commits](https://github.com/jbgutierrez/path-parse/commits/v1.0.7) --- updated-dependencies: - dependency-name: path-parse dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump path-parse from 1.0.6 to 1.0.7 in /ui/tests (#981) * Bump golangci to get the latest lints & fix lints (#984) * Ci cherry pick version (#989) * chore: add distro translation (#995) * chore: add distro translation * fix: lowercase * update: release version Co-authored-by: LINKIWI <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
NormalizePDEndPointis responsible for setting the URL scheme of the PD endpoint (ref). However, it's currently called beforecfg.CoreConfig.ClusterTLSConfigis set, which causes Dashboard to make plaintext HTTP requests to PD all the time, regardless of whether--cluster-[ca|cert|key]flags are set.This change fixes the TLS origination behavior by running this logic after
cfg.CoreConfig.ClusterTLSConfigis (optionally) populated bybuildTLSConfig. This allows Dashboard to properly use HTTPS to PD when cluster TLS is enabled.I didn't find any existing unit tests to cover this change, but if there's a test I can add or update to cover this, I'm happy to include it in this PR.