Add docs for theia and policy recommendation CLI #36

dreamtalen · 2022-06-03T00:17:56Z

Signed-off-by: Yongming Ding dyongming@vmware.com

jianjuns · 2022-06-03T00:45:45Z

docs/network-policy-recommendation.md

+
+## Deployment
+
+To use the NetworkPolicy recommendation feature, please follow this [doc](https://github.com/antrea-io/antrea/blob/main/docs/network-flow-visibility.md) to deploy Flow Exporter and Flow Aggregator first.


Copy my comments here too:

We received a valid feedback the deployment steps are too complex to follow, and can be a big challenge for a normal user. Could we have an all-in-one quick-start to deploy everything, rather than refer to multiple docs? At least, we should have a quick-start to deploy Grafana Flow Collector with default settings, and then this doc can point to that.

For your reference, this is the quick-start we are adding for multi-cluster (yes it still looks complex without any automation): antrea-io/antrea#3853 (comment)

But I do not mean you should create a separate quick-start doc. I kindof suggest to add a quick-start (or just call call it installation) section to the Theia network-flow-visibility doc with all steps to quickly set up all components (Antrea, Flow Aggregator, Grafana, etc.), with default settings (the simpler the better).

Thank Jianjun, for the quick-start, I think using helm is the simplest way to achieve that. After #31 is merged, all the needed steps will be:

Deploy Antrea with Flow Exporter, and Flow Aggregator following a doc inside the Antrea repo.

Run the helm install command of Theia to deploy Clickhouse, grafana, spark operator, etc.

Is this good enough now? Or we'd better come up with a solution to deploy all components in one step?

cc @salv-orlando

@dreamtalen - as a user, ideally I would be able to deploy antrea + theia by:

setting mandatory parameters in values.yaml

do something like helm install theia && helm install antrea (or the other way around)

So, your plan looks quite good, but in the quick start doc I think it's better to provide users a way to do things very quickly only using helm (e.g.: not 1) set config values for antrea; 2) deploy antrea with helm; 3) do some config changes; 4)set config values for Theia; 5) deploy theia with helm

jianjuns · 2022-06-03T22:20:18Z

docs/theia.md

@@ -0,0 +1,68 @@
+# Theia


We should use "theia" to refer to the CLI tool.

Maybe use "theia Command-line Tool" or "Theia Command-line Tool" as the title

Thanks, changed to "Theia Command-line Tool" now.

jianjuns · 2022-06-03T22:20:58Z

docs/theia.md

@@ -0,0 +1,68 @@
+# Theia
+
+Theia is the command-line tool which provides access to Theia network flow


Use "theia" in the text.

Thanks, addressed.

jianjuns · 2022-06-03T22:21:36Z

docs/theia.md

+
+<!-- toc -->
+- [Installation](#installation)
+- [Compile](#compile)


"Compilation" to be consistent with others.

Thanks, addressed.

jianjuns · 2022-06-03T22:22:20Z

docs/theia.md

+theia help
+```
+
+For Linux, we also publish binaries for Arm-based systems.


It is a single binary? Then: "binary"

I took a look at antctl release and found there will be two binaries for arm and arm64

Imho this statement is probably misplaced here. Here we expect instructions for fetching an installing the CLI. This note about ARM support can be in the paragraph at lines 16-18

docs/network-policy-recommendation.md

jianjuns · 2022-06-03T22:28:21Z

docs/network-policy-recommendation.md

+
+We need to install the [Kubernetes Operator for Apache Spark](https://github.com/GoogleCloudPlatform/spark-on-k8s-operator)
+in the cluster.
+Our policy recommendation logic is implemented as a [Spark](https://github.com/apache/spark) application,


Do you want a new line or not? If not do not switch to a new line for each sentence.

Thanks, addressed.

jianjuns · 2022-06-03T22:28:52Z

docs/network-policy-recommendation.md

+Our policy recommendation logic is implemented as a [Spark](https://github.com/apache/spark) application,
+the Kubernetes Operator for Apache Spark helps us schedule the Spark job in the Kubernetes cluster.
+
+Please run the following command:


Thanks, addressed.

jianjuns · 2022-06-03T22:29:02Z

docs/network-policy-recommendation.md

+helm install policy-reco spark-operator/spark-operator --namespace flow-visibility --set image.tag=latest
+```
+
+This will install the Kubernetes Operator for Apache Spark into the `flow-visibility` namespace.


Thanks, addressed.

jianjuns · 2022-06-03T22:29:44Z

docs/network-policy-recommendation.md

+The goal of this feature is to aid the cluster admin or developers to secure
+their applications based on the idea of the Zero Trust model.
+
+## Deployment


Call it Installation or Preparation

Thanks, addressed.

jianjuns · 2022-06-03T22:31:40Z

docs/network-policy-recommendation.md

+
+## Deployment
+
+To use the NetworkPolicy recommendation feature, please follow this [doc](https://github.com/antrea-io/antrea/blob/main/docs/network-flow-visibility.md)


I am fine to merge the current version first, but again let us work on a single all-in-one quick-start (probably as a section of network-flow-visibility.md) to configure Exporter, deploy Flow Aggregator and Collector before we release 0.1!

salv-orlando · 2022-06-03T22:34:50Z

docs/network-policy-recommendation.md

+[Grafana Flow Collector](network-flow-visibility.md#grafana-flow-collector) to generate
+[Kubernetes NetworkPolicies](https://kubernetes.io/docs/concepts/services-networking/network-policies/)
+or [Antrea NetworkPolicies](https://github.com/antrea-io/antrea/blob/main/docs/antrea-network-policy.md).
+The goal of this feature is to aid the cluster admin or developers to secure


Consider rephrase:

This feature assists cluster administrators and app developers in securing their applications according to Zero Trust principles

Thanks, addressed.

salv-orlando · 2022-06-03T22:36:27Z

docs/network-policy-recommendation.md

+
+## Deployment
+
+To use the NetworkPolicy recommendation feature, please follow this [doc](https://github.com/antrea-io/antrea/blob/main/docs/network-flow-visibility.md)


Consider rephrase:

Theia's NetworkPolicy Recommendation capablity leverages Antrea Flow Exporter and Flow Aggreator. Before leveraging this feature, follow this doc to ensure they are deployed.

Thanks, addressed.

salv-orlando · 2022-06-03T22:38:01Z

docs/network-policy-recommendation.md

+
+To use the NetworkPolicy recommendation feature, please follow this [doc](https://github.com/antrea-io/antrea/blob/main/docs/network-flow-visibility.md)
+to deploy Flow Exporter and Flow Aggregator first.
+Please update the `#podLabels: false` to `podLabels: true` inside


Consider rephrase:

In addition, ensure podLabels: true is set in Flow Aggregator Configuration, as Theia's NetworkPolicy Recommendation (let's be consistent in the way we call things) feature needs to process pod label information.

Thanks, addressed.

salv-orlando · 2022-06-03T22:38:40Z

docs/network-policy-recommendation.md

+because policy recommendation feature needs Pod labels information to recommend network policies.
+
+Then, please follow these [deployment steps](network-flow-visibility.md#deployment-steps)
+to deploy the Grafana Flow Collector.


I would just say "deploy Theia". does that make sense?

Sounds good to me, actually user could finish deployment here with helm install after PR#31 merged.

salv-orlando · 2022-06-03T22:39:40Z

docs/network-policy-recommendation.md

+Then, please follow these [deployment steps](network-flow-visibility.md#deployment-steps)
+to deploy the Grafana Flow Collector.
+
+After we finish the deployment of Grafana Flow Collector and can see some flow records in the Grafana UI,


Is it required that we verify flows are visibile in Grafana?

Otherwise I'd just write: "Once Theia is deployed, the Kubernetes Operator for Apache Spark will need to be installed in the cluster.

Comment out these lines now since user won't need to deploy Spark Operator separately after PR#31 merged.

salv-orlando · 2022-06-03T22:40:24Z

docs/network-policy-recommendation.md

+
+We need to install the [Kubernetes Operator for Apache Spark](https://github.com/GoogleCloudPlatform/spark-on-k8s-operator)
+in the cluster.
+Our policy recommendation logic is implemented as a [Spark](https://github.com/apache/spark) application,


consider use consistent naming. In line with the previous comments this could be "Theia's NetworkPolicy Recommendation", but that's just an idea...

Thanks, changed the title and filename accordingly.

salv-orlando · 2022-06-03T22:42:19Z

docs/network-policy-recommendation.md

+- `theia pr status`
+- `theia pr retrieve`
+
+To see all options and usage examples of these commands, you may run `theia policy-recommendation [subcommand] --help`. In the following sections, we will go through a simple example to utilize these commands with default options.


Nit: Perhaps the last sentence in not needed.

Thanks, removed.

salv-orlando · 2022-06-03T22:47:55Z

docs/theia.md

@@ -0,0 +1,68 @@
+# Theia
+
+Theia is the command-line tool which provides access to Theia network flow


Honest question: should we refer to theia with lowercase initial and monospaced font?

salv-orlando · 2022-06-03T22:49:20Z

docs/theia.md

+
+## Installation
+
+Starting with Theia release v0.1, we publish the theia binaries for


This probably just a personal prefenrence, but to me seems documentation is best written in 3rd person.

E.g.:

theia binaries are published for different OS/CPU Architecture combionations. Refer to the...

Sounds good to me, changed.

salv-orlando · 2022-06-03T22:50:34Z

docs/theia.md

+## Compile
+
+If you made some changes on theia code and would like to compile
+and test locally, please run these commands inside the Theia main directory:


this paragraph seems redundant. Maybe just

In order to compile theia the following commands should be run from Theia's main directory:

Thanks, addressed.

salv-orlando · 2022-06-06T14:36:29Z

docs/networkpolicy-recommendation.md

+
+## Perform NetworkPolicy Recommendation
+
+Currently, the NetworkPolicy recommendation feature only supports command line interaction through `theia`.


Opinion: I would rephrase this as "Users can leverage Theia's NetworkPolicy recommendation feature through theia CLI."

Thanks, addressed.

salv-orlando · 2022-06-06T14:37:06Z

docs/networkpolicy-recommendation.md

+`theia` is the command-line tool which provides access to Theia network flow visibility capabilities.
+To get more information about `theia`, please refer to this [doc](theia.md).
+
+We have 3 `theia` commands for the policy recommendation feature:


Usual opinion about impersonal docs: "There are 3 theia commands for..."

salv-orlando · 2022-06-06T14:37:57Z

docs/networkpolicy-recommendation.md

+
+### Run a policy recommendation job
+
+The `theia policy-recommendation run` command can run a new policy recommendation job.


s/can run/triggers/

Thanks, addressed.

salv-orlando · 2022-06-06T14:38:50Z

docs/networkpolicy-recommendation.md

+
+The `theia policy-recommendation status` command is used to check the status of a previous policy recommendation job.
+
+For example the job we just created above, we could check the status of it by:


s/status of it/its status/

Also consider replacing "by" with "via" or "through"

Thanks, addressed.

salv-orlando · 2022-06-06T14:41:20Z

docs/networkpolicy-recommendation.md

+
+It will return the status of this policy recommendation job like `SUBMITTED`, `RUNNING`, `COMPLETED`, and `FAILED`.
+
+For a complete list of the possible status of a policy recommendation job, please refer to the definition [here](https://github.com/GoogleCloudPlatform/spark-on-k8s-operator/blob/3b58b2632545b1f20e105a6080d6597513af60da/pkg/apis/sparkoperator.k8s.io/v1beta2/types.go#L331).


"statuses" (plural)

Isn't there a documentation link you could share instead of a pointer to spark operator code?

Sure, I replaced it with an api-doc link

salv-orlando · 2022-06-06T14:44:22Z

docs/theia.md

+theia help
+```
+
+For Linux, we also publish binaries for Arm-based systems.


Imho this statement is probably misplaced here. Here we expect instructions for fetching an installing the CLI. This note about ARM support can be in the paragraph at lines 16-18

salv-orlando · 2022-06-06T14:45:02Z

docs/theia.md

+## Compilation
+
+In order to compile `theia` the following commands should be run from
+Theia's main directory:


Should Windows compilation instructions be provided as well?

I didn't include the compilation of Windows because I think make won't work on Windows. Or maybe we could delete the whole Compilation paragraph since I find antctl doc only provides instructions on installation: https://github.com/antrea-io/antrea/blob/main/docs/antctl.md#installation

I agree with removal. The section isn't really adding any useful info

salv-orlando · 2022-06-06T14:48:02Z

docs/networkpolicy-recommendation.md

+Then, please follow these [deployment steps](network-flow-visibility.md#deployment-steps)
+to deploy Theia.
+
+Commet out following lines since user won't need to deploy Spark Operator separately after PR#31 merged.


I think the Installation section might become obsolete considering spark-operator is also being adding to flow-visibility.yaml?

I would probably list "pre-requisites" here, and namely:

flow exporter must be enabled (which is a pre-requisite for all network visibility)

flow aggregator must store pod label

It is also true that we want to have a "hard way" installation story (the one below), but it should be - in my opinion - together with flow visibility installation info

Prerequisite sounds good to me, updated this doc first, and will change the flow visibility doc after we finalized the overall install story.

docs/networkpolicy-recommendation.md

ziyouw · 2022-06-07T07:23:40Z

docs/networkpolicy-recommendation.md

+### Run a policy recommendation job
+
+The `theia policy-recommendation run` command can run a new policy recommendation job.
+If the new policy recommendation job is created successfully, the `recommendation ID` of this job will be returned:


A few words about the ID? It will be needed for status/retrieve, so customers need to store it somewhere. Otherwise, it may be ignored and seems we don't have a way for customers to get it back?

That's a very valuable usability advice, @ziyouw - I've bee thinking too that we need a story to manage recommendations jobs (for instance at some point we want to delete them!).

We need additional CLI commands to list/delete recommendation jobs. I will create a new github issue to track this.

Sure, added a short description of ID here.

Signed-off-by: Yongming Ding <dyongming@vmware.com>

docs/networkpolicy-recommendation.md

jianjuns · 2022-06-08T17:40:12Z

docs/theia.md

+curl -Lo ./theia "https://github.com/antrea-io/theia/releases/download/<TAG>/theia-$(uname)-x86_64"
+chmod +x ./theia
+mv ./theia /some-dir-in-your-PATH/theia
+theia help


Maybe remove this command, as it is not about installation?

Hi Jianjun, I was following the convention in the antctl doc here. I think this help command could help users verify the installation is successful.

Ok. NP to me.

jianjuns · 2022-06-08T17:40:43Z

docs/networkpolicy-recommendation.md

+```
+
+To apply recommended policies in the cluster, we can save the recommended
+policies to a yml file and apply it through `kubectl`:


yml -> YAML

through -> using

Thanks, addressed.

jianjuns · 2022-06-08T17:43:02Z

docs/networkpolicy-recommendation.md

+
+## Prerequisite
+
+- [Flow Exporter](


Could we simply point to network-flow-visibility.md which should include a section to install everything?

Yes, we will change this link to a quick-start section in network-flow-visibility.md in the future. Once we finalized the installation story, adding the helm chart for Flow Aggregator, etc.

Could we have the change in Theia 0.1? Again, the current installation steps are too hard for users to follow.

Got it, according to the discussion inside team yesterday, we plan to add a Flow Aggregator chart in Theia first since Antrea won't ship helm charts in v1.7, and the whole installation steps will be kubectl apply -f antrea.yaml && helm install flow-aggregator && helm install theia.
For the work estimate of this change, I would like to transfer this question to @salv-orlando
cc. @yanjunz97

Even if we cannot get charts for Flow Aggregator in 1.7, let us at least clean up installation section in network-flow-visibility.md to document the basic steps to stet up everything with default settings, and then this doc can point to that.

We plan to have a PR containing the Flow Aggregator chart and quick-start section merged by the end of this week.

dreamtalen · 2022-06-08T23:09:43Z

Hi @jianjuns @salv-orlando @ziyouw, may I know if you have further comments on this PR (except the quick-start section which will be included in a separate PR)?

jianjuns

Two nits. Otherwise, LGTM

jianjuns · 2022-06-08T23:19:22Z

docs/networkpolicy-recommendation.md

+
+## Prerequisite
+
+- [Flow Exporter](


"Antrea Flow Exporter"

OK, changed the Flow Aggregator to Antrea Flow Aggregator too.

docs/networkpolicy-recommendation.md

jianjuns · 2022-06-08T23:25:23Z

docs/networkpolicy-recommendation.md

+is enabled
+- [Antrea Flow Aggregator](
+https://github.com/antrea-io/antrea/blob/main/docs/network-flow-visibility.md#flow-aggregator)
+is deployed and stores Pod labels


Probably "and configured to store Pod labels"?

Sounds good to me.

Signed-off-by: Yongming Ding <dyongming@vmware.com>

ziyouw

LGTM, thanks!

jianjuns

@dreamtalen : would you merge this one first?

dreamtalen · 2022-06-10T16:32:06Z

@dreamtalen : would you merge this one first?

Sure, thanks for the reviews!

dreamtalen mentioned this pull request Jun 3, 2022

Implementation of theia and policy recommendation CLI #25

Merged

dreamtalen force-pushed the cli-doc branch 2 times, most recently from f444a11 to f670dd9 Compare June 3, 2022 00:31

jianjuns reviewed Jun 3, 2022

View reviewed changes

dreamtalen force-pushed the cli-doc branch 2 times, most recently from 4bead5b to a5030ae Compare June 3, 2022 18:01

dreamtalen requested review from salv-orlando, annakhm, heanlan and yanjunz97 June 3, 2022 18:02

annakhm approved these changes Jun 3, 2022

View reviewed changes

salv-orlando added this to the 0.1 milestone Jun 3, 2022

jianjuns reviewed Jun 3, 2022

View reviewed changes

salv-orlando reviewed Jun 3, 2022

View reviewed changes

dreamtalen force-pushed the cli-doc branch 3 times, most recently from 7337c25 to 30202dd Compare June 4, 2022 01:00

salv-orlando reviewed Jun 6, 2022

View reviewed changes

jianjuns reviewed Jun 6, 2022

View reviewed changes

docs/networkpolicy-recommendation.md Outdated Show resolved Hide resolved

dreamtalen mentioned this pull request Jun 6, 2022

Add Spark Operator into Theia Helm chart #31

Merged

heanlan reviewed Jun 6, 2022

View reviewed changes

ziyouw reviewed Jun 7, 2022

View reviewed changes

dreamtalen force-pushed the cli-doc branch 4 times, most recently from 1f4ca82 to 5581f39 Compare June 7, 2022 18:22

Add docs for theia and policy recommendation CLI

db99b77

Signed-off-by: Yongming Ding <dyongming@vmware.com>

ziyouw reviewed Jun 8, 2022

View reviewed changes

docs/networkpolicy-recommendation.md Outdated Show resolved Hide resolved

dreamtalen force-pushed the cli-doc branch from 5581f39 to 8b9c6ed Compare June 8, 2022 17:02

jianjuns reviewed Jun 8, 2022

View reviewed changes

dreamtalen force-pushed the cli-doc branch from 8b9c6ed to b47f905 Compare June 8, 2022 17:56

jianjuns reviewed Jun 8, 2022

View reviewed changes

dreamtalen force-pushed the cli-doc branch from b47f905 to 8feeb5a Compare June 8, 2022 23:22

jianjuns reviewed Jun 8, 2022

View reviewed changes

Address comments

5e95dad

Signed-off-by: Yongming Ding <dyongming@vmware.com>

dreamtalen force-pushed the cli-doc branch from 8feeb5a to 5e95dad Compare June 8, 2022 23:27

dreamtalen requested review from salv-orlando and ziyouw June 9, 2022 16:48

ziyouw approved these changes Jun 10, 2022

View reviewed changes

jianjuns approved these changes Jun 10, 2022

View reviewed changes

dreamtalen merged commit d38144f into antrea-io:main Jun 10, 2022


		## Deployment

		To use the NetworkPolicy recommendation feature, please follow this [doc](https://github.com/antrea-io/antrea/blob/main/docs/network-flow-visibility.md) to deploy Flow Exporter and Flow Aggregator first.

		@@ -0,0 +1,68 @@
		# Theia

		Theia is the command-line tool which provides access to Theia network flow


		## Installation

		Starting with Theia release v0.1, we publish the theia binaries for


		## Perform NetworkPolicy Recommendation

		Currently, the NetworkPolicy recommendation feature only supports command line interaction through `theia`.


		### Run a policy recommendation job

		The `theia policy-recommendation run` command can run a new policy recommendation job.


		The `theia policy-recommendation status` command is used to check the status of a previous policy recommendation job.

		For example the job we just created above, we could check the status of it by:


		It will return the status of this policy recommendation job like `SUBMITTED`, `RUNNING`, `COMPLETED`, and `FAILED`.

		For a complete list of the possible status of a policy recommendation job, please refer to the definition [here](https://github.com/GoogleCloudPlatform/spark-on-k8s-operator/blob/3b58b2632545b1f20e105a6080d6597513af60da/pkg/apis/sparkoperator.k8s.io/v1beta2/types.go#L331).

Add docs for theia and policy recommendation CLI #36

Add docs for theia and policy recommendation CLI #36

Conversation

dreamtalen commented Jun 3, 2022

Choose a reason for hiding this comment

jianjuns Jun 3, 2022 • edited

Choose a reason for hiding this comment

dreamtalen Jun 3, 2022 • edited

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

dreamtalen Jun 4, 2022 • edited

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns Jun 3, 2022 •

edited

dreamtalen Jun 3, 2022 •

edited

dreamtalen Jun 4, 2022 •

edited