diff --git a/common/common.go b/common/common.go index 65e7347e82c..1eb9372df4a 100644 --- a/common/common.go +++ b/common/common.go @@ -17,7 +17,11 @@ limitations under the License. package common import ( + "fmt" + "maps" "os" + "slices" + "strings" ) const ( @@ -57,6 +61,31 @@ func InfraNodeSelector() map[string]string { } } +// NamespaceNodeSelectorAnnotation builds the openshift.io/node-selector value for the +// default GitOps namespace. runOnInfra uses the infra role; GitopsService nodeSelector +// entries are merged. An empty result means the annotation should be removed. +func NamespaceNodeSelectorAnnotation(runOnInfra bool, nodeSelector map[string]string) string { + if !runOnInfra && len(nodeSelector) == 0 { + return "" + } + if runOnInfra && len(nodeSelector) == 0 { + return InfraNodeSelectorAnnotationValue + } + sel := maps.Clone(nodeSelector) + if sel == nil { + sel = map[string]string{} + } + if runOnInfra { + sel[InfraNodeLabelSelector] = "" + } + keys := slices.Sorted(maps.Keys(sel)) + parts := make([]string, 0, len(keys)) + for _, k := range keys { + parts = append(parts, fmt.Sprintf("%s=%s", k, sel[k])) + } + return strings.Join(parts, ",") +} + func StringFromEnv(env string, defaultValue string) string { if str := os.Getenv(env); str != "" { return str diff --git a/common/common_test.go b/common/common_test.go new file mode 100644 index 00000000000..0d6b8252d14 --- /dev/null +++ b/common/common_test.go @@ -0,0 +1,36 @@ +package common + +import "testing" + +func TestNamespaceNodeSelectorAnnotation(t *testing.T) { + tests := map[string]struct { + runOnInfra bool + nodeSelector map[string]string + want string + }{ + "neither": { + want: "", + }, + "runOnInfra only keeps exact infra value": { + runOnInfra: true, + want: InfraNodeSelectorAnnotationValue, + }, + "custom selector": { + nodeSelector: map[string]string{"foo": "bar"}, + want: "foo=bar", + }, + "runOnInfra plus custom merges": { + runOnInfra: true, + nodeSelector: map[string]string{"foo": "bar"}, + want: "foo=bar,node-role.kubernetes.io/infra=", + }, + } + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + got := NamespaceNodeSelectorAnnotation(tt.runOnInfra, tt.nodeSelector) + if got != tt.want { + t.Fatalf("got %q, want %q", got, tt.want) + } + }) + } +} diff --git a/controllers/gitopsservice_controller.go b/controllers/gitopsservice_controller.go index d37b887aec1..eb0f5c09844 100644 --- a/controllers/gitopsservice_controller.go +++ b/controllers/gitopsservice_controller.go @@ -264,7 +264,7 @@ func (r *ReconcileGitopsService) Reconcile(ctx context.Context, request reconcil if err != nil { if errors.IsNotFound(err) { reqLogger.Info("Creating a new Namespace", "Name", namespace) - ensureInfraNodeSelectorAnnotation(namespaceRef, instance.Spec.RunOnInfra) + ensureInfraNodeSelectorAnnotation(namespaceRef, instance.Spec.RunOnInfra, instance.Spec.NodeSelector) err = r.Client.Create(ctx, namespaceRef) if err != nil { return reconcile.Result{}, err @@ -273,7 +273,7 @@ func (r *ReconcileGitopsService) Reconcile(ctx context.Context, request reconcil return reconcile.Result{}, err } } else { - if ensureNamespaceMetadata(namespaceRef, instance.Spec.RunOnInfra) { + if ensureNamespaceMetadata(namespaceRef, instance.Spec.RunOnInfra, instance.Spec.NodeSelector) { err = r.Client.Update(context.TODO(), namespaceRef) if err != nil { return reconcile.Result{}, err @@ -460,7 +460,7 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli if err != nil { if errors.IsNotFound(err) { reqLogger.Info("Creating a new Namespace", "Name", argocdNS.Name) - ensureInfraNodeSelectorAnnotation(argocdNS, instance.Spec.RunOnInfra) + ensureInfraNodeSelectorAnnotation(argocdNS, instance.Spec.RunOnInfra, instance.Spec.NodeSelector) err = r.Client.Create(context.TODO(), argocdNS) if err != nil { return reconcile.Result{}, err @@ -490,7 +490,7 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli } } - if ensureNamespaceMetadata(argocdNS, instance.Spec.RunOnInfra) { + if ensureNamespaceMetadata(argocdNS, instance.Spec.RunOnInfra, instance.Spec.NodeSelector) { err = r.Client.Update(context.TODO(), argocdNS) if err != nil { return reconcile.Result{}, err @@ -504,15 +504,9 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli return reconcile.Result{}, err } - if len(instance.Spec.NodeSelector) > 0 { - if defaultArgoCDInstance.Spec.NodePlacement == nil { - defaultArgoCDInstance.Spec.NodePlacement = &argoapp.ArgoCDNodePlacementSpec{ - NodeSelector: instance.Spec.NodeSelector, - } - } else { - defaultArgoCDInstance.Spec.NodePlacement.NodeSelector = argocdutil.AppendStringMap(defaultArgoCDInstance.Spec.NodePlacement.NodeSelector, instance.Spec.NodeSelector) - } - } + // Node selectors for the default instance are applied on the openshift-gitops + // namespace (openshift.io/node-selector), not on ArgoCD spec.nodePlacement. + // GitopsService still owns NodePlacement.Tolerations for taints. if len(instance.Spec.Tolerations) > 0 { if defaultArgoCDInstance.Spec.NodePlacement == nil { defaultArgoCDInstance.Spec.NodePlacement = &argoapp.ArgoCDNodePlacementSpec{ @@ -588,13 +582,13 @@ func (r *ReconcileGitopsService) reconcileDefaultArgoCDInstance(instance *pipeli changed = true } - // if user is patching nodePlacement through GitopsService CR, then existingArgoCD NodePlacement is updated. + // GitopsService owns the default ArgoCD instance. Sync NodePlacement from + // GitopsService (tolerations only; node selectors use the namespace annotation). if defaultArgoCDInstance.Spec.NodePlacement != nil { if !reflect.DeepEqual(existingArgoCD.Spec.NodePlacement, defaultArgoCDInstance.Spec.NodePlacement) { existingArgoCD.Spec.NodePlacement = defaultArgoCDInstance.Spec.NodePlacement changed = true } - // Handle the case where NodePlacement should be removed } else if existingArgoCD.Spec.NodePlacement != nil { existingArgoCD.Spec.NodePlacement = defaultArgoCDInstance.Spec.NodePlacement changed = true @@ -1057,11 +1051,11 @@ func policyRuleForBackendServiceClusterRole() []rbacv1.PolicyRule { } } -func ensureNamespaceMetadata(namespace *corev1.Namespace, runOnInfra bool) bool { +func ensureNamespaceMetadata(namespace *corev1.Namespace, runOnInfra bool, nodeSelector map[string]string) bool { changed := false labelUpdate, _ := ensurePodSecurityLabels(namespace) changed = changed || labelUpdate - annotationUpdate, _ := ensureInfraNodeSelectorAnnotation(namespace, runOnInfra) + annotationUpdate, _ := ensureInfraNodeSelectorAnnotation(namespace, runOnInfra, nodeSelector) changed = changed || annotationUpdate return changed } @@ -1077,20 +1071,22 @@ func ensurePodSecurityLabels(namespace *corev1.Namespace) (bool, *corev1.Namespa return false, namespace } -func ensureInfraNodeSelectorAnnotation(namespace *corev1.Namespace, runOnInfra bool) (bool, *corev1.Namespace) { +func ensureInfraNodeSelectorAnnotation(namespace *corev1.Namespace, runOnInfra bool, nodeSelector map[string]string) (bool, *corev1.Namespace) { if namespace.Annotations == nil { namespace.Annotations = make(map[string]string) } - if runOnInfra { - if namespace.Annotations[common.InfraNodeSelectorAnnotation] != common.InfraNodeSelectorAnnotationValue { - namespace.Annotations[common.InfraNodeSelectorAnnotation] = common.InfraNodeSelectorAnnotationValue - return true, namespace - } - } else { - if namespace.Annotations[common.InfraNodeSelectorAnnotation] == common.InfraNodeSelectorAnnotationValue { - delete(namespace.Annotations, common.InfraNodeSelectorAnnotation) - return true, namespace + want := common.NamespaceNodeSelectorAnnotation(runOnInfra, nodeSelector) + got := namespace.Annotations[common.InfraNodeSelectorAnnotation] + if want == "" { + if got == "" { + return false, namespace } + delete(namespace.Annotations, common.InfraNodeSelectorAnnotation) + return true, namespace } - return false, namespace + if got == want { + return false, namespace + } + namespace.Annotations[common.InfraNodeSelectorAnnotation] = want + return true, namespace } diff --git a/controllers/gitopsservice_controller_test.go b/controllers/gitopsservice_controller_test.go index 160b96c82ae..7eb72d359d1 100644 --- a/controllers/gitopsservice_controller_test.go +++ b/controllers/gitopsservice_controller_test.go @@ -152,8 +152,6 @@ func TestReconcileDefaultForArgoCDNodeplacement(t *testing.T) { s := scheme.Scheme addKnownTypesToScheme(s) - var err error - gitopsService := &pipelinesv1alpha1.GitopsService{ ObjectMeta: v1.ObjectMeta{ Name: serviceName, @@ -189,18 +187,103 @@ func TestReconcileDefaultForArgoCDNodeplacement(t *testing.T) { }, } - err = fakeClient.Create(context.TODO(), existingArgoCD) + err := fakeClient.Create(context.TODO(), existingArgoCD) + assertNoError(t, err) + + _, err = reconciler.Reconcile(context.TODO(), newRequest("test", "test")) + assertNoError(t, err) + + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: common.ArgoCDInstanceName, Namespace: serviceNamespace}, + existingArgoCD) + assertNoError(t, err) + // Node selectors are applied on the namespace, not on the default ArgoCD CR. + assert.Assert(t, existingArgoCD.Spec.NodePlacement == nil) + + ns := &corev1.Namespace{} + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: serviceNamespace}, ns) + assertNoError(t, err) + assert.Equal(t, ns.Annotations[common.InfraNodeSelectorAnnotation], "foo=bar") +} + +func TestReconcileDefaultArgoCDNodePlacementClearsWhenGitopsServiceCleared(t *testing.T) { + logf.SetLogger(argocd.ZapLogger(true)) + s := scheme.Scheme + addKnownTypesToScheme(s) + + gitopsService := &pipelinesv1alpha1.GitopsService{ + ObjectMeta: v1.ObjectMeta{ + Name: serviceName, + }, + Spec: pipelinesv1alpha1.GitopsServiceSpec{ + NodeSelector: map[string]string{ + "key1": "value1", + }, + Tolerations: deploymentDefaultTolerations(), + }, + } + + fakeClient := fake.NewFakeClient(gitopsService) + reconciler := newReconcileGitOpsService(fakeClient, s) + + existingArgoCD := &argoapp.ArgoCD{ + ObjectMeta: v1.ObjectMeta{ + Name: serviceNamespace, + Namespace: serviceNamespace, + }, + Spec: argoapp.ArgoCDSpec{ + Server: argoapp.ArgoCDServerSpec{ + Route: argoapp.ArgoCDRouteSpec{ + Enabled: true, + }, + }, + ApplicationSet: &argoapp.ArgoCDApplicationSet{}, + SSO: &argoapp.ArgoCDSSOSpec{ + Provider: "dex", + Dex: &argoapp.ArgoCDDexSpec{ + Config: "test-config", + }, + }, + }, + } + + err := fakeClient.Create(context.TODO(), existingArgoCD) assertNoError(t, err) _, err = reconciler.Reconcile(context.TODO(), newRequest("test", "test")) assertNoError(t, err) - // verify whether existingArgoCD NodePlacement is updated when user is patching nodePlacement through GitopsService CR err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: common.ArgoCDInstanceName, Namespace: serviceNamespace}, existingArgoCD) assertNoError(t, err) assert.Check(t, existingArgoCD.Spec.NodePlacement != nil) - assert.DeepEqual(t, existingArgoCD.Spec.NodePlacement.NodeSelector, gitopsService.Spec.NodeSelector) + assert.Assert(t, existingArgoCD.Spec.NodePlacement.NodeSelector == nil) + assert.DeepEqual(t, existingArgoCD.Spec.NodePlacement.Tolerations, deploymentDefaultTolerations()) + + ns := &corev1.Namespace{} + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: serviceNamespace}, ns) + assertNoError(t, err) + assert.Equal(t, ns.Annotations[common.InfraNodeSelectorAnnotation], "key1=value1") + + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: serviceName}, gitopsService) + assertNoError(t, err) + gitopsService.Spec.NodeSelector = nil + gitopsService.Spec.Tolerations = nil + err = fakeClient.Update(context.TODO(), gitopsService) + assertNoError(t, err) + + _, err = reconciler.Reconcile(context.TODO(), newRequest("test", "test")) + assertNoError(t, err) + + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: common.ArgoCDInstanceName, Namespace: serviceNamespace}, + existingArgoCD) + assertNoError(t, err) + assert.Assert(t, existingArgoCD.Spec.NodePlacement == nil) + + ns = &corev1.Namespace{} + err = fakeClient.Get(context.TODO(), types.NamespacedName{Name: serviceNamespace}, ns) + assertNoError(t, err) + _, hasAnnotation := ns.Annotations[common.InfraNodeSelectorAnnotation] + assert.Assert(t, !hasAnnotation) } // If the DISABLE_DEFAULT_ARGOCD_INSTANCE is set, ensure that the default ArgoCD instance is not created. diff --git a/test/openshift/e2e/ginkgo/sequential/1-071_validate_node_selectors_test.go b/test/openshift/e2e/ginkgo/sequential/1-071_validate_node_selectors_test.go index 7e7556aa921..c2655091863 100644 --- a/test/openshift/e2e/ginkgo/sequential/1-071_validate_node_selectors_test.go +++ b/test/openshift/e2e/ginkgo/sequential/1-071_validate_node_selectors_test.go @@ -68,19 +68,27 @@ var _ = Describe("GitOps Operator Sequential E2E Tests", func() { gs.Spec.NodeSelector["key1"] = "value1" }) - By("ensuring Deployments and StatefulSets pick up the change we made to nodeSelector in GitOpsService CR") + By("ensuring the openshift-gitops namespace annotation picks up GitOpsService nodeSelector") + openshiftGitopsNS := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "openshift-gitops"}} + Eventually(openshiftGitopsNS).Should(namespaceFixture.HaveAnnotation(common.InfraNodeSelectorAnnotation, "key1=value1")) - for _, deploymentName := range deploymentNameList { + By("ensuring the GitOps Service backend Deployment still has nodeSelector on the pod template") + clusterDepl := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: "cluster", Namespace: "openshift-gitops"}} + Eventually(clusterDepl).Should(deploymentFixture.HaveTemplateSpecNodeSelector(map[string]string{ + "kubernetes.io/os": "linux", + "key1": "value1", + })) + + By("ensuring Argo CD workload templates keep the default nodeSelector (namespace annotation schedules them)") + argoCDDeploymentNames := []string{"openshift-gitops-server", "openshift-gitops-repo-server", "openshift-gitops-dex-server", "openshift-gitops-redis"} + for _, deploymentName := range argoCDDeploymentNames { depl := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: deploymentName, Namespace: "openshift-gitops"}} Eventually(depl).Should(deploymentFixture.HaveTemplateSpecNodeSelector(map[string]string{ "kubernetes.io/os": "linux", - "key1": "value1", })) } - Eventually(appControllerSS).Should(statefulsetFixture.HaveTemplateSpecNodeSelector(map[string]string{ "kubernetes.io/os": "linux", - "key1": "value1", })) By("enabling runOnInfra and setting various tolerations on GitOpsService") @@ -93,17 +101,26 @@ var _ = Describe("GitOps Operator Sequential E2E Tests", func() { Value: "reserved"}} }) - By("ensuring the openshift-gitops namespace has the infra node-selector annotation") - openshiftGitopsNS := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "openshift-gitops"}} - Eventually(openshiftGitopsNS).Should(namespaceFixture.HaveAnnotation(common.InfraNodeSelectorAnnotation, common.InfraNodeSelectorAnnotationValue)) + By("ensuring the openshift-gitops namespace annotation merges infra and custom selectors") + Eventually(openshiftGitopsNS).Should(namespaceFixture.HaveAnnotation(common.InfraNodeSelectorAnnotation, + common.NamespaceNodeSelectorAnnotation(true, map[string]string{"key1": "value1"}))) - By("ensuring Deployments and StatefulSets still have the custom nodeSelector and tolerations") + By("ensuring the GitOps Service backend Deployment still has the custom nodeSelector and tolerations") + Eventually(clusterDepl).Should(deploymentFixture.HaveTemplateSpecNodeSelector(map[string]string{ + "kubernetes.io/os": "linux", + "key1": "value1", + })) + Eventually(clusterDepl).Should(deploymentFixture.HaveTolerations([]corev1.Toleration{{ + Effect: "NoSchedule", + Key: "infra", + Value: "reserved", + }})) - for _, deploymentName := range deploymentNameList { + By("ensuring Argo CD workloads get GitopsService tolerations, with node selection via the namespace annotation") + for _, deploymentName := range argoCDDeploymentNames { depl := &appsv1.Deployment{ObjectMeta: metav1.ObjectMeta{Name: deploymentName, Namespace: "openshift-gitops"}} Eventually(depl).Should(deploymentFixture.HaveTemplateSpecNodeSelector(map[string]string{ "kubernetes.io/os": "linux", - "key1": "value1", })) Eventually(depl).Should(deploymentFixture.HaveTolerations([]corev1.Toleration{{ Effect: "NoSchedule", @@ -111,10 +128,8 @@ var _ = Describe("GitOps Operator Sequential E2E Tests", func() { Value: "reserved", }})) } - Eventually(appControllerSS).Should(statefulsetFixture.HaveTemplateSpecNodeSelector(map[string]string{ "kubernetes.io/os": "linux", - "key1": "value1", })) Eventually(appControllerSS).Should(statefulsetFixture.HaveTolerations([]corev1.Toleration{{ Effect: "NoSchedule",