follow-up after 75a9229

Changes: * added e2e tests * added webhook validation for externalConfig * simplified a bit code at vmauth podSpec generation * added configSecret as deprecated value Signed-off-by: f41gh7 <[email protected]>
VictoriaMetrics · Oct 31, 2024 · d4afd8e · d4afd8e
1 parent 68878b4
commit d4afd8e
Show file tree

Hide file tree

Showing 9 changed files with 328 additions and 1,006 deletions.
diff --git a/api/operator/v1beta1/vmauth_types.go b/api/operator/v1beta1/vmauth_types.go
@@ -73,6 +73,13 @@ type VMAuthSpec struct {
 	// See [here](https://docs.victoriametrics.com/enterprise)
 	// +optional
 	License *License `json:"license,omitempty"`
+	// ConfigSecret is the name of a Kubernetes Secret in the same namespace as the
+	// VMAuth object, which contains auth configuration for vmauth,
+	// configuration must be inside secret key: config.yaml.
+	// It must be created and managed manually.
+	// If it's defined, configuration for vmauth becomes unmanaged and operator'll not create any related secrets/config-reloaders
+	// Deprecated, use externalConfig.secretRef instead
+	ConfigSecret string `json:"configSecret,omitempty"`
 	// ExternalConfig defines a source of external VMAuth configuration.
 	// If it's defined, configuration for vmauth becomes unmanaged and operator'll not create any related secrets/config-reloaders
 	// +optional
@@ -403,7 +410,9 @@ func (cr *VMAuth) AsCRDOwner() []metav1.OwnerReference {
 
 // IsUnmanaged checks if object should managed any  config objects
 func (cr *VMAuth) IsUnmanaged() bool {
-	return (!cr.Spec.SelectAllByDefault && cr.Spec.UserSelector == nil && cr.Spec.UserNamespaceSelector == nil) || (cr.Spec.ExternalConfig.SecretRef == nil && cr.Spec.ExternalConfig.SecretRef.Name != "") || cr.Spec.ExternalConfig.LocalPath != ""
+	return (!cr.Spec.SelectAllByDefault && cr.Spec.UserSelector == nil && cr.Spec.UserNamespaceSelector == nil) ||
+		cr.Spec.ExternalConfig.SecretRef == nil ||
+		cr.Spec.ExternalConfig.LocalPath != ""
 }
 
 // LastAppliedSpecAsPatch return last applied cluster spec as patch annotation

diff --git a/api/operator/v1beta1/vmauth_webhook.go b/api/operator/v1beta1/vmauth_webhook.go
@@ -44,6 +44,21 @@ func (r *VMAuth) sanityCheck() error {
 			return fmt.Errorf("spec.ingress.tlsHosts cannot be empty with non-empty spec.ingress.tlsSecretName")
 		}
 	}
+	if r.Spec.ConfigSecret != "" && r.Spec.ExternalConfig.SecretRef != nil {
+		return fmt.Errorf("spec.configSecret and spec.externalConfig.secretRef cannot be used at the same time")
+	}
+	if r.Spec.ExternalConfig.SecretRef != nil && r.Spec.ExternalConfig.LocalPath != "" {
+		return fmt.Errorf("at most one option can be used for externalConfig: spec.configSecret or spec.externalConfig.secretRef")
+	}
+	if r.Spec.ExternalConfig.SecretRef != nil {
+		if r.Spec.ExternalConfig.SecretRef.Name == r.PrefixedName() {
+			return fmt.Errorf("spec.externalConfig.secretRef cannot be equal to the vmauth-config-CR_NAME=%q, it's operator reserved value", r.ConfigSecretName())
+		}
+		if r.Spec.ExternalConfig.SecretRef.Name == "" || r.Spec.ExternalConfig.SecretRef.Key == "" {
+			return fmt.Errorf("name=%q and key=%q fields must be non-empty for spec.externalConfig.secretRef",
+				r.Spec.ExternalConfig.SecretRef.Name, r.Spec.ExternalConfig.SecretRef.Key)
+		}
+	}
 	return nil
 }
 

diff --git a/config/crd/overlay/crd.yaml b/config/crd/overlay/crd.yaml
diff --git a/internal/controller/operator/factory/build/defaults.go b/internal/controller/operator/factory/build/defaults.go
@@ -180,6 +180,15 @@ func addVMAuthDefaults(objI interface{}) {
 	cr := objI.(*vmv1beta1.VMAuth)
 	c := getCfg()
 
+	if cr.Spec.ConfigSecret != "" {
+		// Removed if later with ConfigSecret field later
+		cr.Spec.ExternalConfig.SecretRef = &corev1.SecretKeySelector{
+			Key: "config.yaml",
+			LocalObjectReference: corev1.LocalObjectReference{
+				Name: cr.Spec.ConfigSecret,
+			},
+		}
+	}
 	cv := config.ApplicationDefaults(c.VMAuthDefault)
 	addDefaultsToCommonParams(&cr.Spec.CommonDefaultableParams, &cv)
 	addDefaluesToConfigReloader(&cr.Spec.CommonConfigReloaderParams, ptr.Deref(cr.Spec.UseDefaultResources, false), &cv)

diff --git a/internal/controller/operator/factory/build/security.go b/internal/controller/operator/factory/build/security.go
@@ -40,17 +40,16 @@ var (
 )
 
 // AddStrictSecuritySettingsToContainers conditionally adds Security settings to given containers
-func AddStrictSecuritySettingsToContainers(p *vmv1beta1.SecurityContext, containers []corev1.Container, enableStrictSecurity bool) []corev1.Container {
+func AddStrictSecuritySettingsToContainers(p *vmv1beta1.SecurityContext, containers []corev1.Container, enableStrictSecurity bool) {
 	if !enableStrictSecurity {
-		return containers
+		return
 	}
 	for idx := range containers {
 		container := &containers[idx]
 		if container.SecurityContext == nil {
 			container.SecurityContext = containerSecurityContext(p)
 		}
 	}
-	return containers
 }
 
 func containerSecurityContext(p *vmv1beta1.SecurityContext) *corev1.SecurityContext {

diff --git a/internal/controller/operator/factory/build/security_test.go b/internal/controller/operator/factory/build/security_test.go
@@ -263,8 +263,8 @@ func TestAddStrictSecuritySettingsToContainers(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			res := AddStrictSecuritySettingsToContainers(tt.args.sc, tt.args.containers, true)
-			assert.Equal(t, tt.expected, res)
+			AddStrictSecuritySettingsToContainers(tt.args.sc, tt.args.containers, true)
+			assert.Equal(t, tt.expected, tt.args.containers)
 		})
 	}
 }
diff --git a/internal/controller/operator/factory/vmauth/vmauth.go b/internal/controller/operator/factory/vmauth/vmauth.go
@@ -40,14 +40,12 @@ const (
 
 // CreateOrUpdateVMAuth - handles VMAuth deployment reconciliation.
 func CreateOrUpdateVMAuth(ctx context.Context, cr *vmv1beta1.VMAuth, rclient client.Client) error {
-	if err := deletePrevStateResources(ctx, cr, rclient); err != nil {
-		return err
-	}
+
 	if cr.IsOwnsServiceAccount() {
 		if err := reconcile.ServiceAccount(ctx, rclient, build.ServiceAccount(cr)); err != nil {
 			return fmt.Errorf("failed create service account: %w", err)
 		}
-		if ptr.Deref(cr.Spec.UseVMConfigReloader, false) && cr.Spec.ExternalConfig.SecretRef == nil {
+		if ptr.Deref(cr.Spec.UseVMConfigReloader, false) {
 			if err := createVMAuthSecretAccess(ctx, cr, rclient); err != nil {
 				return err
 			}
@@ -89,7 +87,13 @@ func CreateOrUpdateVMAuth(ctx context.Context, cr *vmv1beta1.VMAuth, rclient cli
 	if err != nil {
 		return fmt.Errorf("cannot build new deploy for vmauth: %w", err)
 	}
-	return reconcile.Deployment(ctx, rclient, newDeploy, prevDeploy, false)
+	if err := reconcile.Deployment(ctx, rclient, newDeploy, prevDeploy, false); err != nil {
+		return fmt.Errorf("cannot reconcile vmauth deployment: %w", err)
+	}
+	if err := deletePrevStateResources(ctx, cr, rclient); err != nil {
+		return err
+	}
+	return nil
 }
 
 func newDeployForVMAuth(cr *vmv1beta1.VMAuth) (*appsv1.Deployment, error) {
@@ -149,6 +153,9 @@ func makeSpecForVMAuth(cr *vmv1beta1.VMAuth) (*corev1.PodTemplateSpec, error) {
 
 	ports = append(ports, corev1.ContainerPort{Name: "http", Protocol: "TCP", ContainerPort: intstr.Parse(cr.Spec.Port).IntVal})
 
+	useStrictSecurity := ptr.Deref(cr.Spec.UseStrictSecurity, false)
+	useCustomConfigReloader := ptr.Deref(cr.Spec.UseVMConfigReloader, false)
+
 	var volumes []corev1.Volume
 	var volumeMounts []corev1.VolumeMount
 
@@ -191,28 +198,11 @@ func makeSpecForVMAuth(cr *vmv1beta1.VMAuth) (*corev1.PodTemplateSpec, error) {
 	volumes, volumeMounts = cr.Spec.License.MaybeAddToVolumes(volumes, volumeMounts, vmv1beta1.SecretsDir)
 	args = cr.Spec.License.MaybeAddToArgs(args, vmv1beta1.SecretsDir)
 
-	args = build.AddExtraArgsOverrideDefaults(args, cr.Spec.ExtraArgs, "-")
-	sort.Strings(args)
-
-	vmauthContainer := corev1.Container{
-		Name:                     "vmauth",
-		Image:                    fmt.Sprintf("%s:%s", cr.Spec.Image.Repository, cr.Spec.Image.Tag),
-		Ports:                    ports,
-		Args:                     args,
-		VolumeMounts:             volumeMounts,
-		Resources:                cr.Spec.Resources,
-		Env:                      envs,
-		TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError,
-		ImagePullPolicy:          cr.Spec.Image.PullPolicy,
-	}
-	vmauthContainer = build.Probe(vmauthContainer, cr)
-
-	operatorContainers := []corev1.Container{vmauthContainer}
-	useStrictSecurity := ptr.Deref(cr.Spec.UseStrictSecurity, false)
-	useCustomConfigReloader := ptr.Deref(cr.Spec.UseVMConfigReloader, false)
-
 	var initContainers []corev1.Container
-	if cr.Spec.ExternalConfig.SecretRef != nil && cr.Spec.ExternalConfig.SecretRef.Name != "" {
+	var operatorContainers []corev1.Container
+	// config mount options
+	switch {
+	case cr.Spec.ExternalConfig.SecretRef != nil:
 		var keyToPath []corev1.KeyToPath
 		if cr.Spec.ExternalConfig.SecretRef.Key != "" {
 			keyToPath = append(keyToPath, corev1.KeyToPath{
@@ -233,8 +223,12 @@ func makeSpecForVMAuth(cr *vmv1beta1.VMAuth) (*corev1.PodTemplateSpec, error) {
 			Name:      vmAuthVolumeName,
 			MountPath: vmAuthConfigFolder,
 		})
-		operatorContainers[0].VolumeMounts = volumeMounts
-	} else if cr.Spec.ExternalConfig.LocalPath == "" {
+
+	case cr.Spec.ExternalConfig.LocalPath != "":
+		// no-op external managed configuration
+		// add check interval
+		args = append(args, "-configCheckInterval=1m")
+	default:
 		volumes = append(volumes, corev1.Volume{
 			Name: "config-out",
 			VolumeSource: corev1.VolumeSource{
@@ -259,14 +253,32 @@ func makeSpecForVMAuth(cr *vmv1beta1.VMAuth) (*corev1.PodTemplateSpec, error) {
 			Name:      "config-out",
 			MountPath: vmAuthConfigFolder,
 		})
-		operatorContainers[0].VolumeMounts = volumeMounts
 
 		configReloader := buildVMAuthConfigReloaderContainer(cr)
 		operatorContainers = append(operatorContainers, configReloader)
 		initContainers = append(initContainers,
 			buildInitConfigContainer(useCustomConfigReloader, cr.Spec.ConfigReloaderImageTag, cr.Spec.ConfigReloaderResources, configReloader.Args)...)
 	}
 
+	args = build.AddExtraArgsOverrideDefaults(args, cr.Spec.ExtraArgs, "-")
+	sort.Strings(args)
+
+	vmauthContainer := corev1.Container{
+		Name:                     "vmauth",
+		Image:                    fmt.Sprintf("%s:%s", cr.Spec.Image.Repository, cr.Spec.Image.Tag),
+		Ports:                    ports,
+		Args:                     args,
+		VolumeMounts:             volumeMounts,
+		Resources:                cr.Spec.Resources,
+		Env:                      envs,
+		TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError,
+		ImagePullPolicy:          cr.Spec.Image.PullPolicy,
+	}
+	vmauthContainer = build.Probe(vmauthContainer, cr)
+
+	// move vmauth container to the 0 index
+	operatorContainers = append([]corev1.Container{vmauthContainer}, operatorContainers...)
+
 	build.AddStrictSecuritySettingsToContainers(cr.Spec.SecurityContext, operatorContainers, useStrictSecurity)
 	containers, err := k8stools.MergePatchContainers(operatorContainers, cr.Spec.Containers)
 	if err != nil {
@@ -299,7 +311,7 @@ func makeSpecForVMAuth(cr *vmv1beta1.VMAuth) (*corev1.PodTemplateSpec, error) {
 // CreateOrUpdateVMAuthConfig configuration secret for vmauth.
 func CreateOrUpdateVMAuthConfig(ctx context.Context, rclient client.Client, cr *vmv1beta1.VMAuth) error {
 	// fast path
-	if cr.Spec.ExternalConfig.SecretRef != nil && cr.Spec.ExternalConfig.SecretRef.Name != "" {
+	if cr.Spec.ExternalConfig.SecretRef != nil || cr.Spec.ExternalConfig.LocalPath != "" {
 		return nil
 	}
 	s := makeVMAuthConfigSecret(cr)
@@ -571,24 +583,26 @@ func deletePrevStateResources(ctx context.Context, cr *vmv1beta1.VMAuth, rclient
 	if cr.ParsedLastAppliedSpec == nil {
 		return nil
 	}
-	prevSvc, currSvc := cr.ParsedLastAppliedSpec.ServiceSpec, cr.Spec.ServiceSpec
+	prevCR := cr.DeepCopy()
+	prevCR.Spec = *cr.ParsedLastAppliedSpec
+	prevSvc, currSvc := prevCR.Spec.ServiceSpec, cr.Spec.ServiceSpec
 	if err := reconcile.AdditionalServices(ctx, rclient, cr.PrefixedName(), cr.Namespace, prevSvc, currSvc); err != nil {
 		return fmt.Errorf("cannot remove additional service: %w", err)
 	}
 
 	objMeta := metav1.ObjectMeta{Name: cr.PrefixedName(), Namespace: cr.Namespace}
-	if cr.Spec.PodDisruptionBudget == nil && cr.ParsedLastAppliedSpec.PodDisruptionBudget != nil {
+	if cr.Spec.PodDisruptionBudget == nil && prevCR.Spec.PodDisruptionBudget != nil {
 		if err := finalize.SafeDeleteWithFinalizer(ctx, rclient, &policyv1.PodDisruptionBudget{ObjectMeta: objMeta}); err != nil {
 			return fmt.Errorf("cannot delete PDB from prev state: %w", err)
 		}
 	}
 
-	if cr.Spec.Ingress == nil && cr.ParsedLastAppliedSpec.Ingress != nil {
+	if cr.Spec.Ingress == nil && prevCR.Spec.Ingress != nil {
 		if err := finalize.SafeDeleteWithFinalizer(ctx, rclient, &networkingv1.Ingress{ObjectMeta: objMeta}); err != nil {
 			return fmt.Errorf("cannot delete ingress from prev state: %w", err)
 		}
 	}
-	if ptr.Deref(cr.Spec.DisableSelfServiceScrape, false) && !ptr.Deref(cr.ParsedLastAppliedSpec.DisableSelfServiceScrape, false) {
+	if ptr.Deref(cr.Spec.DisableSelfServiceScrape, false) && !ptr.Deref(prevCR.Spec.DisableSelfServiceScrape, false) {
 		if err := finalize.SafeDeleteWithFinalizer(ctx, rclient, &vmv1beta1.VMServiceScrape{ObjectMeta: objMeta}); err != nil {
 			return fmt.Errorf("cannot remove serviceScrape: %w", err)
 		}

diff --git a/internal/controller/operator/factory/vmcluster/vmcluster.go b/internal/controller/operator/factory/vmcluster/vmcluster.go
@@ -1480,7 +1480,7 @@ func buildVMauthLBDeployment(cr *vmv1beta1.VMCluster) (*appsv1.Deployment, error
 	containers := []corev1.Container{
 		vmauthLBCnt,
 	}
-	containers = build.AddStrictSecuritySettingsToContainers(spec.SecurityContext, containers, ptr.Deref(spec.UseStrictSecurity, false))
+	build.AddStrictSecuritySettingsToContainers(spec.SecurityContext, containers, ptr.Deref(spec.UseStrictSecurity, false))
 
 	var err error
 	containers, err = k8stools.MergePatchContainers(containers, spec.Containers)