Implementation of theia and policy recommendation CLI #25

dreamtalen · 2022-05-20T22:02:17Z

In this commit, we add the implementation of theia, the command-line
tool for Theia based on cobra framework.
Policy recommendation CLI is also included. It consists of 3 commands
which let users could run a new policy recommendation job, check the
current status of a previous job, and retrieve the recommendation result of a
previous job respectively.

salv-orlando

Initial review

Makefile

salv-orlando · 2022-05-23T13:12:57Z

docs/network-policy-recommendation.md

+we can continue to deploy other necessary components of policy recommendation feature:
+
+We need to install the [Kubernetes Operator for Apache Spark](https://github.com/GoogleCloudPlatform/spark-on-k8s-operator)
+in the cluster.


@yanjunz97 - Can you assess how difficult would it to add this to helm charts as well?
If not possible, we might want to document that the Theia helm chart does not yet suppport Spark Operator deployment

It should not be difficult to add it. I would take a look at it.

@salv-orlando This support is added in PR #31

Then we should just document that way (assuming it is simpler). Again, consider create a separate PR just for documentation.

docs/network-policy-recommendation.md

salv-orlando · 2022-05-23T13:20:32Z

pkg/theiactl/commands/root.go

+	Use:   "theiactl",
+	Short: "theiactl is the command line tool for Theia",
+	Long: `theiactl is the command line tool for Theia that supports the
+policy recommendation feature`,


For the time being I'd keep "short" and "long" description the same or change it to

theiactl is the command line tool for Theia which provides access to some network flow visibility capabilities

I don't think we want to expand the long description for every feature we add.

Thanks, changed.

pkg/theiactl/commands/root.go

pkg/theiactl/commands/start.go

salv-orlando · 2022-05-23T13:30:42Z

pkg/theiactl/commands/start.go

+'YYYY-MM-DD hh:mm:ss' format, for example: 2006-01-02 15:04:05`, err)
+			}
+			recoJobArgs = append(recoJobArgs, "--end_time", endTime)
+		}


Can we add a simple check to make sure that if both start and end time are specified start_time < end_time ?

Sure, added

salv-orlando · 2022-05-23T13:31:24Z

pkg/theiactl/commands/start.go

+			err := json.Unmarshal([]byte(nsAllowList), &parsedNsAllowList)
+			if err != nil {
+				return fmt.Errorf(`parsing ns_allow_list: %v, ns_allow_list should 
+be a list of namespace string, for example: '["kube-system","flow-aggregator","flow-visibility"]'`, err)


Would it be too difficult for the CLI to parse a list of comma-separated strings? The JSON notation is not super user-friendly

Understood. Since in CLI we don't actually use the parsed list of namespaces. Here is just a pre-check for the format of the input is valid and pass the input string to the PySpark directly.

pkg/theiactl/commands/start.go

heanlan

A few nits while looking through the doc

docs/network-policy-recommendation.md

yanjunz97 · 2022-05-26T01:41:15Z

I added a commit to implement the two CLI commands to

check the current status of a previous job
get the recommendation result of a previous job

Currently kubectl port-forward is used for port forwarding. I'm still working on the unit tests for these two commands. Will update the commit once they are finished.

Updated: Unit tests for the two commands are added.

salv-orlando

More comments about check and result commands

pkg/theiactl/commands/check.go

salv-orlando · 2022-05-27T12:35:01Z

pkg/theiactl/commands/check.go

 	},
 }

+func getPolicyRecommendationProgress(id string) (string, error) {


is the 'id' passed to this function the same as the SparkAppId that would be retrieved from the monitoring service?
If so, do we actually need to pass it?

id is different from SparkAppId. But it's true that we do not need this parameter in this function any more. Removed it now, thanks!

pkg/theiactl/commands/check.go

pkg/theiactl/commands/result.go

pkg/theiactl/commands/utils.go

pkg/theiactl/commands/result.go

docs/network-policy-recommendation.md

annakhm · 2022-05-27T17:48:40Z

docs/network-policy-recommendation.md

+## Command-line Usage
+
+Currently, the network policy recommendation feature only supports command line interaction through Theiactl.
+Theiactl is the Theia command-line tool. To get more information about Theiactl, please refer to this [doc](./theiactl.md). We have 3 Theiactl commands for the policy recommendation feature:


Following antctl docs convention, I think we should use lowercase for theiactl unless beginning of sentence?
https://github.com/antrea-io/antrea/blob/main/docs/antctl.md

Thanks, addressed

docs/network-policy-recommendation.md

pkg/theiactl/commands/utils.go

yanjunz97 · 2022-05-27T21:38:52Z

I notice that theiactl recopolicy start starts the recommendation job but does not clean it, which means the the policy-reco driver pod remained in Completed status when the job finished. Is that possible to delete the pod and related svc automatically somewhere?

salv-orlando · 2022-05-30T16:04:25Z

/theia-test-e2e

jianjuns · 2022-06-03T20:30:25Z

pkg/theia/commands/policy_recommendation_retrieve.go

+
+// policyRecommendationRetrieveCmd represents the policy-recommendation retrieve command
+var policyRecommendationRetrieveCmd = &cobra.Command{
+	Use:   "retrieve",


I personally prefer "results" over "retrieve". But no strong opinion.

Hi Jianjun, I find sonobuoy have retrieve command to get results and another results command to inspect results. I prefer we could keep retrieve here.

Sure. Let us if others have an opinion.

I do not have a strong opinion, both terms are good for me.

jianjuns · 2022-06-03T20:30:47Z

pkg/theia/commands/policy_recommendation_retrieve.go

+	Use:   "retrieve",
+	Short: "Get the recommendation result of a policy recommendation Spark job",
+	Long: `Get the recommendation result of a policy recommendation Spark job by ID.
+It will return the recommended network policies described in yaml.`,


NetworkPolicies

Thanks, addressed.

jianjuns · 2022-06-03T20:32:08Z

pkg/theia/commands/policy_recommendation_run.go

+	Use:   "run",
+	Short: "Run a new policy recommendation Spark job",
+	Long: `Run a new policy recommendation Spark job.
+Network policies will be recommended based on the flow records sent by Flow Aggregator.


NetworkPolicies

I feel we can remove this sentence (need not to talk about implementation in CLI help message).

Thanks, removed this sentence.

jianjuns · 2022-06-03T20:33:38Z

pkg/theia/commands/policy_recommendation_run.go

+	Long: `Run a new policy recommendation Spark job.
+Network policies will be recommended based on the flow records sent by Flow Aggregator.
+Must finish the deployment of Theia first, please follow the steps in 
+https://github.com/antrea-io/theia/blob/main/docs/network-policy-recommendation.md`,


We can remove "please follow the steps..." too. Needs not to be in CLI help messages.

Rigth. Also because the link on main branch might become invalid or the content might change

Thanks, removed this sentence.

pkg/theia/commands/policy_recommendation_run.go

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

pkg/theia/commands/policy_recommendation_run.go

+		"use-cluster-ip",
+		false,
+		`Enable this option will use ClusterIP instead of port forwarding when connecting to the ClickHouse Service.
+It can only be used when running in cluster. (Only works when wait is enabled)`,


What does "(Only works when wait is enabled)" mean?

Should we check "wait" is enabled and say: "It can only be used when running in cluster and when wait is enabled"?

Thanks addressed.

jianjuns · 2022-06-03T22:08:11Z

pkg/theia/commands/policy_recommendation_run.go

+		"file",
+		"f",
+		"",
+		"The file path where you want to save the results. (Only works when wait is enabled)",


You used "result" in other places.

Thanks, change to use result.

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

pkg/theia/commands/policy_recommendation_run.go

+	policyRecommendationRunCmd.Flags().String(
+		"clickhouse-endpoint",
+		"",
+		"The ClickHouse Service endpoint. (Only works when wait is enabled)",


Thanks addressed.

jianjuns · 2022-06-03T22:09:48Z

pkg/theia/commands/policy_recommendation_status.go

+$ theia policy-recommendation status --id e998433e-accb-4888-9fc8-06563f073e86
+Or
+$ theia policy-recommendation status e998433e-accb-4888-9fc8-06563f073e86
+Use Cluster IP when checking the current status of job with ID e998433e-accb-4888-9fc8-06563f073e86


Thanks addressed.

salv-orlando

A few more minor comments

salv-orlando · 2022-06-03T22:14:41Z

pkg/theia/commands/policy_recommendation_run.go

+	Long: `Run a new policy recommendation Spark job.
+Network policies will be recommended based on the flow records sent by Flow Aggregator.
+Must finish the deployment of Theia first, please follow the steps in 
+https://github.com/antrea-io/theia/blob/main/docs/network-policy-recommendation.md`,


Rigth. Also because the link on main branch might become invalid or the content might change

salv-orlando · 2022-06-03T22:21:23Z

pkg/theia/commands/policy_recommendation_run.go

+		}
+		matchResult, err := regexp.MatchString(k8sQuantitiesReg, driverCoreRequest)
+		if err != nil || !matchResult {
+			return fmt.Errorf("driver-core-request should conform to the Kubernetes convention")


pedant comment: Kubernetes resource quantity convention

Thanks addressed.

pkg/theia/commands/policy_recommendation_run.go

Makefile

yanjunz97 · 2022-06-03T23:34:34Z

pkg/theia/commands/utils.go

+		LabelSelector: "app.kubernetes.io/instance=policy-reco,app.kubernetes.io/name=spark-operator",
+	})
+	if err != nil {
+		return fmt.Errorf("error %v when finding the policy-reco-spark-operator Pod, please check the deployment of the Spark Operator", err)
+	}
+	if len(pods.Items) < 1 {
+		return fmt.Errorf("can't find the policy-reco-spark-operator Pod, please check the deployment of the Spark Operator")


Also replace policy-reco- by policy-recommendation?

Sure, I will leave the Labels as it is for now before #31 is merged.

pkg/theia/commands/policy_recommendation_run.go

salv-orlando

I don't have any additional comment. PR looks good to me.

ziyouw

For UT, we can consider to cover more cases later.

ziyouw · 2022-06-06T03:22:20Z

pkg/theia/commands/policy_recommendation_retrieve_test.go

+	username, password, err := getClickHouseSecret(fakeClientset)
+	assert.NoError(t, err)
+	assert.Equal(t, "clickhouse_operator", string(username))
+	assert.Equal(t, "clickhouse_operator_password", string(password))
+}
+
+func TestGetResultFromClickHouse(t *testing.T) {
+	recoID := "db2134ea-7169-46f8-b56d-d643d4751d1d"
+	expectedResult := "recommend-allow-acnp-kube-system-rpeal"
+	db, mock, err := sqlmock.New(sqlmock.QueryMatcherOption(sqlmock.QueryMatcherEqual))
+	assert.NoError(t, err)
+	defer db.Close()
+	resultRow := sqlmock.NewRows([]string{"yamls"}).AddRow(expectedResult)
+	mock.ExpectQuery("SELECT yamls FROM recommendations WHERE id = (?);").WithArgs(recoID).WillReturnRows(resultRow)
+	result, err := getResultFromClickHouse(db, recoID)
+	assert.NoError(t, err)
+	assert.Equal(t, expectedResult, result)


For UT, only covering happy path may be not enough, we may also consider some negative cases later.

Added some failure cases in the second commit, we could add more cases after this PR merged.

In this commit, we add the implementation of theia, the command-line tool for Theia based on cobra framework. Policy recommendation CLI is also included. It consists of 3 commands which let users could run a new policy recommendation job, check the current status of a previous job, and retrieve the recommendation result of a previous job respectively. In order to access theia services externally, theia can make use of k8s port forwarder functionality. Port forwarder provides tooling to start/stop port forwarding channel for eather Pod+Port or Service+Port. The code is loosely based on corresponding code in kubectl. Document of theia and policy recommendation will be added in a seperate commit later. Signed-off-by: Yongming Ding <dyongming@vmware.com> Co-authored-by: Yanjun Zhou <zhouya@vmware.com> Co-authored-by: Anna Khmelnitsky <akhmelnitsky@vmware.com>

heanlan · 2022-06-06T21:00:45Z

pkg/theia/commands/policy_recommendation_run.go

+const (
+	flowVisibilityNS        = "flow-visibility"
+	k8sQuantitiesReg        = "^([+-]?[0-9.]+)([eEinumkKMGTP]*[-+]?[0-9]*)$"
+	sparkImage              = "antrea/theia-policy-recommendation:latest"


Is it necessary to change to "projects.registry.vmware.com/antrea/theia-policy-recommendation:latest" before getting merged?

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

dreamtalen marked this pull request as draft May 20, 2022 22:45

dreamtalen changed the title ~~Implementation of Theia CLI framework and policyreco start command~~ Implementation of Theiactl and policy recommendation commands May 20, 2022

dreamtalen changed the title ~~Implementation of Theiactl and policy recommendation commands~~ Implementation of Theiactl and policy recommendation CLI May 20, 2022

dreamtalen force-pushed the cli branch from 73826e2 to 3fdbccd Compare May 20, 2022 23:08

dreamtalen marked this pull request as ready for review May 20, 2022 23:21

dreamtalen force-pushed the cli branch 8 times, most recently from e9dc141 to c05ef81 Compare May 22, 2022 22:51

salv-orlando reviewed May 23, 2022

View reviewed changes

heanlan reviewed May 25, 2022

View reviewed changes

salv-orlando reviewed May 27, 2022

View reviewed changes

annakhm reviewed May 27, 2022

View reviewed changes

docs/network-policy-recommendation.md Outdated Show resolved Hide resolved

annakhm reviewed May 27, 2022

View reviewed changes

docs/network-policy-recommendation.md Outdated Show resolved Hide resolved

annakhm reviewed May 27, 2022

View reviewed changes

docs/network-policy-recommendation.md Outdated Show resolved Hide resolved

annakhm reviewed May 27, 2022

View reviewed changes

docs/network-policy-recommendation.md Outdated Show resolved Hide resolved

annakhm reviewed May 27, 2022

View reviewed changes

pkg/theiactl/commands/utils.go Outdated Show resolved Hide resolved

annakhm reviewed May 27, 2022

View reviewed changes

pkg/theiactl/commands/utils.go Outdated Show resolved Hide resolved

yanjunz97 force-pushed the cli branch 3 times, most recently from c58e8b9 to daa942a Compare May 27, 2022 20:53

yanjunz97 force-pushed the cli branch from daa942a to 19ad710 Compare May 28, 2022 00:03

yanjunz97 mentioned this pull request Jun 3, 2022

Add Spark Operator into Theia Helm chart #31

Merged

dreamtalen force-pushed the cli branch from 50b4988 to a905b4f Compare June 3, 2022 04:07

dreamtalen changed the title ~~Implementation of Theiactl and policy recommendation CLI~~ Implementation of theia and policy recommendation CLI Jun 3, 2022

dreamtalen force-pushed the cli branch 3 times, most recently from 819f75c to 6a9fc60 Compare June 3, 2022 17:55

yanjunz97 mentioned this pull request Jun 3, 2022

Add e2e test for policy recommendation #27

Merged

annakhm approved these changes Jun 3, 2022

View reviewed changes

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

dreamtalen force-pushed the cli branch 3 times, most recently from 54f6ade to 9a8b26e Compare June 3, 2022 21:43

jianjuns reviewed Jun 3, 2022

View reviewed changes

salv-orlando reviewed Jun 3, 2022

View reviewed changes

dreamtalen force-pushed the cli branch from 6a2f279 to dd3c2ba Compare June 3, 2022 23:28

yanjunz97 reviewed Jun 3, 2022

View reviewed changes

dreamtalen force-pushed the cli branch from dd3c2ba to 0f04a83 Compare June 4, 2022 00:15

yanjunz97 reviewed Jun 4, 2022

View reviewed changes

pkg/theia/commands/policy_recommendation_run.go Outdated Show resolved Hide resolved

dreamtalen force-pushed the cli branch from 0f04a83 to ac9f9ef Compare June 5, 2022 04:58

salv-orlando approved these changes Jun 5, 2022

View reviewed changes

ziyouw approved these changes Jun 6, 2022

View reviewed changes

dreamtalen force-pushed the cli branch 2 times, most recently from 6806eab to 96b630b Compare June 6, 2022 17:12

dreamtalen force-pushed the cli branch from 96b630b to 9437083 Compare June 6, 2022 20:38

heanlan reviewed Jun 6, 2022

View reviewed changes

Refactor UT and add more testcases

a4cc579

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

dreamtalen force-pushed the cli branch from 0bc4622 to a4cc579 Compare June 7, 2022 00:24

dreamtalen merged commit 4391011 into antrea-io:main Jun 7, 2022

dreamtalen mentioned this pull request Jun 15, 2022

Update CHANGELOG and VERSION for v0.1.0 release #48

Merged

Implementation of theia and policy recommendation CLI #25

Implementation of theia and policy recommendation CLI #25

Conversation

dreamtalen commented May 20, 2022 • edited Loading

salv-orlando 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

heanlan left a comment

Choose a reason for hiding this comment

yanjunz97 commented May 26, 2022 • edited Loading

salv-orlando 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

yanjunz97 commented May 27, 2022

salv-orlando commented May 30, 2022

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

salv-orlando 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

salv-orlando left a comment

Choose a reason for hiding this comment

ziyouw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heanlan Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

dreamtalen commented May 20, 2022 •

edited

Loading

yanjunz97 commented May 26, 2022 •

edited

Loading

heanlan Jun 6, 2022 •

edited

Loading