antctl support traceflow #932

lzhecheng · 2020-07-09T07:42:37Z

Fixed #923

antctl traceflow is on remote mode, inside controller mode and inside agent mode.
It supports yaml and json output. It can return without retrieving results.

$ antctl traceflow -S busybox0 -D busybox1
name: default-busybox0-to-default-busybox1
phase: Succeeded
source: default/busybox0
destination: default/busybox1
noderesults:
- node: antrea-linux-testbed7-2
  role: ""
  timestamp: 1595391265
  observations:
  - component: SpoofGuard
    componentinfo: ""
    action: Forwarded
    pod: ""
    dstmac: ""
    networkpolicy: ""
    ttl: 0
    translatedsrcip: ""
    translateddstip: ""
    tunneldstip: ""
  - component: Forwarding
    componentinfo: Output
    action: Delivered
    pod: ""
    dstmac: ""
    networkpolicy: ""
    ttl: 0
    translatedsrcip: ""
    translateddstip: ""
    tunneldstip: ""

antrea-bot · 2020-07-09T07:42:49Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

/test-e2e: to trigger e2e tests.
/skip-e2e: to skip e2e tests.
/test-conformance: to trigger conformance tests.
/skip-conformance: to skip conformance tests.
/test-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

These commands can only be run by members of the vmware-tanzu organization.

lzhecheng · 2020-07-09T07:43:05Z

/test-e2e
/test-conformance

lzhecheng · 2020-07-09T08:34:52Z

/test-e2e

lzhecheng · 2020-07-09T09:03:33Z

/test-e2e

lzhecheng · 2020-07-14T08:48:16Z

/test-e2e

antrea-bot · 2020-07-14T10:51:39Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

/test-e2e: to trigger e2e tests.
/skip-e2e: to skip e2e tests.
/test-conformance: to trigger conformance tests.
/skip-conformance: to skip conformance tests.
/test-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

These commands can only be run by members of the vmware-tanzu organization.

lzhecheng · 2020-07-16T13:54:33Z

/test-e2e

lzhecheng · 2020-07-17T02:15:51Z

/test-e2e

antrea-bot · 2020-08-11T01:54:54Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

/test-e2e: to trigger e2e tests.
/skip-e2e: to skip e2e tests.
/test-conformance: to trigger conformance tests.
/skip-conformance: to skip conformance tests.
/test-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-hw-offload: to trigger ovs hardware offload test.
/skip-hw-offload: to skip ovs hardware offload test.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

lzhecheng · 2020-08-11T01:56:26Z

/test-all

lzhecheng · 2020-08-11T03:02:56Z

/test-all

lzhecheng · 2020-08-11T03:13:47Z

/test-e2e

lzhecheng · 2020-08-11T03:14:36Z

/test-e2e

pkg/antctl/raw/traceflow/command.go

pkg/antctl/raw/traceflow/command_test.go

pkg/apis/ops/v1alpha1/register.go

lzhecheng · 2020-08-11T06:31:27Z

/test-all

weiqiangt

Some nits, otherwise LGTM.

weiqiangt · 2020-08-11T07:41:19Z

pkg/antctl/raw/traceflow/command.go

+		var isPod bool
+		var err error
+		split = strings.Split(option.destination, "/")
+		if len(split) == 1 {


I think the following if blocks can be simplified since the only different is the namespace.
Consider following workflow:

if len == 1 { setNameSpace } else if len == 2 { setNameSapce } else { error } original codes in `if` blocks.

Also, we can just check isPod one time by using this flow.

Good idea! Updated.

weiqiangt · 2020-08-11T07:44:29Z

pkg/antctl/raw/traceflow/command.go

+)
+
+const (
+	ICMPProtocol int32 = 1


Looks like we only use these constants to construct a map, how about just decalre a map here?

Fixed antrea-io#923 antctl traceflow is on remote mode, inside controller mode and inside agent mode. It supports yaml and json output. It can return without retrieving results. e.g. ``` $ antctl traceflow -S busybox0 -D busybox1 name: default-busybox0-to-default-busybox1-fpllngzi phase: Succeeded source: default/busybox0 destination: default/busybox1 results: - node: antrea-linux-testbed7-1 timestamp: 1596435607 observations: - component: SpoofGuard action: Forwarded - component: Forwarding componentInfo: Output action: Delivered ```

lzhecheng · 2020-08-11T10:04:59Z

/test-all

weiqiangt

LGTM.

tnqn

Sorry for late review, but I hope the comments can be addressed with a separate PR.

tnqn · 2020-08-11T15:40:45Z

pkg/antctl/antctl.go

@@ -324,6 +325,11 @@ var CommandList = &commandList{
 			supportAgent:      true,
 			supportController: true,
 		},
+		{
+			cobraCommand:      traceflow.Command,
+			supportAgent:      true,


traceflow can support agent?

antctl traceflow can be run on agent.

pkg/antctl/raw/traceflow/command.go

tnqn · 2020-08-11T15:47:06Z

pkg/antctl/raw/traceflow/command.go

+	return nil
+}
+
+func setupKubeconfig(kubeconfig *rest.Config, groupVersion *schema.GroupVersion) {


Why are the following setting necessary? for example, I think the kubeconfig should have CAData set?

You are right. I followed code style in support bundle. But it is unnecessary since it is enough for traceflow to have connection to apiserver.

tnqn · 2020-08-11T15:54:13Z

pkg/antctl/raw/traceflow/command.go

+		} else {
+			return nil, fmt.Errorf("destination should be in the format of namespace/pod, pod, namespace/service or service")
+		}
+		if isPod, err = dstIsPod(client, dst.Namespace, dest); err != nil {


Pod and service can have same name, I think it's not reliable and clear to determine user's intention by querying runtime state. Maybe we should have an argument or flag to specify the type of the destination object.

Yes. But could you please help check code here: https://github.com/vmware-tanzu/antrea/blob/master/pkg/agent/apiserver/handlers/ovstracing/handler.go#L151-L155
It seems like it is doing the same: find pod first, if not found then service.

OK, then maybe we could discuss them together later, my personal feeling is it's not deterministic

tnqn · 2020-08-11T15:58:37Z

pkg/antctl/raw/traceflow/command.go

+	for _, v := range strings.Split(cleanFlow, ",") {
+		n, ok := protocols[v]
+		if ok {
+			(*pkt).IPHeader.Protocol = n


Does it really need to use (*pkt)? So do all such cases in this file.

Yes, it is not necessary.

tnqn · 2020-08-11T16:00:24Z

pkg/antctl/raw/traceflow/command.go

+		}
+	}
+
+	r, ok, err := getFieldPortValue(cleanFlow, "tcp_src")


Can getFieldPortValue just return a map to avoid repeated computation and the following redundant code?
It could be named getPortFields

tnqn · 2020-08-11T16:02:24Z

pkg/antctl/raw/traceflow/command.go

+			return fmt.Errorf("error when converting output to yaml: %w", err)
+		}
+	} else {
+		return fmt.Errorf("output types are yaml and json")


The error is not clear

Turned it into "output types should be yaml or json".

tnqn · 2020-08-11T16:04:46Z

pkg/antctl/raw/traceflow/command.go

+	for i := range b {
+		randIdx := rand.Intn(len(lettersAndDigits))
+		b[i] = lettersAndDigits[randIdx]
+	}


Consider "k8s.io/apimachinery/pkg/util/rand.String" for simplicity.

tnqn · 2020-08-11T16:08:09Z

pkg/apiserver/apiserver.go

+
+	if err := installAPIGroup(s, c); err != nil {
+		return nil, err
+	}


Why making this change?

I think 1) it looks clearer and 2) it keeps the same code style like agent/apiserver.

ok, but I think it's controversial to separate a short function into multiple ones when they are not reused, especially in a PR that is not related. We should focus on the purpose of the PR and leave unrelated style unification to separate PR to save reviewers' effort and avoid controversy.

Make sense. I will pay more attention next time. I used to change some apiserver code in this PR, but such change is dropped with a comment. However, this code remains due to the reason I mentioned above..

Fixed antrea-io#923 antctl traceflow is on remote mode, inside controller mode and inside agent mode. It supports yaml and json output. It can return without retrieving results. e.g. ``` $ antctl traceflow -S busybox0 -D busybox1 name: default-busybox0-to-default-busybox1-fpllngzi phase: Succeeded source: default/busybox0 destination: default/busybox1 results: - node: antrea-linux-testbed7-1 timestamp: 1596435607 observations: - component: SpoofGuard action: Forwarded - component: Forwarding componentInfo: Output action: Delivered ```

vmwclabot added the cla-not-required label Jul 9, 2020

lzhecheng force-pushed the tf-cli branch from be8e146 to 8a09181 Compare July 14, 2020 03:14

lzhecheng force-pushed the tf-cli branch from 8a09181 to 0eaefec Compare July 14, 2020 10:51

lzhecheng marked this pull request as draft July 15, 2020 03:40

lzhecheng force-pushed the tf-cli branch 13 times, most recently from 49a7060 to 610ef7d Compare July 16, 2020 10:37

lzhecheng force-pushed the tf-cli branch 5 times, most recently from 0453a1e to dff189c Compare July 17, 2020 09:01

lzhecheng added this to the Antrea v0.9.0 release milestone Aug 5, 2020

lzhecheng force-pushed the tf-cli branch from 54bc225 to 453ea6c Compare August 11, 2020 01:54

lzhecheng requested a review from weiqiangt August 11, 2020 01:56

lzhecheng force-pushed the tf-cli branch 4 times, most recently from 1362e92 to dd11186 Compare August 11, 2020 03:00

weiqiangt reviewed Aug 11, 2020

View reviewed changes

pkg/antctl/raw/traceflow/command.go Outdated Show resolved Hide resolved

pkg/antctl/raw/traceflow/command.go Show resolved Hide resolved

pkg/antctl/raw/traceflow/command_test.go Show resolved Hide resolved

pkg/apis/ops/v1alpha1/register.go Outdated Show resolved Hide resolved

lzhecheng force-pushed the tf-cli branch 4 times, most recently from 30ec43e to f871cdc Compare August 11, 2020 05:57

weiqiangt reviewed Aug 11, 2020

View reviewed changes

lzhecheng force-pushed the tf-cli branch from f871cdc to f94da15 Compare August 11, 2020 08:30

lzhecheng requested a review from weiqiangt August 11, 2020 11:39

weiqiangt approved these changes Aug 11, 2020

View reviewed changes

lzhecheng merged commit 8fb94c1 into antrea-io:master Aug 11, 2020

lzhecheng deleted the tf-cli branch August 11, 2020 12:03

tnqn reviewed Aug 11, 2020

View reviewed changes

lzhecheng mentioned this pull request Aug 12, 2020

[Antctl] Improve implementation of antctl traceflow #1070

Merged

antctl support traceflow #932

antctl support traceflow #932

Conversation

lzhecheng commented Jul 9, 2020 • edited Loading

antrea-bot commented Jul 9, 2020

lzhecheng commented Jul 9, 2020

lzhecheng commented Jul 9, 2020

lzhecheng commented Jul 9, 2020

lzhecheng commented Jul 14, 2020

antrea-bot commented Jul 14, 2020

lzhecheng commented Jul 16, 2020

lzhecheng commented Jul 17, 2020

antrea-bot commented Aug 11, 2020

lzhecheng commented Aug 11, 2020

lzhecheng commented Aug 11, 2020

lzhecheng commented Aug 11, 2020

lzhecheng commented Aug 11, 2020

lzhecheng commented Aug 11, 2020

weiqiangt left a comment

Choose a reason for hiding this comment

weiqiangt Aug 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzhecheng commented Aug 11, 2020

weiqiangt left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzhecheng commented Jul 9, 2020 •

edited

Loading

weiqiangt Aug 11, 2020 •

edited

Loading