Skip to content

Commit

Permalink
Don't use hardcoded system namespace in MultiClusterService
Browse files Browse the repository at this point in the history
  • Loading branch information
@wahabmk
wahabmk committed Nov 8, 2024
1 parent d39ae84 commit 5684350
Show file tree
Hide file tree
Showing 8 changed files with 48 additions and 48 deletions.
2 changes: 1 addition & 1 deletion api/v1alpha1/multiclusterservice_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ type ServiceStatus struct {
type MultiClusterServiceStatus struct {
// Services contains details for the state of services.
Services []ServiceStatus `json:"services,omitempty"`
// Conditions contains details for the current state of the ManagedCluster
// Conditions contains details for the current state of the MultiClusterService.
Conditions []metav1.Condition `json:"conditions,omitempty"`
// ObservedGeneration is the last observed generation.
ObservedGeneration int64 `json:"observedGeneration,omitempty"`
Expand Down
5 changes: 3 additions & 2 deletions cmd/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,8 @@ func main() {
}

if err = (&controller.MultiClusterServiceReconciler{
Client: mgr.GetClient(),
Client: mgr.GetClient(),
SystemNamespace: currentNamespace,
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "MultiClusterService")
os.Exit(1)
Expand Down Expand Up @@ -331,7 +332,7 @@ func setupWebhooks(mgr ctrl.Manager, currentNamespace string) error {
setupLog.Error(err, "unable to create webhook", "webhook", "ManagedCluster")
return err
}
if err := (&hmcwebhook.MultiClusterServiceValidator{}).SetupWebhookWithManager(mgr); err != nil {
if err := (&hmcwebhook.MultiClusterServiceValidator{SystemNamespace: currentNamespace}).SetupWebhookWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create webhook", "webhook", "MultiClusterService")
return err
}
Expand Down
9 changes: 5 additions & 4 deletions internal/controller/multiclusterservice_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import (
// MultiClusterServiceReconciler reconciles a MultiClusterService object
type MultiClusterServiceReconciler struct {
client.Client
SystemNamespace string
}

// Reconcile reconciles a MultiClusterService object.
Expand Down Expand Up @@ -115,8 +116,8 @@ func (r *MultiClusterServiceReconciler) reconcileUpdate(ctx context.Context, mcs
}

// By using DefaultSystemNamespace we are enforcing that MultiClusterService
// may only use ServiceTemplates that are present in the hmc-system namespace.
opts, err := helmChartOpts(ctx, r.Client, utils.DefaultSystemNamespace, mcs.Spec.Services)
// may only use ServiceTemplates that are present in the system namespace.
opts, err := helmChartOpts(ctx, r.Client, r.SystemNamespace, mcs.Spec.Services)
if err != nil {
return ctrl.Result{}, err
}
Expand Down Expand Up @@ -179,7 +180,7 @@ func helmChartOpts(ctx context.Context, c client.Client, namespace string, servi
// Here we can use the same namespace for all services
// because if the services slice is part of:
// 1. ManagedCluster: Then the referred template must be in its own namespace.
// 2. MultiClusterService: Then the referred template must be in hmc-system namespace.
// 2. MultiClusterService: Then the referred template must be in system namespace.
tmplRef := client.ObjectKey{Name: svc.Template, Namespace: namespace}
if err := c.Get(ctx, tmplRef, tmpl); err != nil {
return nil, fmt.Errorf("failed to get ServiceTemplate %s: %w", tmplRef.String(), err)
Expand Down Expand Up @@ -356,7 +357,7 @@ func requeueSveltosProfileForClusterSummary(ctx context.Context, obj client.Obje

cs, ok := obj.(*sveltosv1beta1.ClusterSummary)
if !ok {
l.Error(errors.New("request is not for a ClusterSummary object"), msg, "ClusterSummary.Name", obj.GetName(), "ClusterSummary.Namespace", obj.GetNamespace())
l.Error(errors.New("request is not for a ClusterSummary object"), msg, "Requested.Name", obj.GetName(), "Requested.Namespace", obj.GetNamespace())
return []ctrl.Request{}
}

Expand Down
28 changes: 13 additions & 15 deletions internal/controller/multiclusterservice_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,11 @@ import (
"sigs.k8s.io/controller-runtime/pkg/reconcile"

hmc "github.com/Mirantis/hmc/api/v1alpha1"
"github.com/Mirantis/hmc/internal/utils"
)

var _ = Describe("MultiClusterService Controller", func() {
Context("When reconciling a resource", func() {
const (
testNamespace = utils.DefaultSystemNamespace
serviceTemplateName = "test-service-0-1-0"
helmRepoName = "test-helmrepo"
helmChartName = "test-helmchart"
Expand Down Expand Up @@ -66,31 +64,31 @@ var _ = Describe("MultiClusterService Controller", func() {
multiClusterService := &hmc.MultiClusterService{}
clusterProfile := &sveltosv1beta1.ClusterProfile{}

helmRepositoryRef := types.NamespacedName{Namespace: testNamespace, Name: helmRepoName}
helmChartRef := types.NamespacedName{Namespace: testNamespace, Name: helmChartName}
serviceTemplateRef := types.NamespacedName{Namespace: testNamespace, Name: serviceTemplateName}
helmRepositoryRef := types.NamespacedName{Namespace: testSystemNamespace, Name: helmRepoName}
helmChartRef := types.NamespacedName{Namespace: testSystemNamespace, Name: helmChartName}
serviceTemplateRef := types.NamespacedName{Namespace: testSystemNamespace, Name: serviceTemplateName}
multiClusterServiceRef := types.NamespacedName{Name: multiClusterServiceName}
clusterProfileRef := types.NamespacedName{Name: multiClusterServiceName}

BeforeEach(func() {
By("creating Namespace")
err := k8sClient.Get(ctx, types.NamespacedName{Name: testNamespace}, namespace)
err := k8sClient.Get(ctx, types.NamespacedName{Name: testSystemNamespace}, namespace)
if err != nil && apierrors.IsNotFound(err) {
namespace = &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: testNamespace,
Name: testSystemNamespace,
},
}
Expect(k8sClient.Create(ctx, namespace)).To(Succeed())
}

By("creating HelmRepository")
err = k8sClient.Get(ctx, types.NamespacedName{Name: helmRepoName, Namespace: testNamespace}, helmRepo)
err = k8sClient.Get(ctx, types.NamespacedName{Name: helmRepoName, Namespace: testSystemNamespace}, helmRepo)
if err != nil && apierrors.IsNotFound(err) {
helmRepo = &sourcev1.HelmRepository{
ObjectMeta: metav1.ObjectMeta{
Name: helmRepoName,
Namespace: testNamespace,
Namespace: testSystemNamespace,
},
Spec: sourcev1.HelmRepositorySpec{
URL: "oci://test/helmrepo",
Expand All @@ -100,12 +98,12 @@ var _ = Describe("MultiClusterService Controller", func() {
}

By("creating HelmChart")
err = k8sClient.Get(ctx, types.NamespacedName{Name: helmChartName, Namespace: testNamespace}, helmChart)
err = k8sClient.Get(ctx, types.NamespacedName{Name: helmChartName, Namespace: testSystemNamespace}, helmChart)
if err != nil && apierrors.IsNotFound(err) {
helmChart = &sourcev1.HelmChart{
ObjectMeta: metav1.ObjectMeta{
Name: helmChartName,
Namespace: testNamespace,
Namespace: testSystemNamespace,
},
Spec: sourcev1.HelmChartSpec{
SourceRef: sourcev1.LocalHelmChartSourceReference{
Expand All @@ -131,7 +129,7 @@ var _ = Describe("MultiClusterService Controller", func() {
serviceTemplate = &hmc.ServiceTemplate{
ObjectMeta: metav1.ObjectMeta{
Name: serviceTemplateName,
Namespace: testNamespace,
Namespace: testSystemNamespace,
Labels: map[string]string{
hmc.HMCManagedLabelKey: "true",
},
Expand All @@ -142,7 +140,7 @@ var _ = Describe("MultiClusterService Controller", func() {
ChartRef: &helmcontrollerv2.CrossNamespaceSourceReference{
Kind: "HelmChart",
Name: helmChartName,
Namespace: testNamespace,
Namespace: testSystemNamespace,
},
},
},
Expand Down Expand Up @@ -195,7 +193,7 @@ var _ = Describe("MultiClusterService Controller", func() {
multiClusterServiceResource := &hmc.MultiClusterService{}
Expect(k8sClient.Get(ctx, multiClusterServiceRef, multiClusterServiceResource)).NotTo(HaveOccurred())

reconciler := &MultiClusterServiceReconciler{Client: k8sClient}
reconciler := &MultiClusterServiceReconciler{Client: k8sClient, SystemNamespace: testSystemNamespace}
Expect(k8sClient.Delete(ctx, multiClusterService)).To(Succeed())
// Running reconcile to remove the finalizer and delete the MultiClusterService
_, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: multiClusterServiceRef})
Expand All @@ -219,7 +217,7 @@ var _ = Describe("MultiClusterService Controller", func() {

It("should successfully reconcile the resource", func() {
By("reconciling MultiClusterService")
multiClusterServiceReconciler := &MultiClusterServiceReconciler{Client: k8sClient}
multiClusterServiceReconciler := &MultiClusterServiceReconciler{Client: k8sClient, SystemNamespace: testSystemNamespace}

_, err := multiClusterServiceReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: multiClusterServiceRef})
Expect(err).NotTo(HaveOccurred())
Expand Down
3 changes: 2 additions & 1 deletion internal/controller/suite_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import (
const (
mutatingWebhookKind = "MutatingWebhookConfiguration"
validatingWebhookKind = "ValidatingWebhookConfiguration"
testSystemNamespace = "test-system-namespace"
)

var (
Expand Down Expand Up @@ -150,7 +151,7 @@ var _ = BeforeSuite(func() {
err = (&hmcwebhook.ManagedClusterValidator{}).SetupWebhookWithManager(mgr)
Expect(err).NotTo(HaveOccurred())

err = (&hmcwebhook.MultiClusterServiceValidator{}).SetupWebhookWithManager(mgr)
err = (&hmcwebhook.MultiClusterServiceValidator{SystemNamespace: testSystemNamespace}).SetupWebhookWithManager(mgr)
Expect(err).NotTo(HaveOccurred())

err = (&hmcwebhook.ManagementValidator{}).SetupWebhookWithManager(mgr)
Expand Down
18 changes: 9 additions & 9 deletions internal/webhook/multiclusterservice_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package webhook

import (
"context"
"errors"
"fmt"

apierrors "k8s.io/apimachinery/pkg/api/errors"
Expand All @@ -26,11 +27,11 @@ import (
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"

"github.com/Mirantis/hmc/api/v1alpha1"
"github.com/Mirantis/hmc/internal/utils"
)

type MultiClusterServiceValidator struct {
client.Client
SystemNamespace string
}

const invalidMultiClusterServiceMsg = "the MultiClusterService is invalid"
Expand Down Expand Up @@ -62,7 +63,7 @@ func (v *MultiClusterServiceValidator) ValidateCreate(ctx context.Context, obj r
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected MultiClusterService but got a %T", obj))
}

if err := validateServices(ctx, v.Client, utils.DefaultSystemNamespace, mcs.Spec.Services); err != nil {
if err := validateServices(ctx, v.Client, v.SystemNamespace, mcs.Spec.Services); err != nil {
return nil, fmt.Errorf("%s: %w", invalidMultiClusterServiceMsg, err)
}

Expand All @@ -76,7 +77,7 @@ func (v *MultiClusterServiceValidator) ValidateUpdate(ctx context.Context, _, ne
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected MultiClusterService but got a %T", newObj))
}

if err := validateServices(ctx, v.Client, utils.DefaultSystemNamespace, mcs.Spec.Services); err != nil {
if err := validateServices(ctx, v.Client, v.SystemNamespace, mcs.Spec.Services); err != nil {
return nil, fmt.Errorf("%s: %w", invalidMultiClusterServiceMsg, err)
}

Expand All @@ -93,17 +94,16 @@ func getServiceTemplate(ctx context.Context, c client.Client, templateNamespace,
return tpl, c.Get(ctx, client.ObjectKey{Namespace: templateNamespace, Name: templateName}, tpl)
}

func validateServices(ctx context.Context, c client.Client, namespace string, services []v1alpha1.ServiceSpec) error {
func validateServices(ctx context.Context, c client.Client, namespace string, services []v1alpha1.ServiceSpec) (errs error) {
for _, svc := range services {
tpl, err := getServiceTemplate(ctx, c, namespace, svc.Template)
if err != nil {
return err
errs = errors.Join(errs, err)
continue
}

if err := isTemplateValid(tpl.GetCommonStatus()); err != nil {
return err
}
errs = errors.Join(errs, isTemplateValid(tpl.GetCommonStatus()))
}

return nil
return errs
}
29 changes: 14 additions & 15 deletions internal/webhook/multiclusterservice_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"

"github.com/Mirantis/hmc/api/v1alpha1"
"github.com/Mirantis/hmc/internal/utils"
"github.com/Mirantis/hmc/test/objects/multiclusterservice"
"github.com/Mirantis/hmc/test/objects/template"
"github.com/Mirantis/hmc/test/scheme"
Expand All @@ -36,6 +35,7 @@ const (
testMCSName = "testmcs"
testSvcTemplate1Name = "test-servicetemplate-1"
testSvcTemplate2Name = "test-servicetemplate-2"
testSystemNamespace = "test-system-namespace"
)

func TestMultiClusterServiceValidateCreate(t *testing.T) {
Expand All @@ -53,7 +53,7 @@ func TestMultiClusterServiceValidateCreate(t *testing.T) {
warnings admission.Warnings
}{
{
name: "should fail if the ServiceTemplates are not found in hmc-system namespace",
name: "should fail if the ServiceTemplates are not found in system namespace",
mcs: multiclusterservice.NewMultiClusterService(
multiclusterservice.WithName(testMCSName),
multiclusterservice.WithServiceTemplate(testSvcTemplate1Name),
Expand All @@ -76,7 +76,7 @@ func TestMultiClusterServiceValidateCreate(t *testing.T) {
existingObjects: []runtime.Object{
template.NewServiceTemplate(
template.WithName(testSvcTemplate1Name),
template.WithNamespace(utils.DefaultSystemNamespace),
template.WithNamespace(testSystemNamespace),
template.WithValidationStatus(v1alpha1.TemplateValidationStatus{
Valid: false,
ValidationError: "validation error example",
Expand All @@ -94,7 +94,7 @@ func TestMultiClusterServiceValidateCreate(t *testing.T) {
existingObjects: []runtime.Object{
template.NewServiceTemplate(
template.WithName(testSvcTemplate1Name),
template.WithNamespace(utils.DefaultSystemNamespace),
template.WithNamespace(testSystemNamespace),
template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}),
),
},
Expand All @@ -109,12 +109,12 @@ func TestMultiClusterServiceValidateCreate(t *testing.T) {
existingObjects: []runtime.Object{
template.NewServiceTemplate(
template.WithName(testSvcTemplate1Name),
template.WithNamespace(utils.DefaultSystemNamespace),
template.WithNamespace(testSystemNamespace),
template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}),
),
template.NewServiceTemplate(
template.WithName(testSvcTemplate2Name),
template.WithNamespace(utils.DefaultSystemNamespace),
template.WithNamespace(testSystemNamespace),
template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}),
),
},
Expand All @@ -130,9 +130,8 @@ func TestMultiClusterServiceValidateCreate(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

c := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(tt.existingObjects...).Build()
validator := &MultiClusterServiceValidator{Client: c}
validator := &MultiClusterServiceValidator{Client: c, SystemNamespace: testSystemNamespace}
warn, err := validator.ValidateCreate(ctx, tt.mcs)
if tt.err != "" {
g.Expect(err).To(MatchError(tt.err))
Expand Down Expand Up @@ -169,7 +168,7 @@ func TestMultiClusterServiceValidateUpdate(t *testing.T) {
warnings admission.Warnings
}{
{
name: "should fail if the ServiceTemplates are not found in hmc-system namespace",
name: "should fail if the ServiceTemplates are not found in system namespace",
newMCS: multiclusterservice.NewMultiClusterService(
multiclusterservice.WithName(testMCSName),
multiclusterservice.WithServiceTemplate(testSvcTemplate1Name),
Expand All @@ -178,7 +177,7 @@ func TestMultiClusterServiceValidateUpdate(t *testing.T) {
existingObjects: []runtime.Object{
template.NewServiceTemplate(
template.WithName(testSvcTemplate1Name),
template.WithNamespace(utils.DefaultSystemNamespace),
template.WithNamespace(testSystemNamespace),
template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}),
),
template.NewServiceTemplate(
Expand All @@ -199,12 +198,12 @@ func TestMultiClusterServiceValidateUpdate(t *testing.T) {
existingObjects: []runtime.Object{
template.NewServiceTemplate(
template.WithName(testSvcTemplate1Name),
template.WithNamespace(utils.DefaultSystemNamespace),
template.WithNamespace(testSystemNamespace),
template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}),
),
template.NewServiceTemplate(
template.WithName(testSvcTemplate2Name),
template.WithNamespace(utils.DefaultSystemNamespace),
template.WithNamespace(testSystemNamespace),
template.WithValidationStatus(v1alpha1.TemplateValidationStatus{
Valid: false,
ValidationError: "validation error example",
Expand All @@ -223,12 +222,12 @@ func TestMultiClusterServiceValidateUpdate(t *testing.T) {
existingObjects: []runtime.Object{
template.NewServiceTemplate(
template.WithName(testSvcTemplate1Name),
template.WithNamespace(utils.DefaultSystemNamespace),
template.WithNamespace(testSystemNamespace),
template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}),
),
template.NewServiceTemplate(
template.WithName(testSvcTemplate2Name),
template.WithNamespace(utils.DefaultSystemNamespace),
template.WithNamespace(testSystemNamespace),
template.WithValidationStatus(v1alpha1.TemplateValidationStatus{Valid: true}),
),
},
Expand All @@ -246,7 +245,7 @@ func TestMultiClusterServiceValidateUpdate(t *testing.T) {
g := NewWithT(t)

c := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(tt.existingObjects...).Build()
validator := &MultiClusterServiceValidator{Client: c}
validator := &MultiClusterServiceValidator{Client: c, SystemNamespace: testSystemNamespace}
warn, err := validator.ValidateUpdate(ctx, oldMCS, tt.newMCS)
if tt.err != "" {
g.Expect(err).To(MatchError(tt.err))
Expand Down
2 changes: 1 addition & 1 deletion templates/provider/hmc/templates/crds/hmc.mirantis.com_multiclusterservices.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ spec:
properties:
conditions:
description: Conditions contains details for the current state of
the ManagedCluster
the MultiClusterService.
items:
description: Condition contains details for one aspect of the current
state of this API Resource.
Expand Down

0 comments on commit 5684350

Please sign in to comment.