Skip to content
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: conditionally change hubble-relay port in hubble-ui #16511

Merged
merged 1 commit into from Jun 21, 2021
Merged

fix: conditionally change hubble-relay port in hubble-ui #16511

merged 1 commit into from Jun 21, 2021

Conversation

alex1989hu
Copy link
Contributor

@alex1989hu alex1989hu commented Jun 11, 2021

In case of enabled TLS for Hubble Relay the hubble-ui
shall follow the service port change

Signed-off-by: Alex Szakaly alex.szakaly@gmail.com

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

Fixes: #16510

conditionally change hubble relay port in hubble-ui

In case of enabled TLS for Hubble Relay the hubble-ui
shall follow the service port change

Fixes #16510

Signed-off-by: Alex Szakaly <alex.szakaly@gmail.com>
@alex1989hu alex1989hu requested a review from a team as a code owner June 11, 2021 14:46
@alex1989hu alex1989hu requested review from a team, kaworu and nathanjsweet June 11, 2021 14:46
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 11, 2021
@alex1989hu alex1989hu changed the title fix: conditionally change hubble relay port in hubble-ui fix: conditionally change hubble-relay port in hubble-ui Jun 11, 2021
@gandro gandro added release-note/misc This PR makes changes that have no direct user impact. sig/hubble Impacts hubble server or relay labels Jun 14, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 14, 2021
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please not that the UI does not yet support connecting to Relay via mTLS. While this change by itself will be eventually needed, it is not sufficient to run UI with Values.hubble.relay.tls.server.enabled=true.

Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What @gandro mentioned is indeed valid and something to keep in mind. However, as of now, users enabling TLS for Hubble Relay will already notice that the Hubble UI will stop working when doing so. With this change however, the error might be more clear as the connection should fail with a TLS error instead of timing out. TLDR: I think this is OK to merge even though Hubble UI does not yet support TLS.

@rolinh
Copy link
Member

rolinh commented Jun 21, 2021

Given that Hubble UI is not deployed anywhere in our CI, a full CI run is unnecessary. As this PR got all required approvals from the various teams, marking as ready to merge.

@rolinh rolinh added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 21, 2021
@gandro gandro removed the request for review from nathanjsweet June 21, 2021 13:06
@jibi jibi merged commit 44866f3 into cilium:master Jun 21, 2021
@alex1989hu alex1989hu deleted the fix/conditionally-change-hubble-relay-port-in-hubble-ui branch June 22, 2021 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/hubble Impacts hubble server or relay
Projects
None yet
Development

Successfully merging this pull request may close these issues.

helm: hubble-ui shall conditionally set FLOWS_API_ADDR hubble-relay svc port
8 participants