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.

Sign up for GitHub

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

Jump to bottom

cli: Add "cilium bpf proxy list" command #2504

Merged
merged 1 commit into from Jan 16, 2018
Merged

cli: Add "cilium bpf proxy list" command #2504

merged 1 commit into from Jan 16, 2018

Conversation

vadorovsky
Copy link
Member

@vadorovsky vadorovsky commented Jan 12, 2018 •
edited

The command which dumps proxy BPF maps for debugging purposes.

Example (with Cilium, Envoy and Kubernetes):

$ kubectl create -f examples/kubernetes-grpc/cc-door-app.yaml
deployment "cc-door-mgr" created
service "cc-door-server" created
pod "terminal-87" created
$ kubectl create -f examples/kubernetes-grpc/cc-door-ingress-security.yaml
$ sudo cilium bpf proxy list
PROXY                             DESTINATION
10.0.118.183:39808 (6) => 10000   10.0.202.84:50051
10.0.118.183:39794 (6) => 10000   10.0.202.84:50051
10.0.118.183:39822 (6) => 10000   10.0.202.84:50051

Fixes #2277

Signed-off-by: Michal Rostecki mrostecki@suse.com

@vadorovsky vadorovsky requested a review from a team as a code owner January 12, 2018 13:03
@ciliumbot
Copy link

Can one of the admins verify this patch?

@vadorovsky vadorovsky requested a review from a team as a code owner January 12, 2018 13:03
@ciliumbot
Copy link

Can one of the admins verify this patch?

@aalemayhu
Copy link
Contributor

test-me-please

aalemayhu
aalemayhu suggested changes Jan 12, 2018
View reviewed changes
Copy link
Contributor

@aalemayhu aalemayhu left a comment

Choose a reason for hiding this comment

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

Can you add an example of the output in the commit message? 10 lines would be good.

Proxy                Destination
--------------------------------
[f00d::a69:0:0:981b]:53208 (6) => 10000 f00d::a69:0:0:f236:80
[f00d::a69:0:0:981b]:58082 (6) => 10003 f00d::a69:0:0:f236:80
[f00d::a69:0:0:981b]:58056 (6) => 10000 f00d::a69:0:0:f236:80
[fc00::10ca:1]:40288 (6) => 10003 f00d::a69:0:0:dc06:80
[f00d::a69:0:0:981b]:58116 (6) => 10000 f00d::a69:0:0:f236:80
[f00d::a69:0:0:981b]:53756 (6) => 10000 f00d::a69:0:0:dc06:80
[f00d::a69:0:0:981b]:58730 (6) => 10000 f00d::a69:0:0:f236:80
[f00d::a69:0:0:981b]:58714 (6) => 10000 f00d::a69:0:0:f236:80

joestringer
joestringer reviewed Jan 12, 2018
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Looks good to me, just one comment for the output of long lines - would be good to see the use of tabwriter. Thanks!

cilium/cmd/bpf_proxy_list.go Outdated
return
}

title := fmt.Sprintf("%-20s %-s", "Proxy", "Destination")
Copy link
Member

Choose a reason for hiding this comment

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

Some of the other CLI tools use tabwriter, and I think it's a good way to ensure that the columns being outputted line up. For example, with IPv6 it's easy for addresses to exceed 20 characters.

Would you mind updating this to do the same? Here's an example:

cilium/cilium/cmd/bpf_policy_list.go

Line 82 in b2f9223

w := tabwriter.NewWriter(os.Stdout, 5, 0, 3, ' ', 0)

cilium/cilium/cmd/bpf_policy_list.go

Line 115 in b2f9223

fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\t\n", labelsIDTitle, portTitle, actionTitle, bytesTitle, packetsTitle)

cilium/cilium/cmd/bpf_policy_list.go

Line 134 in b2f9223

fmt.Fprintf(w, "%s\t%s\t%s\t%d\t%d\t\n", lbl, port, act.String(), stat.Bytes, stat.Packets)

@ianvernon
Copy link
Member

test-me-please

@ciliumbot
Copy link

Can one of the admins verify this patch?

@ianvernon
Copy link
Member

test-me-please

@vadorovsky
Copy link
Member Author

@joestringer done

@aalemayhu aalemayhu added the area/cli Impacts the command line interface of any command in the repository. label Jan 15, 2018
@aalemayhu aalemayhu self-assigned this Jan 15, 2018
@aalemayhu aalemayhu added the kind/enhancement This would improve or streamline existing functionality. label Jan 15, 2018
@aalemayhu
Copy link
Contributor

test-me-please

@ciliumbot
Copy link

Can one of the admins verify this patch?

@vadorovsky vadorovsky changed the title [WIP] cli: Add "cilium bpf proxy list" command cli: Add "cilium bpf proxy list" command Jan 16, 2018
@vadorovsky
Copy link
Member Author

@scanf I've just added an example in the commit message

aalemayhu
aalemayhu approved these changes Jan 16, 2018
View reviewed changes
Copy link
Contributor

@aalemayhu aalemayhu left a comment

Choose a reason for hiding this comment

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

One minor nitpick, can be fixed while rebasing. Rest LGTM, thanks.

cilium/cmd/bpf_proxy_list.go Outdated
"github.com/cilium/cilium/common"
"github.com/cilium/cilium/pkg/bpf"
"github.com/cilium/cilium/pkg/proxy"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

The order is supposed to be std, cilium and then 3rd party. So import cobra should be moved further down.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@aalemayhu aalemayhu removed their assignment Jan 16, 2018
@aalemayhu
Copy link
Contributor

test-me-please

@vadorovsky
cli: Add "cilium bpf proxy list" command
cc77a7b 
The command which dumps proxy BPF maps for debugging purposes.

Example (with Cilium, Envoy and Kubernetes):

$ kubectl create -f examples/kubernetes-grpc/cc-door-app.yaml
deployment "cc-door-mgr" created
service "cc-door-server" created
pod "terminal-87" created
$ kubectl create -f examples/kubernetes-grpc/cc-door-ingress-security.yaml
$ sudo cilium bpf proxy list
PROXY                             DESTINATION
10.0.118.183:39808 (6) => 10000   10.0.202.84:50051
10.0.118.183:39794 (6) => 10000   10.0.202.84:50051
10.0.118.183:39822 (6) => 10000   10.0.202.84:50051

Fixes cilium#2277

Signed-off-by: Michal Rostecki <mrostecki@suse.com>
manalibhutiyani
manalibhutiyani approved these changes Jan 16, 2018
View reviewed changes
Copy link

@manalibhutiyani manalibhutiyani left a comment

Choose a reason for hiding this comment

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

looks great! Thanks for doing this!

@joestringer
Copy link
Member

test-me-please

@manalibhutiyani manalibhutiyani merged commit e37c945 into cilium:master Jan 16, 2018
@vadorovsky vadorovsky deleted the cli-bpf-proxy-list branch January 17, 2018 13:20
@tgraf tgraf added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aalemayhu aalemayhu aalemayhu approved these changes

@joestringer joestringer joestringer approved these changes

@ianvernon ianvernon ianvernon approved these changes

@manalibhutiyani manalibhutiyani manalibhutiyani approved these changes

Assignees
No one assigned
Labels
area/cli Impacts the command line interface of any command in the repository. kind/enhancement This would improve or streamline existing functionality. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@vadorovsky @ciliumbot @aalemayhu @ianvernon @joestringer @manalibhutiyani @tgraf