factory: adds configmap prefix to tls asset path

* Previously it was possible a case, when secretOrConfigmap has reference to the secret and configmap with the same name and the same key. Operator fetches refrenced content and puts it into single secret, called tlsAsset. Secret content has key formed from reference as - "ASSETNAMESPACE_ASSETNAME_ASSETKEY". ASSETNAME is actual name of secret or configmap. Operator may overwrite ASSETNAME if secret and configmap uses the same name. It should be really rare case, because usually names and content do not clash. * Now operator adds "configmap_" prefix to ASSETNAME. It eliminates clash possibilty and resolves issue. No action required from user side before upgrade. Related issue #1067 Signed-off-by: f41gh7 <[email protected]>
VictoriaMetrics · Aug 15, 2024 · 8ab67c8 · 8ab67c8
1 parent e6d9026
commit 8ab67c8
Show file tree

Hide file tree

Showing 9 changed files with 138 additions and 124 deletions.
diff --git a/api/operator/v1beta1/vmextra_types.go b/api/operator/v1beta1/vmextra_types.go
@@ -799,13 +799,13 @@ type TLSConfig struct {
 func (c *TLSConfig) AsArgs(args []string, prefix, pathPrefix string) []string {
 	if c.CAFile != "" {
 		args = append(args, fmt.Sprintf("-%s.tlsCAFile=%s", prefix, c.CAFile))
-	} else if c.CA.Name() != "" {
-		args = append(args, fmt.Sprintf("-%s.tlsCAFile=%s", prefix, c.BuildAssetPath(pathPrefix, c.CA.Name(), c.CA.Key())))
+	} else if c.CA.PrefixedName() != "" {
+		args = append(args, fmt.Sprintf("-%s.tlsCAFile=%s", prefix, c.BuildAssetPath(pathPrefix, c.CA.PrefixedName(), c.CA.Key())))
 	}
 	if c.CertFile != "" {
 		args = append(args, fmt.Sprintf("-%s.tlsCertFile=%s", prefix, c.CertFile))
-	} else if c.Cert.Name() != "" {
-		args = append(args, fmt.Sprintf("-%s.tlsCertFile=%s", prefix, c.BuildAssetPath(pathPrefix, c.Cert.Name(), c.Cert.Key())))
+	} else if c.Cert.PrefixedName() != "" {
+		args = append(args, fmt.Sprintf("-%s.tlsCertFile=%s", prefix, c.BuildAssetPath(pathPrefix, c.Cert.PrefixedName(), c.Cert.Key())))
 	}
 	if c.KeyFile != "" {
 		args = append(args, fmt.Sprintf("-%s.tlsKeyFile=%s", prefix, c.KeyFile))
@@ -879,24 +879,14 @@ func (c *SecretOrConfigMap) Validate() error {
 	return nil
 }
 
-// BuildSelectorWithPrefix builds prefix path
-func (c *SecretOrConfigMap) BuildSelectorWithPrefix(prefix string) string {
-	if c.Secret != nil {
-		return fmt.Sprintf("%s%s/%s", prefix, c.Secret.Name, c.Secret.Key)
-	}
-	if c.ConfigMap != nil {
-		return fmt.Sprintf("%s%s/%s", prefix, c.ConfigMap.Name, c.ConfigMap.Key)
-	}
-	return ""
-}
-
-// Name returns actual name
-func (c *SecretOrConfigMap) Name() string {
+// PrefixedName returns name with possible prefix
+// prefix added only to configmap to avoid clash with secret name
+func (c *SecretOrConfigMap) PrefixedName() string {
 	if c.Secret != nil {
 		return c.Secret.Name
 	}
 	if c.ConfigMap != nil {
-		return c.ConfigMap.Name
+		return fmt.Sprintf("configmap_%s", c.ConfigMap.Name)
 	}
 	return ""
 }

diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
@@ -23,6 +23,7 @@ aliases:
 - [vmagent](./resources/vmagent.md): adds `status` and `lastSyncError` status fields to all scrape objects - `VMServiceScrape`, `VMPodScrape`, `VMNodeScrape`,`VMPodScrape`, `VMStaticScrape` and `VMScrapeConfig`. It allows to track config generation for `vmagent` from scrape objects.
 - [operator](./README.md): refactors config builder for `VMAgent`. It fixes minor bug with incorrect skip of scrape object with incorrect references for secrets and configmaps.
 - [operator](./README.md): allows to secure `metrics-bind-address` webserver with `TLS` and `mTLS` protection via flags `tls.enable`,`tls.certDir`,`tls.certName`,`tls.key``,`mtls.enable`,`mtls.clietCA`.  See this [issue](https://github.com/VictoriaMetrics/operator/issues/1033) for details.
+- [operator](./README.md): fixes bug with possible `tlsConfig` `SecretOrConfigmap` references clash. Operator adds `configmap` prefix to the configmap refrenced tls asset. See this [issue](https://github.com/VictoriaMetrics/operator/issues/1067) for details.
 - [operator](./README.md): properly release `PodDisruptionBudget` object finalizer. Previously it could be kept due to typo. See this [issue](https://github.com/VictoriaMetrics/operator/issues/1036) for details.
 - [operator](./README.md): refactors finalizers usage. Simplifies finalizer manipulation with helper functions
 - [operator](./README.md): adds `tls_config` and `authKey` settings to auto-created `VMServiceScrape` for CRD objects from `extraArgs`. See [this](https://github.com/VictoriaMetrics/operator/issues/1033) issue for details.

diff --git a/internal/controller/operator/factory/alertmanager/config_test.go b/internal/controller/operator/factory/alertmanager/config_test.go
@@ -1122,7 +1122,7 @@ authorization:
 				},
 			},
 			want: `tls_config:
-  ca_file: /etc/alertmanager/tls_assets/default_cm-store_ca
+  ca_file: /etc/alertmanager/tls_assets/default_configmap_cm-store_ca
   cert_file: /etc/alertmanager/tls_assets/default_secret-store_cert
   insecure_skip_verify: true
   key_file: /etc/alertmanager/tls_assets/default_secret-store_key

diff --git a/internal/controller/operator/factory/build/common.go b/internal/controller/operator/factory/build/common.go
@@ -22,7 +22,8 @@ type TLSConfigBuilder struct {
 	TLSAssets          map[string]string
 }
 
-// BuildTLSConfig return map with tls config keys, let caller to use their own json tag
+// BuildTLSConfig return map with paths to tls config keys
+// let caller to use their own json tag
 func (cb *TLSConfigBuilder) BuildTLSConfig(tlsCfg *vmv1beta1.TLSConfig, tlsAssetsDir string) (map[string]interface{}, error) {
 	if tlsCfg == nil {
 		return nil, nil
@@ -34,22 +35,22 @@ func (cb *TLSConfigBuilder) BuildTLSConfig(tlsCfg *vmv1beta1.TLSConfig, tlsAsset
 	// and be rewrote to new config files in cr's pods for service to use.
 	if tlsCfg.CAFile != "" {
 		result["ca_file"] = tlsCfg.CAFile
-	} else if tlsCfg.CA.Name() != "" {
-		assetKey := tlsCfg.BuildAssetPath(cb.CurrentCRNamespace, tlsCfg.CA.Name(), tlsCfg.CA.Key())
+	} else if tlsCfg.CA.PrefixedName() != "" {
+		assetKey := tlsCfg.BuildAssetPath(cb.CurrentCRNamespace, tlsCfg.CA.PrefixedName(), tlsCfg.CA.Key())
 		if err := cb.fetchSecretWithAssets(tlsCfg.CA.Secret, tlsCfg.CA.ConfigMap, assetKey); err != nil {
 			return nil, fmt.Errorf("cannot fetch ca: %w", err)
 		}
-		result["ca_file"] = tlsCfg.BuildAssetPath(pathPrefix, tlsCfg.CA.Name(), tlsCfg.CA.Key())
+		result["ca_file"] = tlsCfg.BuildAssetPath(pathPrefix, tlsCfg.CA.PrefixedName(), tlsCfg.CA.Key())
 	}
 
 	if tlsCfg.CertFile != "" {
 		result["cert_file"] = tlsCfg.CertFile
-	} else if tlsCfg.Cert.Name() != "" {
-		assetKey := tlsCfg.BuildAssetPath(cb.CurrentCRNamespace, tlsCfg.Cert.Name(), tlsCfg.Cert.Key())
+	} else if tlsCfg.Cert.PrefixedName() != "" {
+		assetKey := tlsCfg.BuildAssetPath(cb.CurrentCRNamespace, tlsCfg.Cert.PrefixedName(), tlsCfg.Cert.Key())
 		if err := cb.fetchSecretWithAssets(tlsCfg.Cert.Secret, tlsCfg.Cert.ConfigMap, assetKey); err != nil {
 			return nil, fmt.Errorf("cannot fetch cert: %w", err)
 		}
-		result["cert_file"] = tlsCfg.BuildAssetPath(pathPrefix, tlsCfg.Cert.Name(), tlsCfg.Cert.Key())
+		result["cert_file"] = tlsCfg.BuildAssetPath(pathPrefix, tlsCfg.Cert.PrefixedName(), tlsCfg.Cert.Key())
 	}
 
 	if tlsCfg.KeyFile != "" {

diff --git a/internal/controller/operator/factory/vmagent/probe_test.go b/internal/controller/operator/factory/vmagent/probe_test.go
@@ -232,7 +232,7 @@ relabel_configs:
 stream_parse: false
 proxy_tls_config:
   insecure_skip_verify: false
-  ca_file: /etc/vmagent-tls/certs/default_tls-secret_ca
+  ca_file: /etc/vmagent-tls/certs/default_configmap_tls-secret_ca
   cert_file: /etc/vmagent-tls/certs/default_tls-secret_cert
   key_file: /tmp/key-1
 bearer_token_file: /tmp/some_path

diff --git a/internal/controller/operator/factory/vmagent/vmagent.go b/internal/controller/operator/factory/vmagent/vmagent.go
@@ -865,66 +865,75 @@ func addAssetsToCache(
 	}
 	assets, nsSecretCache, nsConfigMapCache := ssCache.tlsAssets, ssCache.nsSecretCache, ssCache.nsCMCache
 
-	prefix := objectNS + "/"
-	secretSelectors := map[string]*corev1.SecretKeySelector{}
-	configMapSelectors := map[string]*corev1.ConfigMapKeySelector{}
-	if tlsConfig.CA != (vmv1beta1.SecretOrConfigMap{}) {
-		selectorKey := tlsConfig.CA.BuildSelectorWithPrefix(prefix)
+	fetchAssetFor := func(assetPath string, src vmv1beta1.SecretOrConfigMap) error {
+		var asset string
+		var err error
+		cacheKey := objectNS + "/" + src.PrefixedName()
 		switch {
-		case tlsConfig.CA.Secret != nil:
-			secretSelectors[selectorKey] = tlsConfig.CA.Secret
-		case tlsConfig.CA.ConfigMap != nil:
-			configMapSelectors[selectorKey] = tlsConfig.CA.ConfigMap
+		case src.Secret != nil:
+			asset, err = k8stools.GetCredFromSecret(
+				ctx,
+				rclient,
+				objectNS,
+				src.Secret,
+				cacheKey,
+				nsSecretCache,
+			)
+			if err != nil {
+				return fmt.Errorf(
+					"failed to extract endpoint tls asset from secret %s and key %s in namespace %s: %w",
+					src.PrefixedName(), src.Key(), objectNS, err,
+				)
+			}
+
+		case src.ConfigMap != nil:
+			asset, err = k8stools.GetCredFromConfigMap(
+				ctx,
+				rclient,
+				objectNS,
+				*src.ConfigMap,
+				cacheKey,
+				nsConfigMapCache,
+			)
+			if err != nil {
+				return fmt.Errorf(
+					"failed to extract endpoint tls asset for  configmap %v and key %v in namespace %v",
+					src.PrefixedName(), src.Key(), objectNS,
+				)
+			}
 		}
-	}
-	if tlsConfig.Cert != (vmv1beta1.SecretOrConfigMap{}) {
-		selectorKey := tlsConfig.Cert.BuildSelectorWithPrefix(prefix)
-		switch {
-		case tlsConfig.Cert.Secret != nil:
-			secretSelectors[selectorKey] = tlsConfig.Cert.Secret
-		case tlsConfig.Cert.ConfigMap != nil:
-			configMapSelectors[selectorKey] = tlsConfig.Cert.ConfigMap
+		if len(asset) > 0 {
+			assets[assetPath] = asset
 		}
+		return nil
 	}
-	if tlsConfig.KeySecret != nil {
-		secretSelectors[prefix+tlsConfig.KeySecret.Name+"/"+tlsConfig.KeySecret.Key] = tlsConfig.KeySecret
+
+	if err := fetchAssetFor(tlsConfig.BuildAssetPath(objectNS, tlsConfig.CA.PrefixedName(), tlsConfig.CA.Key()), tlsConfig.CA); err != nil {
+		return fmt.Errorf("cannot fetch CA tls asset: %w", err)
 	}
 
-	for key, selector := range secretSelectors {
+	if err := fetchAssetFor(tlsConfig.BuildAssetPath(objectNS, tlsConfig.Cert.PrefixedName(), tlsConfig.Cert.Key()), tlsConfig.Cert); err != nil {
+		return fmt.Errorf("cannot fetch Cert tls asset: %w", err)
+	}
+
+	if tlsConfig.KeySecret != nil {
 		asset, err := k8stools.GetCredFromSecret(
 			ctx,
 			rclient,
 			objectNS,
-			selector,
-			key,
+			tlsConfig.KeySecret,
+			objectNS+"/"+tlsConfig.KeySecret.Name,
 			nsSecretCache,
 		)
 		if err != nil {
-			return k8stools.NewKeyNotFoundError(
-				selector.Key, fmt.Sprintf("%s/%s/%s", objectNS, selector.Name, key), "tls_secret",
+			return fmt.Errorf(
+				"failed to extract endpoint tls asset from secret %s and key %s in namespace %s",
+				tlsConfig.KeySecret.Name, tlsConfig.KeySecret.Key, objectNS,
 			)
 		}
-
-		assets[tlsConfig.BuildAssetPath(objectNS, selector.Name, selector.Key)] = asset
+		assets[tlsConfig.BuildAssetPath(objectNS, tlsConfig.KeySecret.Name, tlsConfig.KeySecret.Key)] = asset
 	}
 
-	for key, selector := range configMapSelectors {
-		asset, err := k8stools.GetCredFromConfigMap(
-			ctx,
-			rclient,
-			objectNS,
-			*selector,
-			key,
-			nsConfigMapCache,
-		)
-		if err != nil {
-			return k8stools.NewKeyNotFoundError(
-				selector.Key, fmt.Sprintf("%s/%s/%s", objectNS, selector.Name, key), "tls_configmap",
-			)
-		}
-
-		assets[tlsConfig.BuildAssetPath(objectNS, selector.Name, selector.Key)] = asset
-	}
 	return nil
 }
 
@@ -1060,16 +1069,16 @@ func buildRemoteWrites(cr *vmv1beta1.VMAgent, ssCache *scrapesSecretsCache) []st
 		if rws.TLSConfig != nil {
 			if rws.TLSConfig.CAFile != "" {
 				caPath = rws.TLSConfig.CAFile
-			} else if rws.TLSConfig.CA.Name() != "" {
-				caPath = rws.TLSConfig.BuildAssetPath(pathPrefix, rws.TLSConfig.CA.Name(), rws.TLSConfig.CA.Key())
+			} else if rws.TLSConfig.CA.PrefixedName() != "" {
+				caPath = rws.TLSConfig.BuildAssetPath(pathPrefix, rws.TLSConfig.CA.PrefixedName(), rws.TLSConfig.CA.Key())
 			}
 			if caPath != "" {
 				tlsCAs.isNotNull = true
 			}
 			if rws.TLSConfig.CertFile != "" {
 				certPath = rws.TLSConfig.CertFile
-			} else if rws.TLSConfig.Cert.Name() != "" {
-				certPath = rws.TLSConfig.BuildAssetPath(pathPrefix, rws.TLSConfig.Cert.Name(), rws.TLSConfig.Cert.Key())
+			} else if rws.TLSConfig.Cert.PrefixedName() != "" {
+				certPath = rws.TLSConfig.BuildAssetPath(pathPrefix, rws.TLSConfig.Cert.PrefixedName(), rws.TLSConfig.Cert.Key())
 			}
 			if certPath != "" {
 				tlsCerts.isNotNull = true
@@ -1176,7 +1185,7 @@ func buildRemoteWrites(cr *vmv1beta1.VMAgent, ssCache *scrapesSecretsCache) []st
 				oaSecretKeyFile = path.Join(vmAgentConfDir, rws.AsSecretKey(i, "oauth2Secret"))
 			}
 
-			if len(rws.OAuth2.ClientID.Name()) > 0 && sv != nil {
+			if len(rws.OAuth2.ClientID.PrefixedName()) > 0 && sv != nil {
 				oaclientID = sv.ClientID
 				oauth2ClientID.isNotNull = true
 			}

diff --git a/internal/controller/operator/factory/vmagent/vmagent_scrapeconfig.go b/internal/controller/operator/factory/vmagent/vmagent_scrapeconfig.go
@@ -111,7 +111,7 @@ func createOrUpdateConfigurationSecret(ctx context.Context, cr *vmv1beta1.VMAgen
 
 	ssCache, err := loadScrapeSecrets(ctx, rclient, sos, cr.Namespace, cr.Spec.APIServerConfig, cr.Spec.RemoteWrite)
 	if err != nil {
-		return nil, fmt.Errorf("cannot load scrape target secrets for api server or remote writes: %w", err)
+		return nil, fmt.Errorf("cannot load scrape target secrets: %w", err)
 	}
 
 	if err := createOrUpdateTLSAssets(ctx, cr, rclient, ssCache.tlsAssets); err != nil {
@@ -1093,13 +1093,13 @@ func addTLStoYaml(cfg yaml.MapSlice, namespace string, tls *vmv1beta1.TLSConfig,
 		}
 		if tls.CAFile != "" {
 			tlsConfig = append(tlsConfig, yaml.MapItem{Key: "ca_file", Value: tls.CAFile})
-		} else if tls.CA.Name() != "" {
-			tlsConfig = append(tlsConfig, yaml.MapItem{Key: "ca_file", Value: tls.BuildAssetPath(pathPrefix, tls.CA.Name(), tls.CA.Key())})
+		} else if tls.CA.PrefixedName() != "" {
+			tlsConfig = append(tlsConfig, yaml.MapItem{Key: "ca_file", Value: tls.BuildAssetPath(pathPrefix, tls.CA.PrefixedName(), tls.CA.Key())})
 		}
 		if tls.CertFile != "" {
 			tlsConfig = append(tlsConfig, yaml.MapItem{Key: "cert_file", Value: tls.CertFile})
-		} else if tls.Cert.Name() != "" {
-			tlsConfig = append(tlsConfig, yaml.MapItem{Key: "cert_file", Value: tls.BuildAssetPath(pathPrefix, tls.Cert.Name(), tls.Cert.Key())})
+		} else if tls.Cert.PrefixedName() != "" {
+			tlsConfig = append(tlsConfig, yaml.MapItem{Key: "cert_file", Value: tls.BuildAssetPath(pathPrefix, tls.Cert.PrefixedName(), tls.Cert.Key())})
 		}
 		if tls.KeyFile != "" {
 			tlsConfig = append(tlsConfig, yaml.MapItem{Key: "key_file", Value: tls.KeyFile})

diff --git a/internal/controller/operator/factory/vmagent/vmagent_test.go b/internal/controller/operator/factory/vmagent/vmagent_test.go
@@ -598,7 +598,7 @@ func Test_loadTLSAssets(t *testing.T) {
 					Data: map[string]string{"cert": `cert-data`},
 				},
 			},
-			want: map[string]string{"default_tls-secret_cert": "cert-data", "ns-1_tls-access_ca": "cert-data", "ns-1_tls-cm_cert": "cert-data"},
+			want: map[string]string{"default_tls-secret_cert": "cert-data", "ns-1_tls-access_ca": "cert-data", "ns-1_configmap_tls-cm_cert": "cert-data"},
 		},
 
 		{
@@ -721,7 +721,7 @@ func Test_loadTLSAssets(t *testing.T) {
 			},
 			want: map[string]string{
 				"default_tls-secret_cert": "cert-data", "default_remote1-write-spec_ca": "cert-ca", "default_remote1-write-spec_cert": "cert-data", "default_remote1-write-spec_key": "cert-key",
-				"default_name-clash_clash-key": "value-2",
+				"default_name-clash_clash-key": "value-1", "default_configmap_name-clash_clash-key": "value-2",
 			},
 		},
 	}