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

Reject unsupported postional arguments for antctl #2011

Merged
merged 1 commit into from
Apr 7, 2021

Conversation

hty690
Copy link
Contributor

@hty690 hty690 commented Mar 30, 2021

For traceflow and proxy, antctl will fail and log error when the user
enters extra postional arguments.

Before:

# antctl traceflow -S coredns-7f89b7bc75-4xzxg -D coredns-7f89b7bc75-cczbd udp,udp_dst=53,udp_src=1000
name: default-coredns-7f89b7bc75-4xzxg-to-default-coredns-7f89b7bc75-cczbd-xzfgd55x
phase: Running
source: default/coredns-7f89b7bc75-4xzxg
destination: default/coredns-7f89b7bc75-cczbd
...

After:

# antctl traceflow -S coredns-7f89b7bc75-4xzxg -D coredns-7f89b7bc75-cczbd udp,udp_dst=53,udp_src=1000
Error: adding extra postional arguments

Fixes #1915

@@ -132,6 +132,12 @@ func init() {
Long: "Run a reverse proxy to access Antrea API (Controller or Agent). Command only supports remote mode. HTTPS connections between the proxy and the Antrea API will not be secure (no certificate verification).",
Example: proxyCommandExample,
RunE: runE,
Args: func(cmd *cobra.Command, args []string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we use the builtin args: NoArgs command validator here? See https://pkg.go.dev/github.com/spf13/cobra#readme-positional-and-custom-arguments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Didn't notice that before.

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

I know it's a pretty basic change but do you think you could add a simple unit test to pkg/antctl/antctl_test.go?

@hty690
Copy link
Contributor Author

hty690 commented Apr 1, 2021

I know it's a pretty basic change but do you think you could add a simple unit test to pkg/antctl/antctl_test.go?

I'm not sure how to write unit test but I will try it.

@hty690 hty690 force-pushed the traceflow_reject branch 2 times, most recently from 1bcf499 to d3b1511 Compare April 1, 2021 07:28
@codecov-io
Copy link

codecov-io commented Apr 1, 2021

Codecov Report

Merging #2011 (d6c92f1) into main (9b8440d) will decrease coverage by 8.24%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2011      +/-   ##
==========================================
- Coverage   65.39%   57.15%   -8.25%     
==========================================
  Files         197      262      +65     
  Lines       17217    19469    +2252     
==========================================
- Hits        11259    11127     -132     
- Misses       4785     7166    +2381     
- Partials     1173     1176       +3     
Flag Coverage Δ
e2e-tests 25.39% <ø> (?)
kind-e2e-tests 41.92% <ø> (-14.60%) ⬇️
unit-tests 41.83% <100.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/antctl/raw/traceflow/command.go 27.40% <100.00%> (+2.90%) ⬆️
pkg/apis/controlplane/helper.go 25.00% <0.00%> (-75.00%) ⬇️
pkg/apis/controlplane/v1beta2/helper.go 25.00% <0.00%> (-75.00%) ⬇️
pkg/controller/networkpolicy/store/group.go 5.00% <0.00%> (-75.00%) ⬇️
pkg/controller/networkpolicy/mutate.go 0.00% <0.00%> (-71.77%) ⬇️
pkg/controller/networkpolicy/validate.go 0.00% <0.00%> (-71.57%) ⬇️
pkg/k8s/name.go 33.33% <0.00%> (-66.67%) ⬇️
pkg/agent/controller/networkpolicy/packetin.go 18.01% <0.00%> (-52.77%) ⬇️
pkg/apiserver/handlers/webhook/mutation_crd.go 0.00% <0.00%> (-52.64%) ⬇️
pkg/apiserver/handlers/webhook/validation_crd.go 0.00% <0.00%> (-52.64%) ⬇️
... and 137 more

@hty690 hty690 force-pushed the traceflow_reject branch 2 times, most recently from dad13af to dc7dab0 Compare April 1, 2021 09:50
jianjuns
jianjuns previously approved these changes Apr 1, 2021
pkg/antctl/antctl_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test, this looks good to me, we can merge once you address Jianjun's comment

For traceflow and proxy, antctl will fail and log error when the user
enters extra postional arguments.

Fixes antrea-io#1915
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@tnqn
Copy link
Member

tnqn commented Apr 2, 2021

/test-all

@antoninbas antoninbas merged commit 9097932 into antrea-io:main Apr 7, 2021
@hty690 hty690 deleted the traceflow_reject branch April 14, 2021 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explicitly reject position arguments for antctl commands which do not support them
5 participants