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

hubble: Use a single string to configure the server address #11330

Merged
merged 3 commits into from May 8, 2020

Conversation

michi-covalent
Copy link
Contributor

  • Rename the option to --hubble-listen-address to make it clear that
    Hubble listens to at most one additional address.
  • Remove an info message that doesn't provide any addtional info:
      level=info msg="Starting local Hubble server" address="unix:///var/run/cilium/hubble.sock" subsys=hubble
      level=info msg="Starting Hubble server" address=":4244" subsys=hubble
    - level=info msg="Starting gRPC server on listener" listener="unix:///var/run/cilium/hubble.sock" subsys=hubble
    - level=info msg="Starting gRPC server on listener" listener=":4244" subsys=hubble
    
  • Use port 4244 in the documentation since this is the port that Hubble
    relay expects.

Signed-off-by: Michi Mutsuzaki michi@isovalent.com

@michi-covalent michi-covalent added release-note/misc This PR makes changes that have no direct user impact. sig/hubble Impacts hubble server or relay area/helm Impacts helm charts and user deployment experience labels May 5, 2020
@michi-covalent michi-covalent requested review from a team May 5, 2020 00:11
@michi-covalent michi-covalent requested review from a team as code owners May 5, 2020 00:11
@michi-covalent michi-covalent requested a review from a team May 5, 2020 00:11
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 May 5, 2020
@michi-covalent
Copy link
Contributor Author

Added another commit to to change hubble-ui to connect to hubble-relay.

@coveralls
Copy link

coveralls commented May 5, 2020

Coverage Status

Coverage increased (+0.03%) to 42.66% when pulling 4609feb on pr/michi/address into 2ff7193 on master.

Copy link
Contributor

@soumynathan soumynathan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@soumynathan soumynathan left a comment

Choose a reason for hiding this comment

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

LGTM

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.

It looks good but I think we can simplify this further.

daemon/cmd/hubble.go Outdated Show resolved Hide resolved
@gandro gandro added this to In progress (1.8) in Hubble server May 5, 2020
@michi-covalent michi-covalent force-pushed the pr/michi/address branch 2 times, most recently from bc6007c to 9deccf9 Compare May 5, 2020 20:42
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.

Awesome! Please, just remove the if check as it is superfluous now.

daemon/cmd/hubble.go Outdated Show resolved Hide resolved
pkg/hubble/server/server.go Outdated Show resolved Hide resolved
@michi-covalent
Copy link
Contributor Author

hm i think i broke something. fixing now

@michi-covalent
Copy link
Contributor Author

ready for review cc @rolinh

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.

LGTM, thanks!

@rolinh
Copy link
Member

rolinh commented May 6, 2020

test-me-please

1 similar comment
@michi-covalent
Copy link
Contributor Author

test-me-please

@gandro
Copy link
Member

gandro commented May 7, 2020

restart-ginkgo

@maintainer-s-little-helper
Copy link

Commit abc0caf202b7d10c8b6d31ac7b4fdbf91f791c33 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 7, 2020
@maintainer-s-little-helper
Copy link

Commit abc0caf202b7d10c8b6d31ac7b4fdbf91f791c33 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@michi-covalent michi-covalent removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 7, 2020
@michi-covalent
Copy link
Contributor Author

test-with-kernel

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.

Not sure if I missed the leftover print debugging statement during my last review or if you added them with a force push but I'm pretty sure we don't want to keep them :)

pkg/hubble/server/serveroption/option.go Outdated Show resolved Hide resolved
pkg/hubble/server/serveroption/option.go Outdated Show resolved Hide resolved
- Rename the option to `--hubble-listen-address` to make it clear that
  Hubble listens to at most one additional address.
- Remove an info message that doesn't provide any addtional info:
    ```
      level=info msg="Starting local Hubble server" address="unix:///var/run/cilium/hubble.sock" subsys=hubble
      level=info msg="Starting Hubble server" address=":4244" subsys=hubble
    - level=info msg="Starting gRPC server on listener" listener="unix:///var/run/cilium/hubble.sock" subsys=hubble
    - level=info msg="Starting gRPC server on listener" listener=":4244" subsys=hubble
    ```
- Use port 4244 in the documentation since this is the port that Hubble
  relay expects.

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
- Configure hubble-ui to connect to hubble-relay:80. Now hubble-relay is
  responsible for retrieving flows from Cilium instances.
- Remove hubble-grpc service.

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
Now that Hubble has 1 unix domain socket and at most 1 TCP socket to
serve, we can simply use `With{UnixSocket,TCP}Listener` options.

Signed-off-by: Michi Mutsuzaki <michi@isovalent.com>
@rolinh
Copy link
Member

rolinh commented May 8, 2020

The test failure is unrelated to the PR, should be fixed by #11427

@rolinh rolinh merged commit f796c2b into master May 8, 2020
1.8.0 automation moved this from In progress to Merged May 8, 2020
Hubble server automation moved this from In progress (1.8) to Done May 8, 2020
@rolinh rolinh deleted the pr/michi/address branch May 8, 2020 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Impacts helm charts and user deployment experience release-note/misc This PR makes changes that have no direct user impact. sig/hubble Impacts hubble server or relay
Projects
No open projects
1.8.0
  
Merged
Hubble server
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants