Add NetworkPolicyRecommendation Controller implementations #114

yanjunz97 · 2022-09-20T22:59:40Z

This PR implements the controller workflow for NetworkPolicyRecommendation
and adds RecommendedNetworkPolicy CRD to store the results.

To run the NetworkPolicyRecommendation, please follow the steps below:

Deploy ClickHouse, Spark Operator, and Theia Manager.
Apply the following yaml.

apiVersion: crd.theia.antrea.io/v1alpha1
kind: NetworkPolicyRecommendation
metadata:
  name: pr-test
spec:
  jobType: "initial"
  policyType: "anp-deny-applied"
  executorInstances: 1
  driverCoreRequest: 200m
  driverMemory: 512M
  executorCoreRequest: 200m
  executorMemory: 512M
  excludeLabels: true
  toServices: true

You can observe the Spark Application starts, runs and stops. You could run the following command to get the details about the NetworkPolicyRecommendation anytime.

kubectl get npr pr-test -n flow-visibility -o json

The output would look like the following. When the job is finished, you will also be able to see the recommendedNetworkPolicy in the NetworkPolicyRecommendation status.

{
    "apiVersion": "crd.theia.antrea.io/v1alpha1",
    "kind": "NetworkPolicyRecommendation",
    ...
    "status": {
        "completedStages": 4,
        "sparkApplication": "97186a4d-5681-470d-82d6-e890ba539c34",
        "state": "RUNNING",
        "totalStages": 5,
    }
}

After deleting the NetworkPolicyRecommendation pr-test, the corresponding RecommendedNetworkPolicy CR and the row in ClickHouse database will be deleted at the same time.

Signed-off-by: Yanjun Zhou [email protected]

codecov-commenter · 2022-09-20T23:02:56Z

Codecov Report

Merging #114 (a746f9d) into main (8a3826a) will increase coverage by 17.74%.
The diff coverage is 73.90%.

@@             Coverage Diff             @@
##             main     #114       +/-   ##
===========================================
+ Coverage   30.65%   48.39%   +17.74%     
===========================================
  Files          11       15        +4     
  Lines        1442     2461     +1019     
  Branches        0       41       +41     
===========================================
+ Hits          442     1191      +749     
- Misses        978     1194      +216     
- Partials       22       76       +54

Flag	Coverage Δ
unit-tests	`45.65% <73.90%> (+15.00%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/controller/networkpolicyrecommendation/util.go	`67.06% <67.06%> (ø)`
...ntroller/networkpolicyrecommendation/controller.go	`75.78% <77.63%> (ø)`
...om-plugins/grafana-chord-plugin/src/ChordPanel.tsx	`71.58% <0.00%> (ø)`
...-plugins/grafana-sankey-plugin/src/SankeyPanel.tsx	`89.13% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

pkg/apis/crd/v1alpha1/types.go

pkg/controller/networkpolicyrecommendation/controller.go

wsquan171 · 2022-10-06T00:29:02Z

pkg/controller/networkpolicyrecommendation/controller.go

+	return err
+}
+
+func (c *NPRecommendationController) startSparkApplication(npReco *crdv1alpha1.NetworkPolicyRecommendation) error {


can we abstract these validation logic to some querrier-like interface so it can be shared with API server REST handler? This way we can move the error reporting at REST CREATE time. Otherwise a success response from API server could end up in invalid state due to user input error, which looks confusing. cc @yuntanghsu

As Yanjun pointed out offline, as the similar checks are also done on CLI, and no other component / user is expected to be directly calling the API server at the moment, validation at REST handler doesn't provide much benefit at the moment. probably can skip this at the moment until when UI is to be added and we can reevaluate.

Will the user directly create a NPR CR to start the SparkApplication? If no, maybe we can validate the input only on CLI side.

We do not suggest user to do this. But we do not prevent user from doing this. I think the validation here could be a protection in case this happens.

pkg/controller/networkpolicyrecommendation/controller.go

wsquan171 · 2022-10-06T23:03:36Z

pkg/controller/networkpolicyrecommendation/controller.go

 	for i := 0; i < defaultWorkers; i++ {
 		go wait.Until(c.worker, time.Second, stopCh)
 	}
 	<-stopCh
 }

+func (c *NPRecommendationController) deletionworker() {


We need some GC logic to recycle stale resources (rnp and db entry) in case npr is removed during manager downtime, or manager is restarted without finishing the cleanup job. this may be addressed in a future pr.

The idea of GC sounds great to me. As we may remove rnp in next release, maybe it will be a periodic job checking db to remove an entry with an id not in the current npr list?
Besides, I'm also thinking about whether to do the similar GC for the stale SparkApplication. But I'm a little bit worry about whether this will delete some jobs not created by the manager in the future.

one way around is we introduce additional metadata (annotation / labels) for the ones created by Theia. This way we won't bother with other unrelated SparkApplications. Periodical is good but at least this should be triggered once per mgr init.

wsquan171 · 2022-10-06T23:06:11Z

pkg/controller/networkpolicyrecommendation/util.go

+}
+
+func getPolicyRecommendationResult(client kubernetes.Interface, id string, namespace string) (recommendedNetworkPolicy *crdv1alpha1.RecommendedNetworkPolicy, err error) {
+	connect, err := setupClickHouseConnection(client, namespace)


can we cache / resue connection within the controller instead of reestablishing connection each time db check is needed?

Updated, thanks!

pkg/controller/networkpolicyrecommendation/controller.go

wsquan171

overall lgtm. will check again after rebase and approve.

wsquan171 · 2022-10-08T02:58:31Z

pkg/controller/networkpolicyrecommendation/controller.go

+		recommendedNetworkPolicy := obj.(*crdv1alpha1.RecommendedNetworkPolicy)
+		err = c.crdClient.CrdV1alpha1().RecommendedNetworkPolicies(namespace).Delete(context.TODO(), recommendedNetworkPolicy.Name, metav1.DeleteOptions{})
+		if err != nil {
+			aggregatedErrorMsg += fmt.Sprintf("failed to delete the RecommendedNetworkPolicy %s, error: %v\n", recommendedNetworkPolicy.Name, err)


typically we don't want to have multi-line error messages. could collect failed name and err in 2 lists and call fmt when returning error.

Make sense to me. Updated, thanks!

Signed-off-by: Yanjun Zhou <[email protected]>

yuntanghsu

LGTM

wsquan171

LGTM

yanjunz97 marked this pull request as draft September 20, 2022 23:04

yuntanghsu reviewed Sep 21, 2022

View reviewed changes

pkg/apis/crd/v1alpha1/types.go Show resolved Hide resolved

yanjunz97 force-pushed the manager-controller branch from 35a5be5 to 97b03d3 Compare September 21, 2022 22:10

yanjunz97 marked this pull request as ready for review September 23, 2022 00:07

yanjunz97 requested review from wsquan171, yuntanghsu and salv-orlando September 24, 2022 00:00

yanjunz97 force-pushed the manager-controller branch 2 times, most recently from 4e7e504 to 28522b4 Compare September 27, 2022 22:46

salv-orlando added this to the Theia v0.3 Release milestone Sep 28, 2022

yanjunz97 requested a review from dreamtalen September 28, 2022 16:06

yanjunz97 force-pushed the manager-controller branch from 28522b4 to c59fab8 Compare September 28, 2022 21:45

yuntanghsu reviewed Sep 28, 2022

View reviewed changes

pkg/apis/crd/v1alpha1/types.go Show resolved Hide resolved

pkg/controller/networkpolicyrecommendation/controller.go Outdated Show resolved Hide resolved

pkg/controller/networkpolicyrecommendation/controller.go Show resolved Hide resolved

yanjunz97 force-pushed the manager-controller branch from ab93cad to ac232dd Compare September 29, 2022 00:02

yuntanghsu reviewed Sep 29, 2022

View reviewed changes

pkg/controller/networkpolicyrecommendation/controller.go Outdated Show resolved Hide resolved

yuntanghsu reviewed Sep 29, 2022

View reviewed changes

yuntanghsu mentioned this pull request Oct 3, 2022

Adapt CLI to use Theia Manager #115

Merged

wsquan171 reviewed Oct 6, 2022

View reviewed changes

yanjunz97 force-pushed the manager-controller branch from d142742 to ed81eda Compare October 7, 2022 23:45

wsquan171 reviewed Oct 8, 2022

View reviewed changes

yanjunz97 force-pushed the manager-controller branch 2 times, most recently from 8ddd85e to c255d55 Compare October 10, 2022 16:21

Add NetworkPolicyRecommendation Controller implementations

c5db0f4

Signed-off-by: Yanjun Zhou <[email protected]>

yanjunz97 force-pushed the manager-controller branch from c255d55 to c5db0f4 Compare October 10, 2022 16:24

yuntanghsu approved these changes Oct 10, 2022

View reviewed changes

wsquan171 approved these changes Oct 10, 2022

View reviewed changes

yanjunz97 merged commit 995bf04 into antrea-io:main Oct 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NetworkPolicyRecommendation Controller implementations #114

Add NetworkPolicyRecommendation Controller implementations #114

yanjunz97 commented Sep 20, 2022 •

edited

Loading

codecov-commenter commented Sep 20, 2022 •

edited

Loading

wsquan171 Oct 6, 2022

wsquan171 Oct 6, 2022

yuntanghsu Oct 6, 2022 •

edited

Loading

yanjunz97 Oct 6, 2022

wsquan171 Oct 6, 2022

yanjunz97 Oct 8, 2022 •

edited

Loading

wsquan171 Oct 8, 2022

wsquan171 Oct 6, 2022

yanjunz97 Oct 7, 2022

wsquan171 left a comment

wsquan171 Oct 8, 2022

yanjunz97 Oct 10, 2022

yuntanghsu left a comment

wsquan171 left a comment

Add NetworkPolicyRecommendation Controller implementations #114

Add NetworkPolicyRecommendation Controller implementations #114

Conversation

yanjunz97 commented Sep 20, 2022 • edited Loading

codecov-commenter commented Sep 20, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuntanghsu Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanjunz97 Oct 8, 2022 • 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

wsquan171 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuntanghsu left a comment

Choose a reason for hiding this comment

wsquan171 left a comment

Choose a reason for hiding this comment

yanjunz97 commented Sep 20, 2022 •

edited

Loading

codecov-commenter commented Sep 20, 2022 •

edited

Loading

yuntanghsu Oct 6, 2022 •

edited

Loading

yanjunz97 Oct 8, 2022 •

edited

Loading