Conversation
|
test-me-please |
There was a problem hiding this comment.
IMO it'd be helpful if the commit and PR message provided a one-liner of the intended scope of this component. Even something simple like "hubble-proxy is a new component to safely expose the Hubble API to remote peers".
Please also extend the CODEOWNERS file with a group responsible for owning this module. Feel free to reach out to @aanm or I if you think we need a new team in the cilium organisation to handle reviews and code ownership.
| return fmt.Errorf("too many arguments; expected only the shell type: %s", args) | ||
| } | ||
|
|
||
| if len(args) == 0 || args[0] == "bash" { |
There was a problem hiding this comment.
I noticed there's some subtle differences between the completion code here and the ones in cmd/root.go, is it reasonable to find a way to share this?
There was a problem hiding this comment.
Ah, yes it's because when one runs completion with a wrong argument (eg hubble-proxy completion foo), then the copyright header was being written anyway before the error was printed out and it felt weird. I can create a PR to fix this for cilium's cli as well.
There was a problem hiding this comment.
Thanks for the fix. Can we refactor / share this or is that painful for some reason (eg the way the viper context is passed around or something?)
|
Apparently on the At least right now, GitHub is telling me that I'm a codeowner for all of the files in the review page, which suggests the janitor will be pulled in for any subsequent PRs. |
The relevant codeowners were pulled in because
Indeed. I updated the commit message and PR description with more details. |
6baf7f8 to
e23c291
Compare
Done. |
|
test-me-please |
joestringer
left a comment
There was a problem hiding this comment.
The new description is really helpful, thanks!
I have no general objections from janitor perspective, but i'll defer to @cilium/hubble to review and resolve any outstanding questions.
bc2b919 to
6978abd
Compare
|
test-me-please EDIT: vm provisioning fail, wil re-run |
|
test-me-please |
6978abd to
f73f226
Compare
|
test-me-please |
glibsm
left a comment
There was a problem hiding this comment.
As a skeleton looks good. The remarks are not blocking but are better saved for the upcoming PRs
| Use: "hubble-proxy", | ||
| Short: "hubble-proxy is a proxy server for the hubble API", | ||
| Long: "hubble-proxy is a proxy server for the hubble API.", | ||
| SilenceUsage: true, |
There was a problem hiding this comment.
By default, usage is always being displayed when a command returns an error, whether that's a user input issue or any other error. It'd be OK to display it when a command is called with invalid arguments but it feels rather odd to display it when it's an internal error such as a connection timeout or else.
|
@joestringer @glibsm Reading through the review comments, it wasn't clear to me whether we should merge this now and follow-up or require some of the comments to be addressed now. I assume Robin will look through and address what's obvious, it would be great for both of you to point out what you want to be addressed now vs later after Robin has refreshed the PR. |
f73f226 to
2f5f2d0
Compare
This commit adds code to create a binary, `hubble-proxy`, for the new hubble proxy. This component is a new component that will be required for hubble's multi-node feature. Notably, the introduction of hubble proxy will: - Allow sharing the hubble API externally via load balancer or ingress rules. - Give flexibility in terms of scheduling and resource allocation. - Make it easy to discover from Hubble UI (k8s service) or other clients such as the hubble CLI. - Make it potentially easier to implement API rate limiting to protect the cluster. - Decouple security from Cilium (CNPs can be applied). The hubble proxy will talk directly with the hubble API on the cilium agent running on the same node. This will allow initial hubble peers discovery but also to monitor for peers joining/leaving the cluster or to get information about which peer to connect to to get flows based on the request (typically for long running requests). The hubble proxy will connect to hubble peers in order to get relevant flows according to the flow request to process. Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
Signed-off-by: Robin Hahling <robin.hahling@gw-computing.net>
2f5f2d0 to
5c21cee
Compare
|
test-me-please |
|
The specific items I was concerned about are already addressed. I had a minor longer-term maintenance question on refactoring the basic cmd code here to better share, which I raised because this PR fixed an issue in the Cilium side as well and we just applied the same fix there: That said, the relevant code doesn't change often so it's not a bit deal to have multiple copies of it in the codebase. |
This commit adds code to create a binary,
hubble-proxy, for the newhubble proxy. This component is a new component that will be required
for hubble's multi-node feature.
Notably, the introduction of hubble proxy will:
rules.
clients such as the hubble CLI.
the cluster.
The hubble proxy will talk directly with the hubble API on the cilium
agent running on the same node. This will allow initial hubble peers
discovery but also to monitor for peers joining/leaving the cluster or
to get information about which peer to connect to to get flows based on
the request (typically for long running requests). The hubble proxy will
connect to hubble peers in order to get relevant flows according to the
flow request to process.
Ref: https://github.com/cilium/hubble/issues/89
This change is