diff --git a/api/v1/aerospikecluster_validating_webhook.go b/api/v1/aerospikecluster_validating_webhook.go index 0a8e205fa..0fa995e08 100644 --- a/api/v1/aerospikecluster_validating_webhook.go +++ b/api/v1/aerospikecluster_validating_webhook.go @@ -19,6 +19,7 @@ package v1 import ( "crypto/x509" "encoding/pem" + "errors" "fmt" "os" "path/filepath" @@ -37,6 +38,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + internalerrors "github.com/aerospike/aerospike-kubernetes-operator/errors" "github.com/aerospike/aerospike-management-lib/asconfig" "github.com/aerospike/aerospike-management-lib/deployment" ) @@ -114,7 +116,8 @@ func (c *AerospikeCluster) ValidateUpdate(oldObj runtime.Object) (admission.Warn // Validate AerospikeConfig update if err := validateAerospikeConfigUpdate( - aslog, c.Spec.AerospikeConfig, old.Spec.AerospikeConfig, + aslog, incomingVersion, outgoingVersion, + c.Spec.AerospikeConfig, old.Spec.AerospikeConfig, c.Status.AerospikeConfig, ); err != nil { return nil, err @@ -389,6 +392,16 @@ func (c *AerospikeCluster) validateRackUpdate( return nil } + outgoingVersion, err := GetImageVersion(old.Spec.Image) + if err != nil { + return err + } + + incomingVersion, err := GetImageVersion(c.Spec.Image) + if err != nil { + return err + } + // Old racks cannot be updated // Also need to exclude a default rack with default rack ID. No need to check here, // user should not provide or update default rackID @@ -422,7 +435,8 @@ func (c *AerospikeCluster) validateRackUpdate( // Validate aerospikeConfig update if err := validateAerospikeConfigUpdate( - aslog, &newRack.AerospikeConfig, &oldRack.AerospikeConfig, + aslog, incomingVersion, outgoingVersion, + &newRack.AerospikeConfig, &oldRack.AerospikeConfig, rackStatusConfig, ); err != nil { return fmt.Errorf( @@ -1252,11 +1266,97 @@ func getNamespaceReplicationFactor(nsConf map[string]interface{}) (int, error) { return rf, nil } +func validateSecurityConfigUpdate( + newVersion, oldVersion string, newSpec, oldSpec *AerospikeConfigSpec, +) error { + nv, err := lib.CompareVersions(newVersion, "5.7.0") + if err != nil { + return err + } + + ov, err := lib.CompareVersions(oldVersion, "5.7.0") + if err != nil { + return err + } + + if nv >= 0 || ov >= 0 { + return validateSecurityContext(newVersion, oldVersion, newSpec, oldSpec) + } + + return validateEnableSecurityConfig(newSpec, oldSpec) +} + +func validateEnableSecurityConfig(newConfSpec, oldConfSpec *AerospikeConfigSpec) error { + newConf := newConfSpec.Value + oldConf := oldConfSpec.Value + oldSec, oldSecConfFound := oldConf["security"] + newSec, newSecConfFound := newConf["security"] + + if oldSecConfFound && !newSecConfFound { + return fmt.Errorf("cannot remove cluster security config") + } + + if oldSecConfFound && newSecConfFound { + oldSecFlag, oldEnableSecurityFlagFound := oldSec.(map[string]interface{})["enable-security"] + newSecFlag, newEnableSecurityFlagFound := newSec.(map[string]interface{})["enable-security"] + + if oldEnableSecurityFlagFound && oldSecFlag.(bool) && !newEnableSecurityFlagFound { + return fmt.Errorf("cannot disable cluster security in running cluster") + } + + if oldEnableSecurityFlagFound && newEnableSecurityFlagFound || !reflect.DeepEqual( + oldSecFlag, newSecFlag, + ) { + if oldSecFlag.(bool) && !newSecFlag.(bool) { + return fmt.Errorf("cannot disable cluster security in running cluster") + } + } + } + + return nil +} + +func validateSecurityContext( + newVersion, oldVersion string, newSpec, oldSpec *AerospikeConfigSpec, +) error { + ovflag, err := IsSecurityEnabled(oldVersion, oldSpec) + if err != nil { + if !errors.Is(err, internalerrors.ErrNotFound) { + return fmt.Errorf( + "validateEnableSecurityConfig got an error - oldVersion: %s: %w", + oldVersion, err, + ) + } + } + + ivflag, err := IsSecurityEnabled(newVersion, newSpec) + if err != nil { + if !errors.Is(err, internalerrors.ErrNotFound) { + return fmt.Errorf( + "validateEnableSecurityConfig got an error: %w", err, + ) + } + } + + if !ivflag && ovflag { + return fmt.Errorf("cannot disable cluster security in running cluster") + } + + return nil +} + func validateAerospikeConfigUpdate( - aslog logr.Logger, incomingSpec, outgoingSpec, currentStatus *AerospikeConfigSpec, + aslog logr.Logger, incomingVersion, outgoingVersion string, + incomingSpec, outgoingSpec, currentStatus *AerospikeConfigSpec, ) error { aslog.Info("Validate AerospikeConfig update") + if err := validateSecurityConfigUpdate( + incomingVersion, outgoingVersion, incomingSpec, outgoingSpec, + ); err != nil { + return err + } + newConf := incomingSpec.Value oldConf := outgoingSpec.Value diff --git a/controllers/access_control.go b/controllers/access_control.go index 52f552fe8..a362ff5ed 100644 --- a/controllers/access_control.go +++ b/controllers/access_control.go @@ -64,19 +64,12 @@ func AerospikeAdminCredentials( desiredSecurityErr = incomingVersionErr } - if currentSecurityErr == nil && desiredSecurityErr == nil { - // If security is enabled in either current or desired state, return security enabled credentials. - enabled = currentSecurityEnabled || desiredSecurityEnabled - } else if currentSecurityErr != nil && desiredSecurityErr != nil { + if currentSecurityErr != nil && desiredSecurityErr != nil { return "", "", desiredSecurityErr - } else { - if currentSecurityErr != nil { - enabled = desiredSecurityEnabled - } else { - enabled = currentSecurityEnabled - } } + enabled = currentSecurityEnabled || desiredSecurityEnabled + if !enabled { // Return zero strings if this is not a security enabled cluster. return "", "", nil diff --git a/controllers/rack.go b/controllers/rack.go index 17843896d..66e269349 100644 --- a/controllers/rack.go +++ b/controllers/rack.go @@ -120,34 +120,6 @@ func (r *SingleClusterReconciler) reconcileRacks() reconcileResult { } } - if r.aeroCluster.Status.Pods != nil { - securityDisabledPodNames := r.getsecurityDisabledPodNames() - - // Setup access control. - if err := r.validateAndReconcileAccessControl(securityDisabledPodNames.Union(ignorablePodNames)); err != nil { - r.Log.Error(err, "Failed to Reconcile access control") - r.Recorder.Eventf( - r.aeroCluster, corev1.EventTypeWarning, "ACLUpdateFailed", - "Failed to setup Access Control %s/%s", r.aeroCluster.Namespace, - r.aeroCluster.Name, - ) - - return reconcileError(err) - } - - // Update the AerospikeCluster status. - if err := r.updateAccessControlStatus(); err != nil { - r.Log.Error(err, "Failed to update AerospikeCluster access control status") - r.Recorder.Eventf( - r.aeroCluster, corev1.EventTypeWarning, "StatusUpdateFailed", - "Failed to update AerospikeCluster access control status %s/%s", - r.aeroCluster.Namespace, r.aeroCluster.Name, - ) - - return reconcileError(err) - } - } - for idx := range rackStateList { state := &rackStateList[idx] found := &appsv1.StatefulSet{} diff --git a/controllers/reconciler.go b/controllers/reconciler.go index 586a7116c..e1a96405a 100644 --- a/controllers/reconciler.go +++ b/controllers/reconciler.go @@ -100,6 +100,34 @@ func (r *SingleClusterReconciler) Reconcile() (ctrl.Result, error) { return reconcile.Result{}, err } + if r.aeroCluster.Status.Pods != nil && r.enablingSecurity() { + securityEnabledPods, err := r.getsecurityEnabledPods() + if err != nil { + return reconcile.Result{}, err + } + + if len(securityEnabledPods) > 0 { + ignorablePodNames, err := r.getIgnorablePods(nil, getConfiguredRackStateList(r.aeroCluster)) + if err != nil { + r.Log.Error(err, "Failed to determine pods to be ignored") + + return reconcile.Result{}, err + } + + // Setup access control. + if err := r.validateAndReconcileAccessControl(securityEnabledPods, ignorablePodNames); err != nil { + r.Log.Error(err, "Failed to Reconcile access control") + r.Recorder.Eventf( + r.aeroCluster, corev1.EventTypeWarning, "ACLUpdateFailed", + "Failed to setup Access Control %s/%s", r.aeroCluster.Namespace, + r.aeroCluster.Name, + ) + + return reconcile.Result{}, err + } + } + } + // Reconcile all racks if res := r.reconcileRacks(); !res.isSuccess { if res.err != nil { @@ -144,7 +172,7 @@ func (r *SingleClusterReconciler) Reconcile() (ctrl.Result, error) { return reconcile.Result{}, e } - if err := deployment.InfoQuiesceUndo( + if err = deployment.InfoQuiesceUndo( r.Log, r.getClientPolicy(), allHostConns, ); err != nil { @@ -152,8 +180,14 @@ func (r *SingleClusterReconciler) Reconcile() (ctrl.Result, error) { return reconcile.Result{}, err } + podList, err := r.getClusterPodList() + if err != nil { + return reconcile.Result{}, err + } + // Setup access control. - if err := r.validateAndReconcileAccessControl(ignorablePodNames); err != nil { + // Assuming all pods must be security enabled or disabled. + if err := r.validateAndReconcileAccessControl(podList.Items, ignorablePodNames); err != nil { r.Log.Error(err, "Failed to Reconcile access control") r.Recorder.Eventf( r.aeroCluster, corev1.EventTypeWarning, "ACLUpdateFailed", @@ -164,18 +198,6 @@ func (r *SingleClusterReconciler) Reconcile() (ctrl.Result, error) { return reconcile.Result{}, err } - // Update the AerospikeCluster status. - if err := r.updateAccessControlStatus(); err != nil { - r.Log.Error(err, "Failed to update AerospikeCluster access control status") - r.Recorder.Eventf( - r.aeroCluster, corev1.EventTypeWarning, "StatusUpdateFailed", - "Failed to update AerospikeCluster access control status %s/%s", - r.aeroCluster.Namespace, r.aeroCluster.Name, - ) - - return reconcile.Result{}, err - } - // Use policy from spec after setting up access control policy := r.getClientPolicy() @@ -264,7 +286,8 @@ func (r *SingleClusterReconciler) recoverIgnorablePods() reconcileResult { return reconcileSuccess() } -func (r *SingleClusterReconciler) validateAndReconcileAccessControl(ignorablePodNames sets.Set[string]) error { +func (r *SingleClusterReconciler) validateAndReconcileAccessControl(securityEnabledPods []corev1.Pod, + ignorablePodNames sets.Set[string]) error { version, err := asdbv1.GetImageVersion(r.aeroCluster.Spec.Image) if err != nil { return err @@ -282,8 +305,10 @@ func (r *SingleClusterReconciler) validateAndReconcileAccessControl(ignorablePod return nil } + var conns []*deployment.HostConn + // Create client - conns, err := r.newAllHostConnWithOption(ignorablePodNames) + conns, err = r.newPodsHostConnWithOption(securityEnabledPods, ignorablePodNames) if err != nil { return fmt.Errorf("failed to get host info: %v", err) } @@ -302,6 +327,7 @@ func (r *SingleClusterReconciler) validateAndReconcileAccessControl(ignorablePod // Create policy using status, status has current connection info clientPolicy := r.getClientPolicy() + aeroClient, err := as.NewClientWithPolicyAndHost(clientPolicy, hosts...) if err != nil { @@ -315,15 +341,30 @@ func (r *SingleClusterReconciler) validateAndReconcileAccessControl(ignorablePod err = r.reconcileAccessControl( aeroClient, pp, ) - if err == nil { + + if err != nil { + return fmt.Errorf("failed to reconcile access control: %v", err) + } + + r.Recorder.Eventf( + r.aeroCluster, corev1.EventTypeNormal, "ACLUpdated", + "Updated Access Control %s/%s", r.aeroCluster.Namespace, + r.aeroCluster.Name, + ) + + // Update the AerospikeCluster status. + if err := r.updateAccessControlStatus(); err != nil { + r.Log.Error(err, "Failed to update AerospikeCluster access control status") r.Recorder.Eventf( - r.aeroCluster, corev1.EventTypeNormal, "ACLUpdated", - "Updated Access Control %s/%s", r.aeroCluster.Namespace, - r.aeroCluster.Name, + r.aeroCluster, corev1.EventTypeWarning, "StatusUpdateFailed", + "Failed to update AerospikeCluster access control status %s/%s", + r.aeroCluster.Namespace, r.aeroCluster.Name, ) + + return err } - return err + return nil } func (r *SingleClusterReconciler) updateStatus() error { @@ -920,13 +961,29 @@ func (r *SingleClusterReconciler) AddAPIVersionLabel(ctx context.Context) error return r.Client.Update(ctx, aeroCluster, updateOption) } -func (r *SingleClusterReconciler) getsecurityDisabledPodNames() sets.Set[string] { - securityDisabledPods := sets.Set[string]{} +func (r *SingleClusterReconciler) getsecurityEnabledPods() ([]corev1.Pod, error) { + securityEnabledPods := make([]corev1.Pod, 0, len(r.aeroCluster.Status.Pods)) + for podName := range r.aeroCluster.Status.Pods { - if !r.aeroCluster.Status.Pods[podName].SecurityEnabled { - securityDisabledPods.Insert(podName) + if r.aeroCluster.Status.Pods[podName].SecurityEnabled { + pod := &corev1.Pod{} + podName := types.NamespacedName{Name: podName, Namespace: r.aeroCluster.Namespace} + + if err := r.Client.Get(context.TODO(), podName, pod); err != nil { + return securityEnabledPods, err + } + + securityEnabledPods = append(securityEnabledPods, *pod) } } - return securityDisabledPods + return securityEnabledPods, nil +} + +func (r *SingleClusterReconciler) enablingSecurity() bool { + if r.aeroCluster.Spec.AerospikeAccessControl != nil && r.aeroCluster.Status.AerospikeAccessControl == nil { + return true + } + + return false } diff --git a/test/access_control_test.go b/test/access_control_test.go index 08eb10ebf..b205b7843 100644 --- a/test/access_control_test.go +++ b/test/access_control_test.go @@ -1626,7 +1626,7 @@ var _ = Describe( ctx := goctx.Background() It( - "SecurityUpdateReject: should fail, Cannot update cluster security config", + "SecurityEnable: should enable security in running cluster", func() { var accessControl *asdbv1.AerospikeAccessControlSpec @@ -1712,11 +1712,8 @@ var _ = Describe( err = testAccessControlReconcile( aeroCluster, ctx, ) - if err == nil || !strings.Contains( - err.Error(), - "cannot update cluster security config", - ) { - Fail("SecurityUpdate should have failed") + if err != nil { + Fail("Security should have enabled successfully") } if aeroCluster != nil { @@ -1890,7 +1887,7 @@ var _ = Describe( ) if err == nil || !strings.Contains( err.Error(), - "cannot update cluster security config", + "cannot disable cluster security in running cluster", ) { Fail("SecurityUpdate should have failed") }