Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

cli: add proxy command to dump config #1883

Merged
merged 3 commits into from
Oct 21, 2020

Conversation

shashankram
Copy link
Member

@shashankram shashankram commented Oct 21, 2020

Description:
Adds a cli command to dump proxy configuration
for a given pod. This is useful for debugging
purpose.

Resolves #1762

Affected area:

  • New Functionality [ ]
  • Documentation [ ]
  • Install [ ]
  • Control Plane [ ]
  • CLI Tool [X]
  • Certificate Management [ ]
  • Networking [ ]
  • Metrics [ ]
  • SMI Policy [ ]
  • Security [ ]
  • Tests [ ]
  • CI System [ ]
  • Performance [ ]
  • Other [ ]

Please answer the following questions with yes/no.

  • Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?
    No

Adds a cli command to dump proxy configuration
for a given pod.

Resolves openservicemesh#1762
@shashankram shashankram added wip Work-in-Progress and removed wip Work-in-Progress labels Oct 21, 2020
@shashankram shashankram marked this pull request as ready for review October 21, 2020 18:23
@shashankram shashankram requested a review from a team as a code owner October 21, 2020 18:23
eduser25
eduser25 previously approved these changes Oct 21, 2020

//add mesh name flag
f := cmd.Flags()
f.StringVarP(&dumpConfigCmd.namespace, "namespace", "n", metav1.NamespaceDefault, "Namespace of pod")
Copy link
Contributor

Choose a reason for hiding this comment

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

There's already a -n/--namespace flag automatically added to every command from pkg/cli/environment.go to define the control plane namespace. Does it still work to define both the pod namespace and control plane namespace? If the control plane namespace doesn't need to be defined, then do both flags show up in the help text? That would be confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would not mix the pod namespace with the control plane namespace. This is what the help text looks like:

This command will dump the sidecar proxy configuration for the given pod.

Usage:
  osm proxy dump-config POD ... [flags]

Examples:

# Dump the proxy configuration for pod bookbuyer-5ccf77f46d-rc5mg in the bookbuyer namespace
osm proxy dump-config bookbuyer-5ccf77f46d-rc5mg -n bookbuyer


Flags:
  -h, --help                help for dump-config
  -p, --local-port uint16   Local port to use for port forwarding (default 15000)

Global Flags:
  -n, --namespace string   namespace for osm control plane (default "osm-system")

Copy link
Contributor

Choose a reason for hiding this comment

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

So then does the flag on the command work to change the pod namespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

The flag on the command is meant to specify the pod namespace, not to change it. The pod here is the application pod, not the controller pod. If you did specify a controller pod, the command will error due to the controller not being a meshed pod.

Copy link
Member Author

Choose a reason for hiding this comment

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

I filed #1884 to track the ambiguity between the namespace flags.

cmd/cli/proxy_configdump.go Outdated Show resolved Hide resolved
cmd/cli/proxy_configdump.go Show resolved Hide resolved
cmd/cli/proxy_configdump.go Outdated Show resolved Hide resolved
cmd/cli/proxy_configdump.go Outdated Show resolved Hide resolved
cmd/cli/proxy_configdump_test.go Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Oct 21, 2020

Codecov Report

Merging #1883 into main will decrease coverage by 0.43%.
The diff coverage is 3.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1883      +/-   ##
==========================================
- Coverage   58.87%   58.43%   -0.44%     
==========================================
  Files         127      129       +2     
  Lines        5223     5274      +51     
==========================================
+ Hits         3075     3082       +7     
- Misses       2145     2189      +44     
  Partials        3        3              
Impacted Files Coverage Δ
cmd/cli/osm.go 0.00% <0.00%> (ø)
cmd/cli/proxy.go 0.00% <0.00%> (ø)
cmd/cli/proxy_configdump.go 4.54% <4.54%> (ø)
pkg/envoy/route/config.go 95.00% <0.00%> (-0.84%) ⬇️
...ertificate/providers/tresor/certificate_manager.go 76.40% <0.00%> (+6.74%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b32bc61...b256ffb. Read the comment docs.

@shashankram shashankram merged commit 397f0ed into openservicemesh:main Oct 21, 2020
@shashankram shashankram deleted the cli-proxy branch October 21, 2020 21:09
draychev pushed a commit to draychev/osm that referenced this pull request Oct 28, 2020
Adds a cli command to dump proxy configuration
for a given pod.

Resolves openservicemesh#1762
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create an osm CLI command that dumps proxy config for a given pod
5 participants