Add dynamic layers to tigera infra layer in SG EV-3506 #2639

vara2504 · 2023-05-15T21:14:44Z

Description

Issue: https://tigera.atlassian.net/browse/EV-3506

Changes:

Service graph Layer should display any all the namespace created by the product.

Added a changes to dynamically populate namespace that applicable for cluster type
Standalone cluster/Management cluster:

Additional layers : tigera-manager, tigera-dex, tigera-amazon-cloud-integration based on CR.

Managed cluster:

Additional layers : tigera-guardian, tigera-amazon-cloud-integration based on CR.

Testing:

Management cluster: with dex enabled

Managed cluster:

For PR author

Tests for change.
If changing pkg/apis/, run make gen-files
If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

Milestone set according to targeted release.
Appropriate labels:
- kind/bug if this is a bugfix.
- kind/enhancement if this is a a new feature.
- enterprise if this PR applies to Calico Enterprise only.

pkg/controller/manager/manager_controller.go

caseydavenport · 2023-05-17T17:57:17Z

pkg/controller/clusterconnection/clusterconnection_controller.go

@@ -331,16 +331,25 @@ func (r *ReconcileConnection) Reconcile(ctx context.Context, request reconcile.R
 		}
 	}

+	optionaUILayerNamespaces := []string{render.GuardianNamespace}


I think this needs a comment explaining why these are optional and what a UI layer is at all.

Added a comment and updated the variable name as sgLayerTigeraNameSpaces

caseydavenport · 2023-05-17T17:58:57Z

pkg/render/manager.go

@@ -942,25 +944,27 @@ func managerUserSpecificSettingsGroup() *v3.UISettingsGroup {
 // all of the tigera namespaces.
 //
 // Calico Enterprise only
-func managerClusterWideTigeraLayer() *v3.UISettings {
+func managerClusterWideTigeraLayer(namespace []string) *v3.UISettings {


nit: "namespace" is singular, but it's a slice of multiple elements.

This should be called something like additionalNamespaces or something like that.

Alternatively, remove namespaces from this function altogether and callers just need to pass the full list of namespaces they need.

Done. Passing the full list from the calling controllers.

caseydavenport · 2023-05-17T17:59:13Z

pkg/render/manager.go

 		"tigera-operator",
 		"tigera-packetcapture",
 		"tigera-policy-recommendation",
 		"tigera-prometheus",
 		"tigera-system",
 		"calico-system",
 	}
+
+	for _, ns := range namespace {


What's the behavior if there are duplicates in this slice?

Replaced the slice with map[string]bool to avoid duplicates

rene-dekker · 2023-05-18T22:28:08Z

pkg/controller/clusterconnection/clusterconnection_controller.go

@@ -331,16 +331,29 @@ func (r *ReconcileConnection) Reconcile(ctx context.Context, request reconcile.R
 		}
 	}

+	// Populate a list of namespaces to be displayed in the service graph Tigera infrastructure layer.
+	sgLayerTigeraNameSpaces := render.DefaultSGLayerTigeraNamespaces()
+	if !sgLayerTigeraNameSpaces[render.GuardianNamespace] {


I think there is no need for the if statement and we can simply set it to true or am I missing something?

I think this applies to all of these if-statements in the PR.

rene-dekker · 2023-05-18T22:28:32Z

pkg/controller/clusterconnection/clusterconnection_controller.go

+	}
+	amz, err := utils.GetAmazonCloudIntegration(ctx, r.Client)
+	if err != nil {
+		log.Error(err, "Failed to  fetch GetAmazonCloudIntegration info")


nit: 2 spaces "to fetch"

rene-dekker · 2023-05-18T22:30:25Z

pkg/controller/clusterconnection/clusterconnection_controller.go

+		sgLayerTigeraNameSpaces[render.GuardianNamespace] = true
+	}
+	amz, err := utils.GetAmazonCloudIntegration(ctx, r.Client)
+	if err != nil {


If there is no AmazonCloudIntegration in the cluster, then err will not be nil. I think you want to filter out the NotFound error. If NotFound then we do nothing. If it is another error, we need to SetDegraded()

If there is no error, then AmazonCloudIntegration is in the cluster and we can set sgLayerTigeraNameSpaces[render.AmazonCloudIntegrationNamespace] = true. No need for an if-statement to see if it is already in the map.

pkg/render/manager.go

vara2504 · 2023-05-18T23:10:48Z

@rene-dekker Addressed the review comments.

pkg/controller/clusterconnection/clusterconnection_controller.go

pkg/controller/manager/manager_controller.go

rene-dekker · 2023-05-19T20:27:52Z

pkg/controller/manager/manager_controller.go

@@ -359,6 +359,10 @@ func (r *ReconcileManager) Reconcile(ctx context.Context, request reconcile.Requ
 		trustedSecretNames = append(trustedSecretNames, render.ComplianceServerCertSecret)
 	}

+	// Populate a list of namespaces to be displayed in the service graph Tigera infrastructure layer.
+	sgLayerTigeraNameSpaces := render.DefaultSGLayerTigeraNamespaces()
+	sgLayerTigeraNameSpaces[render.ManagerNamespace] = true


I just realize that compliance is an optional namespace as well. The CR is already fetched in this controller.

rene-dekker

lgtm

…506 (#2649) * Add dynamic layers to tigera infra layer in SG * Return reconcile on error for getamazoncloud integration config

…a#2639)" This reverts commit c35cce6.

…yer in SG EV-3506 (tigera#2649)" This reverts commit e139e32.

…a#2639)" This reverts commit c35cce6.

Revert "Cherry-pick #2639 Add dynamic layers to tigera infra layer in SG EV-3506"

marvin-tigera added this to the v1.30.0 milestone May 15, 2023

marvin-tigera added docs-pr-required release-note-required labels May 15, 2023

rene-dekker reviewed May 15, 2023

View reviewed changes

pkg/controller/manager/manager_controller.go Outdated Show resolved Hide resolved

rene-dekker reviewed May 15, 2023

View reviewed changes

pkg/controller/manager/manager_controller.go Outdated Show resolved Hide resolved

vara2504 marked this pull request as ready for review May 17, 2023 17:22

vara2504 requested a review from a team as a code owner May 17, 2023 17:22

caseydavenport reviewed May 17, 2023

View reviewed changes

vara2504 force-pushed the sg_317 branch from e4f1d8d to d4fbfd7 Compare May 18, 2023 00:47

vara2504 changed the title ~~Add dynamic layers to tigera infra layer in SG~~ Add dynamic layers to tigera infra layer in SG EV-3506 May 18, 2023

vara2504 mentioned this pull request May 18, 2023

Cherry-pick #2639 Add dynamic layers to tigera infra layer in SG EV-3506 #2649

Merged

5 tasks

vara2504 force-pushed the sg_317 branch from d4fbfd7 to d0c911f Compare May 18, 2023 20:15

rene-dekker reviewed May 18, 2023

View reviewed changes

pkg/render/manager.go Show resolved Hide resolved

Add dynamic layers to tigera infra layer in SG

cb9302d

vara2504 force-pushed the sg_317 branch from b5a8ce5 to cb9302d Compare May 18, 2023 23:06

rene-dekker reviewed May 19, 2023

View reviewed changes

pkg/controller/clusterconnection/clusterconnection_controller.go Show resolved Hide resolved

rene-dekker reviewed May 19, 2023

View reviewed changes

pkg/controller/manager/manager_controller.go Show resolved Hide resolved

Return reconcile on error for getamazoncloud integration config

e784642

rene-dekker reviewed May 19, 2023

View reviewed changes

rene-dekker approved these changes May 23, 2023

View reviewed changes

rene-dekker merged commit c35cce6 into tigera:release-v1.30 May 23, 2023

rene-dekker pushed a commit that referenced this pull request May 23, 2023

Cherry-pick #2639 Add dynamic layers to tigera infra layer in SG EV-3…

e139e32

…506 (#2649) * Add dynamic layers to tigera infra layer in SG * Return reconcile on error for getamazoncloud integration config

vara2504 added a commit to vara2504/operator that referenced this pull request May 25, 2023

Revert "Add dynamic layers to tigera infra layer in SG EV-3506 (tiger…

df73c1a

…a#2639)" This reverts commit c35cce6.

vara2504 mentioned this pull request May 25, 2023

Revert "Add dynamic layers to tigera infra layer in SG EV-3506" #2663

Merged

vara2504 added a commit to vara2504/operator that referenced this pull request May 25, 2023

Revert "Cherry-pick tigera#2639 Add dynamic layers to tigera infra la…

403380f

…yer in SG EV-3506 (tigera#2649)" This reverts commit e139e32.

vara2504 added a commit to vara2504/operator that referenced this pull request May 26, 2023

Revert "Cherry-pick tigera#2639 Add dynamic layers to tigera infra la…

40dce4d

…yer in SG EV-3506 (tigera#2649)" This reverts commit e139e32.

vara2504 added a commit to vara2504/operator that referenced this pull request May 26, 2023

Revert "Add dynamic layers to tigera infra layer in SG EV-3506 (tiger…

bfc0686

…a#2639)" This reverts commit c35cce6.

marvin-tigera added a commit that referenced this pull request May 26, 2023

Merge pull request #2664 from vara2504/revert-2649-SGLayer_mst

9395fdd

Revert "Cherry-pick #2639 Add dynamic layers to tigera infra layer in SG EV-3506"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dynamic layers to tigera infra layer in SG EV-3506 #2639

Add dynamic layers to tigera infra layer in SG EV-3506 #2639

vara2504 commented May 15, 2023 •

edited

Loading

caseydavenport May 17, 2023

vara2504 May 18, 2023

caseydavenport May 17, 2023

vara2504 May 18, 2023

caseydavenport May 17, 2023

vara2504 May 18, 2023 •

edited

Loading

rene-dekker May 18, 2023

rene-dekker May 18, 2023

vara2504 May 18, 2023

rene-dekker May 18, 2023

vara2504 May 18, 2023

rene-dekker May 18, 2023 •

edited

Loading

vara2504 May 18, 2023

vara2504 commented May 18, 2023

rene-dekker May 19, 2023

rene-dekker left a comment

Add dynamic layers to tigera infra layer in SG EV-3506 #2639

Add dynamic layers to tigera infra layer in SG EV-3506 #2639

Conversation

vara2504 commented May 15, 2023 • edited Loading

Description

For PR author

For PR reviewers

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

vara2504 May 18, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rene-dekker May 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vara2504 commented May 18, 2023

Choose a reason for hiding this comment

rene-dekker left a comment

Choose a reason for hiding this comment

vara2504 commented May 15, 2023 •

edited

Loading

vara2504 May 18, 2023 •

edited

Loading

rene-dekker May 18, 2023 •

edited

Loading