fix(cfgmap): simplifies create/update (again!) (opendatahub-io#972)

This is a follow-up PR to opendatahub-io#948. One thing which I missed was updating labels if config map already existed. This is now ensured for both Create and Update paths and captured in tests. In addition `CreateOrUpdateConfigMap` takes a pointer to a ConfigMap "prototype" instance, where you can define `ObjectMeta` and `Data`. This simplifies function signature and allows you to simply pass anonymous struct when you do not want use return value later. Previously this had to be ignored by using `_` as variable name. Additional changes: - `ConfigMap.ObjectMeta.[Name|Namespace]` is required as it will be used for initial Get call. It returns error otherwise. Previously both `name` and `namespace` string params were not validated before invokig k8s client. - `.Data` keys will be merged if configmap already exists. This is similar to previous behaviour, except it handles `nil` maps too.
maistra · Apr 18, 2024 · 6711ab3 · 6711ab3
1 parent 1608b0a
commit 6711ab3
Show file tree

Hide file tree

Showing 4 changed files with 100 additions and 65 deletions.
diff --git a/pkg/cluster/cluster_operations_int_test.go b/pkg/cluster/cluster_operations_int_test.go
@@ -6,6 +6,7 @@ import (
 
  v1 "k8s.io/api/core/v1"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+ ctrlruntime "sigs.k8s.io/controller-runtime/pkg/client"
 
  "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster"
  "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels"
@@ -78,26 +79,32 @@ var _ = Describe("Creating cluster resources", func() {
 
  })
 
- Context("config map creation", func() {
+ Context("config map manipulation", func() {
 
  var objectCleaner *envtestutil.Cleaner
 
  BeforeEach(func() {
  objectCleaner = envtestutil.CreateCleaner(envTestClient, envTest.Config, timeout, interval)
  })
 
+ configMapMeta := metav1.ObjectMeta{
+ Name: "config-regs",
+ Namespace: "default",
+ }
+
  It("should create configmap with labels and owner reference", func() {
  // given
- data := map[string]string{
- "test-key": "test-value",
+ configMap := &v1.ConfigMap{
+ ObjectMeta: configMapMeta,
+ Data: map[string]string{
+ "test-key": "test-value",
+ },
  }
 
  // when
- configMap, err := cluster.CreateOrUpdateConfigMap(
+ err := cluster.CreateOrUpdateConfigMap(
  envTestClient,
- "config-regs",
- "default",
- data,
+ configMap,
  cluster.WithLabels(labels.K8SCommon.PartOf, "opendatahub"),
  cluster.WithOwnerReference(metav1.OwnerReference{
  APIVersion: "v1",
@@ -110,43 +117,52 @@ var _ = Describe("Creating cluster resources", func() {
  defer objectCleaner.DeleteAll(configMap)
 
  // then
- Expect(configMap.Labels).To(HaveKeyWithValue(labels.K8SCommon.PartOf, "opendatahub"))
+ actualConfigMap := &v1.ConfigMap{}
+ Expect(envTestClient.Get(context.Background(), ctrlruntime.ObjectKeyFromObject(configMap), actualConfigMap)).To(Succeed())
+ Expect(actualConfigMap.Labels).To(HaveKeyWithValue(labels.K8SCommon.PartOf, "opendatahub"))
  getOwnerRefName := func(reference metav1.OwnerReference) string {
  return reference.Name
  }
- Expect(configMap.OwnerReferences[0]).To(WithTransform(getOwnerRefName, Equal("default")))
+ Expect(actualConfigMap.OwnerReferences[0]).To(WithTransform(getOwnerRefName, Equal("default")))
  })
 
  It("should be able to update existing config map", func() {
  // given
- data := map[string]string{
- "test-key": "test-value",
- }
-
- // when
- configMap, err := cluster.CreateOrUpdateConfigMap(
+ createErr := cluster.CreateOrUpdateConfigMap(
  envTestClient,
- "config-regs",
- "default",
- data,
+ &v1.ConfigMap{
+ ObjectMeta: configMapMeta,
+ Data: map[string]string{
+ "test-key": "test-value",
+ },
+ },
+ cluster.WithLabels("test-step", "create-configmap"),
  )
- Expect(err).ToNot(HaveOccurred())
- updatedConfigMap, err := cluster.CreateOrUpdateConfigMap(
-  envTestClient,
-  "config-regs",
- "default",
- map[string]string{
+ Expect(createErr).ToNot(HaveOccurred())
+
+ // when
+ updatedConfigMap := &v1.ConfigMap{
+ ObjectMeta: configMapMeta,
+ Data: map[string]string{
  "test-key": "new-value",
  "new-key": "sth-new",
  },
+ }
+
+ updateErr := cluster.CreateOrUpdateConfigMap(
+ envTestClient,
+ updatedConfigMap,
+ cluster.WithLabels("test-step", "update-existing-configmap"),
  )
- Expect(err).ToNot(HaveOccurred())
- defer objectCleaner.DeleteAll(configMap)
+ Expect(updateErr).ToNot(HaveOccurred())
+ defer objectCleaner.DeleteAll(updatedConfigMap)
 
  // then
- Expect(updatedConfigMap.Data).To(HaveKeyWithValue("test-key", "new-value"))
- Expect(updatedConfigMap.Data).To(HaveKeyWithValue("new-key", "sth-new"))
-
+ actualConfigMap := &v1.ConfigMap{}
+ Expect(envTestClient.Get(context.Background(), ctrlruntime.ObjectKeyFromObject(updatedConfigMap), actualConfigMap)).To(Succeed())
+ Expect(actualConfigMap.Data).To(HaveKeyWithValue("test-key", "new-value"))
+ Expect(actualConfigMap.Data).To(HaveKeyWithValue("new-key", "sth-new"))
+ Expect(actualConfigMap.Labels).To(HaveKeyWithValue("test-step", "update-existing-configmap"))
  })
  })
 

diff --git a/pkg/cluster/meta.go b/pkg/cluster/meta.go
@@ -4,6 +4,8 @@ import (
  "fmt"
 
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+ "k8s.io/apimachinery/pkg/runtime"
+ "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 // MetaOptions allows to add additional settings for the object being created through a chain
@@ -27,6 +29,12 @@ func WithOwnerReference(ownerReferences ...metav1.OwnerReference) MetaOptions {
  }
 }
 
+func OwnedBy(owner metav1.Object, scheme *runtime.Scheme) MetaOptions {
+ return func(obj metav1.Object) error {
+ return controllerutil.SetOwnerReference(owner, obj, scheme)
+ }
+}
+
 func WithLabels(labels ...string) MetaOptions {
  return func(obj metav1.Object) error {
  labelsMap, err := extractKeyValues(labels)

diff --git a/pkg/cluster/resources.go b/pkg/cluster/resources.go
@@ -79,39 +79,43 @@ func CreateSecret(cli client.Client, name, namespace string, metaOptions ...Meta
  return nil
 }
 
-func CreateOrUpdateConfigMap(c client.Client, name string, namespace string, data map[string]string, metaOptions ...MetaOptions) (*corev1.ConfigMap, error) {
- configMap := &corev1.ConfigMap{
- ObjectMeta: metav1.ObjectMeta{
- Name: name,
- Namespace: namespace,
- },
- Data: data,
+func CreateOrUpdateConfigMap(c client.Client, desiredCfgMap *corev1.ConfigMap, metaOptions ...MetaOptions) error {
+ if desiredCfgMap.GetName() == "" || desiredCfgMap.GetNamespace() == "" {
+ return fmt.Errorf("configmap name and namespace must be set")
  }
 
- if err := ApplyMetaOptions(configMap, metaOptions...); err != nil {
- return nil, err
+ existingCfgMap := &corev1.ConfigMap{}
+ err := c.Get(context.TODO(), client.ObjectKey{
+ Name: desiredCfgMap.Name,
+ Namespace: desiredCfgMap.Namespace,
+ }, existingCfgMap)
+
+ if apierrs.IsNotFound(err) {
+ if applyErr := ApplyMetaOptions(desiredCfgMap, metaOptions...); applyErr != nil {
+ return applyErr
+ }
+ return c.Create(context.TODO(), desiredCfgMap)
+ } else if err != nil {
+ return err
  }
 
- getErr := c.Get(context.TODO(), client.ObjectKey{
- Name: name,
- Namespace: namespace,
- }, configMap)
+ if applyErr := ApplyMetaOptions(existingCfgMap, metaOptions...); applyErr != nil {
+ return applyErr
+ }
 
- if getErr != nil {
- if apierrs.IsNotFound(getErr) {
- if err := c.Create(context.TODO(), configMap); err != nil {
- return nil, err
- }
- } else {
- return nil, getErr
- }
+ if existingCfgMap.Data == nil {
+ existingCfgMap.Data = make(map[string]string)
+ }
+ for key, value := range desiredCfgMap.Data {
+ existingCfgMap.Data[key] = value
  }
 
- for key, value := range data {
- configMap.Data[key] = value
+ if updateErr := c.Update(context.TODO(), existingCfgMap); updateErr != nil {
+ return updateErr
  }
 
- return configMap, c.Update(context.TODO(), configMap)
+ existingCfgMap.DeepCopyInto(desiredCfgMap)
+ return nil
 }
 
 // CreateNamespace creates a namespace and apply metadata.

diff --git a/pkg/feature/servicemesh/resources.go b/pkg/feature/servicemesh/resources.go
@@ -3,6 +3,9 @@ package servicemesh
 import (
  "strings"
 
+ corev1 "k8s.io/api/core/v1"
+ metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
  "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster"
  "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature"
 )
@@ -18,15 +21,17 @@ func MeshRefs(f *feature.Feature) error {
  "MESH_NAMESPACE": meshConfig.Namespace,
  }
 
- _, err := cluster.CreateOrUpdateConfigMap(
+ return cluster.CreateOrUpdateConfigMap(
  f.Client,
- "service-mesh-refs",
- namespace,
- data,
+ &corev1.ConfigMap{
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "service-mesh-refs",
+ Namespace: namespace,
+ },
+ Data: data,
+ },
  feature.OwnedBy(f),
  )
-
- return err
 }
 
 // AuthRefs stores authorization configuration in the config map, so it can
@@ -44,13 +49,15 @@ func AuthRefs(f *feature.Feature) error {
  "AUTHORINO_LABEL": "security.opendatahub.io/authorization-group=default",
  }
 
- _, err := cluster.CreateOrUpdateConfigMap(
+ return cluster.CreateOrUpdateConfigMap(
  f.Client,
- "auth-refs",
- namespace,
- data,
+ &corev1.ConfigMap{
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "auth-refs",
+ Namespace: namespace,
+ },
+ Data: data,
+ },
  feature.OwnedBy(f),
  )
-
- return err
 }