Fix panic in ReconcilerContext with duplicate key reporting (#594)

This fixes a panic reported as part of the debugging in #576 (comment). Symptom: Panic when the operator tries to report a duplicate key in the dashboards config as an event. Cause: Creating an event requires both an EventRecorder and the cluster instance. Both were not set when initializing the ReconcilerContext so both fields were nil which leads to the panic on accessing the field.
opensearch-project · Aug 30, 2023 · bb7c866 · bb7c866
2 parents c26cdd1 + e65fece
commit bb7c866
Show file tree

Hide file tree

Showing 7 changed files with 24 additions and 23 deletions.
diff --git a/go.work.sum b/go.work.sum
@@ -148,6 +148,7 @@ github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsK
 github.com/rogpeppe/go-internal v1.6.1/go.mod h1:xXDCJY+GAPziupqXw64V24skbSoqbTEfhy4qGm1nDQc=
 github.com/rogpeppe/go-internal v1.9.0/go.mod h1:WtVeX8xhTBvf0smdhujwtBcq4Qrzq/fJaraNFVN+nFs=
 github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncjaFoBhdsK/akog=
+github.com/stretchr/objx v0.5.0 h1:1zr/of2m5FGMsad5YfcqgdqdWrIhu+EBEJRhR1U7z/c=
 github.com/stretchr/testify v1.8.3/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
 github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY=
 go.uber.org/goleak v1.2.1/go.mod h1:qlT2yGI9QafXHhZZLxlSuNsMw3FFLxBr+tBRlmO1xH4=

diff --git a/opensearch-operator/controllers/opensearchController.go b/opensearch-operator/controllers/opensearchController.go
@@ -163,7 +163,7 @@ func (r *OpenSearchClusterReconciler) SetupWithManager(mgr ctrl.Manager) error {
 func (r *OpenSearchClusterReconciler) deleteExternalResources(ctx context.Context) (ctrl.Result, error) {
 	r.Logger.Info("Deleting resources")
 	// Run through all sub controllers to delete existing objects
-	reconcilerContext := reconcilers.NewReconcilerContext(r.Instance.Spec.NodePools)
+	reconcilerContext := reconcilers.NewReconcilerContext(r.Recorder, r.Instance, r.Instance.Spec.NodePools)
 
 	tls := reconcilers.NewTLSReconciler(
 		r.Client,
@@ -249,7 +249,7 @@ func (r *OpenSearchClusterReconciler) reconcilePhaseRunning(ctx context.Context)
 	}
 
 	// Run through all sub controllers to create or update all needed objects
-	reconcilerContext := reconcilers.NewReconcilerContext(r.Instance.Spec.NodePools)
+	reconcilerContext := reconcilers.NewReconcilerContext(r.Recorder, r.Instance, r.Instance.Spec.NodePools)
 
 	tls := reconcilers.NewTLSReconciler(
 		r.Client,

diff --git a/opensearch-operator/pkg/reconcilers/configuration_test.go b/opensearch-operator/pkg/reconcilers/configuration_test.go
@@ -46,7 +46,7 @@ var _ = Describe("Configuration Controller", func() {
 				},
 			}
 
-			reconcilerContext := NewReconcilerContext(spec.Spec.NodePools)
+			reconcilerContext := NewReconcilerContext(&helpers.MockEventRecorder{}, &spec, spec.Spec.NodePools)
 
 			underTest := NewConfigurationReconciler(
 				k8sClient,
@@ -88,7 +88,7 @@ var _ = Describe("Configuration Controller", func() {
 				},
 			}
 
-			reconcilerContext := NewReconcilerContext(spec.Spec.NodePools)
+			reconcilerContext := NewReconcilerContext(&helpers.MockEventRecorder{}, &spec, spec.Spec.NodePools)
 
 			underTest := NewConfigurationReconciler(
 				k8sClient,

diff --git a/opensearch-operator/pkg/reconcilers/dashboards_test.go b/opensearch-operator/pkg/reconcilers/dashboards_test.go
@@ -23,7 +23,7 @@ import (
 // http://onsi.github.io/ginkgo/ to learn more about Ginkgo..
 
 func newDashboardsReconciler(spec *opsterv1.OpenSearchCluster) (ReconcilerContext, *DashboardsReconciler) {
-	reconcilerContext := NewReconcilerContext(spec.Spec.NodePools)
+	reconcilerContext := NewReconcilerContext(&helpers.MockEventRecorder{}, spec, spec.Spec.NodePools)
 	underTest := NewDashboardsReconciler(
 		k8sClient,
 		context.Background(),
@@ -140,7 +140,7 @@ var _ = Describe("Dashboards Reconciler", func() {
 			err := k8sClient.Create(context.Background(), &ns)
 			Expect(err).ToNot(HaveOccurred())
 
-			reconcilerContext := NewReconcilerContext(spec.Spec.NodePools)
+			reconcilerContext := NewReconcilerContext(&helpers.MockEventRecorder{}, &spec, spec.Spec.NodePools)
 			underTest := NewDashboardsReconciler(
 				k8sClient,
 				context.Background(),
@@ -208,7 +208,7 @@ var _ = Describe("Dashboards Reconciler", func() {
 			err := k8sClient.Create(context.Background(), &ns)
 			Expect(err).ToNot(HaveOccurred())
 
-			reconcilerContext := NewReconcilerContext(spec.Spec.NodePools)
+			reconcilerContext := NewReconcilerContext(&helpers.MockEventRecorder{}, &spec, spec.Spec.NodePools)
 			underTest := NewDashboardsReconciler(
 				k8sClient,
 				context.Background(),
@@ -267,7 +267,7 @@ var _ = Describe("Dashboards Reconciler", func() {
 			err := k8sClient.Create(context.Background(), &ns)
 			Expect(err).ToNot(HaveOccurred())
 
-			reconcilerContext := NewReconcilerContext(spec.Spec.NodePools)
+			reconcilerContext := NewReconcilerContext(&helpers.MockEventRecorder{}, &spec, spec.Spec.NodePools)
 			underTest := NewDashboardsReconciler(
 				k8sClient,
 				context.Background(),
@@ -323,7 +323,7 @@ var _ = Describe("Dashboards Reconciler", func() {
 			err := k8sClient.Create(context.Background(), &ns)
 			Expect(err).ToNot(HaveOccurred())
 
-			reconcilerContext := NewReconcilerContext(spec.Spec.NodePools)
+			reconcilerContext := NewReconcilerContext(&helpers.MockEventRecorder{}, &spec, spec.Spec.NodePools)
 			underTest := NewDashboardsReconciler(
 				k8sClient,
 				context.Background(),
@@ -419,7 +419,7 @@ var _ = Describe("Dashboards Reconciler", func() {
 			})
 		})
 		It("mount the volumes in the deployment", func() {
-			reconcilerContext := NewReconcilerContext(spec.Spec.NodePools)
+			reconcilerContext := NewReconcilerContext(&helpers.MockEventRecorder{}, spec, spec.Spec.NodePools)
 			underTest := NewDashboardsReconciler(
 				k8sClient,
 				context.Background(),

diff --git a/opensearch-operator/pkg/reconcilers/reconcilers.go b/opensearch-operator/pkg/reconcilers/reconcilers.go
@@ -66,7 +66,7 @@ type NodePoolHash struct {
 	ConfigHash string
 }
 
-func NewReconcilerContext(nodepools []opsterv1.NodePool) ReconcilerContext {
+func NewReconcilerContext(recorder record.EventRecorder, instance *opsterv1.OpenSearchCluster, nodepools []opsterv1.NodePool) ReconcilerContext {
 	var nodePoolHashes []NodePoolHash
 	for _, nodepool := range nodepools {
 		nodePoolHashes = append(nodePoolHashes, NodePoolHash{
@@ -77,26 +77,26 @@ func NewReconcilerContext(nodepools []opsterv1.NodePool) ReconcilerContext {
 		NodePoolHashes:   nodePoolHashes,
 		OpenSearchConfig: make(map[string]string),
 		DashboardsConfig: make(map[string]string),
+		recorder:         recorder,
+		instance:         instance,
 	}
 }
 
 func (c *ReconcilerContext) AddConfig(key string, value string) {
 	_, exists := c.OpenSearchConfig[key]
 	if exists {
 		fmt.Printf("Warning: Config key '%s' already exists. Will be overwritten\n", key)
-		//	c.recorder.Event(c.instance, "Warning", "Config", fmt.Sprintf("Notice - Config key '%s' already exists. Will be overwrittenn\", key)", key))
+		c.recorder.Eventf(c.instance, "Warning", "ConfigDuplicateKey", "Config key '%s' already exists in opensearch config. Will be overwritten", key)
 	}
-	//	c.recorder.Event(c.instance, "Normal", "Config", fmt.Sprintf("Strating to add '%s' config", key))
 	c.OpenSearchConfig[key] = value
 }
 
 func (c *ReconcilerContext) AddDashboardsConfig(key string, value string) {
 	_, exists := c.DashboardsConfig[key]
 	if exists {
-		fmt.Printf("Warning: Config key '%s' already exists. Will be overwritten\n", key)
-		c.recorder.AnnotatedEventf(c.instance, map[string]string{"cluster-name": c.instance.GetName()}, "Warning", "Config", "Notice - Config key '%s' already exists in dashboard config. Will be overwritten\", key)", key)
+		fmt.Printf("Warning: Dashboards Config key '%s' already exists. Will be overwritten\n", key)
+		c.recorder.Eventf(c.instance, "Warning", "ConfigDuplicateKey", "Config key '%s' already exists in dashboards config. Will be overwritten", key)
 	}
-	//c.recorder.Event(c.instance, "Normal", "Config", fmt.Sprintf("Strating to add '%s' dashboard config", key))
 	c.DashboardsConfig[key] = value
 
 }

diff --git a/opensearch-operator/pkg/reconcilers/securityconfig_test.go b/opensearch-operator/pkg/reconcilers/securityconfig_test.go
@@ -30,7 +30,7 @@ var _ = Describe("Securityconfig Reconciler", func() {
 				ObjectMeta: metav1.ObjectMeta{Name: clusterName, Namespace: clusterName, UID: "dummyuid"},
 				Spec:       opsterv1.ClusterSpec{General: opsterv1.GeneralConfig{}}}
 
-			reconcilerContext := NewReconcilerContext(spec.Spec.NodePools)
+			reconcilerContext := NewReconcilerContext(&helpers.MockEventRecorder{}, &spec, spec.Spec.NodePools)
 			underTest := NewSecurityconfigReconciler(
 				k8sClient,
 				context.Background(),
@@ -59,7 +59,7 @@ var _ = Describe("Securityconfig Reconciler", func() {
 					},
 				}}
 
-			reconcilerContext := NewReconcilerContext(spec.Spec.NodePools)
+			reconcilerContext := NewReconcilerContext(&helpers.MockEventRecorder{}, &spec, spec.Spec.NodePools)
 			underTest := NewSecurityconfigReconciler(
 				k8sClient,
 				context.Background(),
@@ -115,7 +115,7 @@ var _ = Describe("Securityconfig Reconciler", func() {
 				},
 			}
 
-			reconcilerContext := NewReconcilerContext(spec.Spec.NodePools)
+			reconcilerContext := NewReconcilerContext(&helpers.MockEventRecorder{}, &spec, spec.Spec.NodePools)
 			underTest := NewSecurityconfigReconciler(
 				k8sClient,
 				context.Background(),
@@ -183,7 +183,7 @@ var _ = Describe("Securityconfig Reconciler", func() {
 					},
 				}}
 
-			reconcilerContext := NewReconcilerContext(spec.Spec.NodePools)
+			reconcilerContext := NewReconcilerContext(&helpers.MockEventRecorder{}, &spec, spec.Spec.NodePools)
 			underTest := NewSecurityconfigReconciler(
 				k8sClient,
 				context.Background(),
@@ -226,7 +226,7 @@ var _ = Describe("Securityconfig Reconciler", func() {
 					},
 				}}
 
-			reconcilerContext := NewReconcilerContext(spec.Spec.NodePools)
+			reconcilerContext := NewReconcilerContext(&helpers.MockEventRecorder{}, &spec, spec.Spec.NodePools)
 			underTest := NewSecurityconfigReconciler(
 				k8sClient,
 				context.Background(),
@@ -263,7 +263,7 @@ var _ = Describe("Securityconfig Reconciler", func() {
 					},
 				}}
 
-			reconcilerContext := NewReconcilerContext(spec.Spec.NodePools)
+			reconcilerContext := NewReconcilerContext(&helpers.MockEventRecorder{}, &spec, spec.Spec.NodePools)
 			underTest := NewSecurityconfigReconciler(
 				k8sClient,
 				context.Background(),

diff --git a/opensearch-operator/pkg/reconcilers/tls_test.go b/opensearch-operator/pkg/reconcilers/tls_test.go
@@ -16,7 +16,7 @@ import (
 )
 
 func newTLSReconciler(spec *opsterv1.OpenSearchCluster) (*ReconcilerContext, *TLSReconciler) {
-	reconcilerContext := NewReconcilerContext(spec.Spec.NodePools)
+	reconcilerContext := NewReconcilerContext(&helpers.MockEventRecorder{}, spec, spec.Spec.NodePools)
 	underTest := NewTLSReconciler(
 		k8sClient,
 		context.Background(),