Remove usage of ES ConfigMap and PublicCertSecret #2954

Josh-Tigera · 2023-10-27T17:56:34Z

These resources are no longer used now that Linseed has taken over this functionality

This has a companion commit in Calico Enterprise here

Description

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.

and adding UTs along the way to protect its usage

pkg/controller/installation/core_controller.go

caseydavenport · 2023-10-30T21:06:18Z

pkg/controller/logstorage/elastic/elastic_controller.go

@@ -227,10 +225,6 @@ func Add(mgr manager.Manager, opts options.AddOptions) error {
 		}
 	}

-	if err = utils.AddConfigMapWatch(c, relasticsearch.ClusterConfigConfigMapName, common.OperatorNamespace(), &handler.EnqueueRequestForObject{}); err != nil {


I see you removed the Watch here, but the code below still uses the configmap.

Josh: Apparently I can modify other people's comments?

Ah you're right, this is the controller that writes it but I removed the watch since I didn't see any reads. I suppose we want to rectify any changes made to it as quickly as possible. I'll add some tests for these cases.

Josh: Apparently I can modify other people's comments?

😆

pkg/render/fluentd.go

caseydavenport · 2023-11-07T23:03:51Z

pkg/render/fluentd.go

+		VolumeMounts:    c.eksLogForwarderVolumeMounts(),
+	}
+
+	initContainer.Env = append(initContainer.Env, envVars...)


I don't understand this - this is appending envVars onto envVars, is it not?

caseydavenport · 2023-11-07T23:05:27Z

pkg/render/intrusion_detection.go

-		c.cfg.ESClusterConfig.Replicas(), c.cfg.ESClusterConfig.Shards())
+	container := c.intrusionDetectionControllerContainer()
+
+	envVars := []corev1.EnvVar{


I don't think there's any need to keep these appended separately from the other env vars defined in intrustionDetectionControllerContainer()

Agreed and this was a change I already had in my next set of changes to be pushed while I was fixing some issues I found once I loaded my images into a cluster.

caseydavenport · 2023-11-07T23:13:22Z

pkg/render/logstorage/esmetrics/elasticsearch_metrics.go

+		},
+	}
+
+	container.Env = append(container.Env, envVars...)


I think throughout we should clean up this split env declaration - it's a bit confusing the way this is laid out here.

Define envVars

Create a container with a separate set of hard-coded env vars

Append the previously created env vars to the container's env.

We should just be able to define all of the env vars in one place.

Agreed that's what I have in my current iteration.

pkg/render/logstorage/esmetrics/elasticsearch_metrics.go

caseydavenport · 2023-11-07T23:16:37Z

pkg/render/manager.go

+			relasticsearch.ElasticIndexSuffixEnvVar(c.cfg.ClusterConfig.ClusterName()),
+		}
+
+		env = append(env, esEnvVars...)


We don't need to define the intermediate variable

env = append(env, []corev1.EnvVar{ relasticsearch.ElasticCAEnvVar(c.SupportedOSType()), . . . })

pkg/render/manager_test.go

caseydavenport · 2023-11-21T21:37:47Z

pkg/controller/intrusiondetection/intrusiondetection_controller.go

+	if isManagedCluster {
+		if esClusterConfig.ClusterName() == render.DefaultElasticsearchClusterName {
+			err = fmt.Errorf("Elasticsearch cluster name must be non-default value in managed clusters")
+			r.status.SetDegraded(operatorv1.InvalidConfigurationError, "", err, reqLogger)


This is an interesting new assertion - any background on this one? It seems safe to me, but curious what prompted you to add it!

I do think we need a user-facing message in the SetDegraded call instead of "" though.

Also, this isn't the Elasticsearch cluster name. Rather this is the just the name of this managed cluster (which is primarily used by our elastic log ingestion pipeline).

This came from when I initially completely removed this configmap entirely and then we talked about how it's still needed so that we can differentiate managed vs management entries in elasticsearch indices eg. *-cluster for management vs *-<managedcluster.name> for managed clusters. I added a corresponding test in intrusion_detection_controller.go for this specific case as well. I can update the msg to contain user-facing information as well as correct the wording about what the field is actually used for.

caseydavenport · 2023-11-21T21:46:52Z

pkg/controller/intrusiondetection/intrusiondetection_controller.go

@@ -422,16 +425,6 @@ func (r *ReconcileIntrusionDetection) Reconcile(ctx context.Context, request rec
 		return reconcile.Result{}, err
 	}

-	esgwCertificate, err := certificateManager.GetCertificate(r.client, relasticsearch.PublicCertSecret, common.OperatorNamespace())


From what I can tell, intrusion-detection-controller is one of the few managed cluster components that still talks directly to elasticsearch via es-gateway.

It does talk to Linseed in most codepaths, but when searching for security events it seems it still talks to ES.

pkg/controller/logcollector/logcollector_controller.go

caseydavenport · 2023-11-21T21:51:29Z

pkg/controller/manager/manager_controller.go

-			return reconcile.Result{}, nil
+	var clusterConfig *relasticsearch.ClusterConfig
+	// We only require Elastic cluster configuration when Kibana is enabled. We infer whether Kibana is enabled by checking
+	// FIPS configuration mode and multi-tenancy mode. See manager.go function kibanaEnabled for more details.


We should probably make kibnanEnabled public and use the same implementation everywhere that we need to check if Kibana is enabled, so that they don't get out of sync.

caseydavenport · 2023-11-21T21:53:14Z

pkg/controller/manager/manager_controller_test.go

@@ -566,6 +560,12 @@ var _ = Describe("Manager controller tests", func() {
 		})

 		Context("image reconciliation", func() {
+			BeforeEach(func() {
+				Expect(c.Create(


This can be a single line - no need to split it up.

Alternatively, we could keep this in the main BeforeEach and then any tests that need something different can just call Update in their BeforeEach to modify the installation.

caseydavenport · 2023-11-21T21:58:40Z

pkg/render/fluentd.go

+		relasticsearch.ElasticUserEnvVar(ElasticsearchEksLogForwarderUserSecret),
+		relasticsearch.ElasticPasswordEnvVar(ElasticsearchEksLogForwarderUserSecret),
+		relasticsearch.ElasticSchemeEnvVar(esScheme),
+		relasticsearch.ElasticHostEnvVar(esHost),


One to keep an eye on - once @vara2504 is done with her work for EKS log forwarder, we can remove most (all?) of these.

Vara sent me her PRs and these have in fact been completely removed from fluentd-docker and replaced with their Linseed counterparts.

pkg/render/fluentd.go

caseydavenport · 2023-11-21T22:03:43Z

pkg/render/fluentd_test.go

@@ -884,6 +895,15 @@ var _ = Describe("Tigera Secure Fluentd rendering tests", func() {
 			}))

 		Expect(envs).To(ContainElement(corev1.EnvVar{Name: "EKS_CLOUDWATCH_LOG_FETCH_INTERVAL", Value: "900"}))
+		Expect(envs).To(ContainElements([]corev1.EnvVar{


Probably want the reverse check as well to catch any cases where there are unexpected env vars. And maybe a length check to catch bugs like the one above where we added env vars twice.

e.g.,

expected := []corev1.EnvVar{...} Expect(envs).Should(ContainElements(expected)) Expect(expected).Should(ContainElements(envs)) Expect(envs).Should(HaveLength(len(expected))

Also, we can merge the assertion on the line right above this.

I've replaced all my checks from Should(ContainElements to To(Equal so that we're more precisely checking the env variables

That's probably OK, but I specifically didn't suggest that approach because I believe it's more finicky and vulnerable to changes of variable ordering, which we don't actually care about so much.

caseydavenport

This LGTM - obviously a bit of a scary change with potential for disruption, so let's keep an eye on e2es and make sure we address any fallout in master quickly. I suspect there will be something we missed 😁

caseydavenport · 2023-12-07T21:11:00Z

@Josh-Tigera a few conflicts to resolve, though

Remove usage of ES ConfigMap and PublicCertSecret

df5ebdc

Josh-Tigera requested a review from a team as a code owner October 27, 2023 17:56

marvin-tigera added this to the v1.32.0 milestone Oct 27, 2023

marvin-tigera added docs-pr-required release-note-required labels Oct 27, 2023

Josh-Tigera added 2 commits November 6, 2023 16:22

Reinstating ConfigMap for components that actually consume it

2dd0346

and adding UTs along the way to protect its usage

Merge branch 'master' into josh.cluster-config-cleanup

98a2ae7

caseydavenport reviewed Nov 7, 2023

View reviewed changes

Josh-Tigera added 4 commits November 17, 2023 16:00

PR feedback and failure fixes

bb319c0

Merge branch 'master' into josh.cluster-config-cleanup

fc5a007

More PR feedbac and failure fixes

f4f9fea

Merge branch 'master' into josh.cluster-config-cleanup

a9237b1

Josh-Tigera added docs-not-required release-note-not-required and removed docs-pr-required release-note-required labels Nov 20, 2023

marvin-tigera added release-note-required and removed release-note-not-required labels Nov 20, 2023

Josh-Tigera added 2 commits November 21, 2023 10:33

Test failure fixes

2f4d490

Fixups

27d1474

caseydavenport reviewed Nov 21, 2023

View reviewed changes

pkg/controller/logcollector/logcollector_controller.go Show resolved Hide resolved

caseydavenport reviewed Nov 21, 2023

View reviewed changes

pkg/render/fluentd.go Outdated Show resolved Hide resolved

caseydavenport reviewed Nov 21, 2023

View reviewed changes

pkg/render/fluentd.go Outdated Show resolved Hide resolved

caseydavenport reviewed Nov 21, 2023

View reviewed changes

Merge branch 'master' into josh.cluster-config-cleanup

0c47825

PR feedback

d12b444

Josh-Tigera added release-note-not-required enterprise Feature applies to enterprise only and removed release-note-required labels Dec 1, 2023

danudey modified the milestones: v1.32.0, v1.32.1, v1.32.2 Dec 2, 2023

caseydavenport approved these changes Dec 7, 2023

View reviewed changes

Josh-Tigera added 3 commits December 8, 2023 15:56

Merge branch 'master' into josh.cluster-config-cleanup

fd3cb18

Use KibanaEnabled function instead of raw logic

b632ee1

Fix test failures

9ca133f

Josh-Tigera merged commit 2558525 into tigera:master Dec 11, 2023
3 checks passed

Josh-Tigera deleted the josh.cluster-config-cleanup branch December 11, 2023 15:43

danudey modified the milestones: v1.32.2, v1.32.3 Dec 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove usage of ES ConfigMap and PublicCertSecret #2954

Remove usage of ES ConfigMap and PublicCertSecret #2954

Josh-Tigera commented Oct 27, 2023 •

edited

Loading

caseydavenport Oct 30, 2023 •

edited by Josh-Tigera

Loading

Josh-Tigera Nov 8, 2023

caseydavenport Dec 1, 2023

caseydavenport Nov 7, 2023

caseydavenport Nov 7, 2023

Josh-Tigera Nov 8, 2023

caseydavenport Nov 7, 2023

Josh-Tigera Nov 8, 2023

caseydavenport Nov 7, 2023

caseydavenport Nov 21, 2023 •

edited

Loading

Josh-Tigera Nov 22, 2023

caseydavenport Nov 21, 2023

caseydavenport Nov 21, 2023

caseydavenport Nov 21, 2023

caseydavenport Nov 21, 2023

Josh-Tigera Dec 1, 2023

caseydavenport Nov 21, 2023

Josh-Tigera Dec 1, 2023

caseydavenport Dec 1, 2023

caseydavenport left a comment

caseydavenport commented Dec 7, 2023

Remove usage of ES ConfigMap and PublicCertSecret #2954

Remove usage of ES ConfigMap and PublicCertSecret #2954

Conversation

Josh-Tigera commented Oct 27, 2023 • edited Loading

Description

For PR author

For PR reviewers

caseydavenport Oct 30, 2023 • edited by Josh-Tigera 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caseydavenport Nov 21, 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

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

caseydavenport left a comment

Choose a reason for hiding this comment

caseydavenport commented Dec 7, 2023

Josh-Tigera commented Oct 27, 2023 •

edited

Loading

caseydavenport Oct 30, 2023 •

edited by Josh-Tigera

Loading

caseydavenport Nov 21, 2023 •

edited

Loading