diff --git a/pkg/addon/controllers/addontemplate/controller.go b/pkg/addon/controllers/addontemplate/controller.go index 9d9968c28..8c057b5d9 100644 --- a/pkg/addon/controllers/addontemplate/controller.go +++ b/pkg/addon/controllers/addontemplate/controller.go @@ -18,6 +18,7 @@ import ( "open-cluster-management.io/addon-framework/pkg/addonfactory" "open-cluster-management.io/addon-framework/pkg/addonmanager" + "open-cluster-management.io/addon-framework/pkg/utils" addonapiv1alpha1 "open-cluster-management.io/api/addon/v1alpha1" addonv1alpha1 "open-cluster-management.io/api/addon/v1alpha1" addonv1alpha1client "open-cluster-management.io/api/client/addon/clientset/versioned" @@ -194,10 +195,11 @@ func (c *addonTemplateController) runController( // image overrides from cluster annotation has lower priority than from the addonDeploymentConfig getValuesClosure, addonfactory.GetAddOnDeploymentConfigValues( - addonfactory.NewAddOnDeploymentConfigGetter(c.addonClient), + utils.NewAddOnDeploymentConfigGetter(c.addonClient), addonfactory.ToAddOnCustomizedVariableValues, templateagent.ToAddOnNodePlacementPrivateValues, templateagent.ToAddOnRegistriesPrivateValues, + templateagent.ToAddOnInstallNamespacePrivateValues, ), ) err = mgr.AddAgent(agentAddon) diff --git a/pkg/addon/templateagent/decorator.go b/pkg/addon/templateagent/decorator.go index b23f8b38c..d1f20119f 100644 --- a/pkg/addon/templateagent/decorator.go +++ b/pkg/addon/templateagent/decorator.go @@ -4,28 +4,146 @@ import ( "fmt" "strings" - appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" "open-cluster-management.io/addon-framework/pkg/addonfactory" + "open-cluster-management.io/addon-framework/pkg/utils" addonapiv1alpha1 "open-cluster-management.io/api/addon/v1alpha1" ) -type deploymentDecorator interface { +// decorator mutate the unstructured and returns an unstructured. +type decorator interface { + decorate(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) +} + +type namespaceDecorator struct { + installNamespace string + // paths is the paths of a resource kind that the decorator needs to set namespace field in it. + // if the returned object is a list, decorator will set namespace for each item. + paths map[string]string +} + +func newNamespaceDecorator(privateValues addonfactory.Values) *namespaceDecorator { + decorator := &namespaceDecorator{ + paths: map[string]string{ + "ClusterRoleBinding": "subjects", + "RoleBinding": "subjects", + }, + } + namespace, ok := privateValues[InstallNamespacePrivateValueKey] + if ok { + decorator.installNamespace = namespace.(string) + } + + return decorator +} + +func (d *namespaceDecorator) decorate(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) { + if len(d.installNamespace) == 0 { + return obj, nil + } + + // set namespace for all manifests, if the manifest is cluster scoped, namespace will be ignored when + // being applied. + obj.SetNamespace(d.installNamespace) + + path, ok := d.paths[obj.GetKind()] + if !ok { + return obj, nil + } + + field, found, err := unstructured.NestedFieldNoCopy(obj.Object, path) + if err != nil { + return obj, err + } + if !found { + return obj, fmt.Errorf("failed to find the path %s for kind %s", path, obj.GetKind()) + } + + // it cannot supported nested structure, only list or map. + switch f := field.(type) { + case []interface{}: + for _, item := range f { + if err := setNamespaceForObject(item, d.installNamespace); err != nil { + return obj, err + } + } + case interface{}: + if err := setNamespaceForObject(f, d.installNamespace); err != nil { + return obj, err + } + } + return obj, nil +} + +func setNamespaceForObject(obj interface{}, namespace string) error { + mapVal, ok := obj.(map[string]interface{}) + if !ok { + return fmt.Errorf("obj %v is not a map, cannot set", obj) + } + mapVal["namespace"] = namespace + return nil +} + +type deploymentDecorator struct { + decorators []podTemplateSpecDecorator +} + +func newDeploymentDecorator( + addonName string, + template *addonapiv1alpha1.AddOnTemplate, + orderedValues orderedValues, + privateValues addonfactory.Values, +) decorator { + return &deploymentDecorator{ + decorators: []podTemplateSpecDecorator{ + newEnvironmentDecorator(orderedValues), + newVolumeDecorator(addonName, template), + newNodePlacementDecorator(privateValues), + newImageDecorator(privateValues), + }, + } +} + +func (d *deploymentDecorator) decorate(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) { + deployment, err := utils.ConvertToDeployment(obj) + // not a deployment, directly return + if err != nil { + return obj, nil + } + + for _, decorator := range d.decorators { + err = decorator.decorate(&deployment.Spec.Template) + if err != nil { + return obj, err + } + } + + result, err := runtime.DefaultUnstructuredConverter.ToUnstructured(deployment) + if err != nil { + return obj, err + } + + return &unstructured.Unstructured{Object: result}, nil +} + +type podTemplateSpecDecorator interface { // decorate modifies the deployment in place - decorate(deployment *appsv1.Deployment) error + decorate(pod *corev1.PodTemplateSpec) error } type environmentDecorator struct { orderedValues orderedValues } -func newEnvironmentDecorator(orderedValues orderedValues) deploymentDecorator { +func newEnvironmentDecorator(orderedValues orderedValues) podTemplateSpecDecorator { return &environmentDecorator{ orderedValues: orderedValues, } } -func (d *environmentDecorator) decorate(deployment *appsv1.Deployment) error { +func (d *environmentDecorator) decorate(pod *corev1.PodTemplateSpec) error { envVars := make([]corev1.EnvVar, len(d.orderedValues)) for index, value := range d.orderedValues { envVars[index] = corev1.EnvVar{ @@ -34,9 +152,9 @@ func (d *environmentDecorator) decorate(deployment *appsv1.Deployment) error { } } - for j := range deployment.Spec.Template.Spec.Containers { - deployment.Spec.Template.Spec.Containers[j].Env = append( - deployment.Spec.Template.Spec.Containers[j].Env, + for j := range pod.Spec.Containers { + pod.Spec.Containers[j].Env = append( + pod.Spec.Containers[j].Env, envVars...) } @@ -48,14 +166,14 @@ type volumeDecorator struct { addonName string } -func newVolumeDecorator(addonName string, template *addonapiv1alpha1.AddOnTemplate) deploymentDecorator { +func newVolumeDecorator(addonName string, template *addonapiv1alpha1.AddOnTemplate) podTemplateSpecDecorator { return &volumeDecorator{ addonName: addonName, template: template, } } -func (d *volumeDecorator) decorate(deployment *appsv1.Deployment) error { +func (d *volumeDecorator) decorate(pod *corev1.PodTemplateSpec) error { volumeMounts := []corev1.VolumeMount{} volumes := []corev1.Volume{} @@ -102,12 +220,12 @@ func (d *volumeDecorator) decorate(deployment *appsv1.Deployment) error { return nil } - for j := range deployment.Spec.Template.Spec.Containers { - deployment.Spec.Template.Spec.Containers[j].VolumeMounts = append( - deployment.Spec.Template.Spec.Containers[j].VolumeMounts, volumeMounts...) + for j := range pod.Spec.Containers { + pod.Spec.Containers[j].VolumeMounts = append( + pod.Spec.Containers[j].VolumeMounts, volumeMounts...) } - deployment.Spec.Template.Spec.Volumes = append(deployment.Spec.Template.Spec.Volumes, volumes...) + pod.Spec.Volumes = append(pod.Spec.Volumes, volumes...) return nil } @@ -116,13 +234,13 @@ type nodePlacementDecorator struct { privateValues addonfactory.Values } -func newNodePlacementDecorator(privateValues addonfactory.Values) deploymentDecorator { +func newNodePlacementDecorator(privateValues addonfactory.Values) podTemplateSpecDecorator { return &nodePlacementDecorator{ privateValues: privateValues, } } -func (d *nodePlacementDecorator) decorate(deployment *appsv1.Deployment) error { +func (d *nodePlacementDecorator) decorate(pod *corev1.PodTemplateSpec) error { nodePlacement, ok := d.privateValues[NodePlacementPrivateValueKey] if !ok { return nil @@ -134,11 +252,11 @@ func (d *nodePlacementDecorator) decorate(deployment *appsv1.Deployment) error { } if np.NodeSelector != nil { - deployment.Spec.Template.Spec.NodeSelector = np.NodeSelector + pod.Spec.NodeSelector = np.NodeSelector } if np.NodeSelector != nil { - deployment.Spec.Template.Spec.Tolerations = np.Tolerations + pod.Spec.Tolerations = np.Tolerations } return nil @@ -148,13 +266,13 @@ type imageDecorator struct { privateValues addonfactory.Values } -func newImageDecorator(privateValues addonfactory.Values) deploymentDecorator { +func newImageDecorator(privateValues addonfactory.Values) podTemplateSpecDecorator { return &imageDecorator{ privateValues: privateValues, } } -func (d *imageDecorator) decorate(deployment *appsv1.Deployment) error { +func (d *imageDecorator) decorate(pod *corev1.PodTemplateSpec) error { registries, ok := d.privateValues[RegistriesPrivateValueKey] if !ok { return nil @@ -165,9 +283,9 @@ func (d *imageDecorator) decorate(deployment *appsv1.Deployment) error { return fmt.Errorf("registries value is invalid") } - for i := range deployment.Spec.Template.Spec.Containers { - deployment.Spec.Template.Spec.Containers[i].Image = addonfactory.OverrideImage( - ims, deployment.Spec.Template.Spec.Containers[i].Image) + for i := range pod.Spec.Containers { + pod.Spec.Containers[i].Image = addonfactory.OverrideImage( + ims, pod.Spec.Containers[i].Image) } return nil diff --git a/pkg/addon/templateagent/decorator_test.go b/pkg/addon/templateagent/decorator_test.go new file mode 100644 index 000000000..4152cf23e --- /dev/null +++ b/pkg/addon/templateagent/decorator_test.go @@ -0,0 +1,134 @@ +package templateagent + +import ( + "testing" + + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + + "open-cluster-management.io/addon-framework/pkg/addonfactory" + + testingcommon "open-cluster-management.io/ocm/pkg/common/testing" +) + +func TestNamespaceDecorator(t *testing.T) { + tests := []struct { + name string + namespace string + object *unstructured.Unstructured + validateObject func(t *testing.T, obj *unstructured.Unstructured) + }{ + { + name: "no namespace set", + object: testingcommon.NewUnstructured("v1", "Pod", "default", "test"), + validateObject: func(t *testing.T, obj *unstructured.Unstructured) { + testingcommon.AssertEqualNameNamespace(t, obj.GetName(), obj.GetNamespace(), "test", "default") + }, + }, + { + name: "namespace set", + object: testingcommon.NewUnstructured("v1", "Pod", "default", "test"), + namespace: "newns", + validateObject: func(t *testing.T, obj *unstructured.Unstructured) { + testingcommon.AssertEqualNameNamespace(t, obj.GetName(), obj.GetNamespace(), "test", "newns") + }, + }, + { + name: "clusterRoleBinding", + object: func() *unstructured.Unstructured { + clusterRoleBinding := &rbacv1.ClusterRoleBinding{ + TypeMeta: metav1.TypeMeta{ + Kind: "ClusterRoleBinding", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + Subjects: []rbacv1.Subject{ + { + Name: "user1", + Namespace: "default", + }, + { + Name: "user2", + Namespace: "default", + }, + }, + } + data, _ := runtime.DefaultUnstructuredConverter.ToUnstructured(clusterRoleBinding) + return &unstructured.Unstructured{Object: data} + }(), + namespace: "newns", + validateObject: func(t *testing.T, obj *unstructured.Unstructured) { + binding := &rbacv1.ClusterRoleBinding{} + err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.Object, binding) + if err != nil { + t.Fatal(err) + } + for _, s := range binding.Subjects { + if s.Namespace != "newns" { + t.Errorf("namespace of subject is not correct, got %v", s) + } + } + }, + }, + { + name: "roleBinding", + object: func() *unstructured.Unstructured { + roleBinding := &rbacv1.RoleBinding{ + TypeMeta: metav1.TypeMeta{ + Kind: "RoleBinding", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + }, + Subjects: []rbacv1.Subject{ + { + Name: "user1", + Namespace: "default", + }, + { + Name: "user2", + Namespace: "default", + }, + }, + } + data, _ := runtime.DefaultUnstructuredConverter.ToUnstructured(roleBinding) + return &unstructured.Unstructured{Object: data} + }(), + namespace: "newns", + validateObject: func(t *testing.T, obj *unstructured.Unstructured) { + testingcommon.AssertEqualNameNamespace(t, obj.GetName(), obj.GetNamespace(), "test", "newns") + binding := &rbacv1.RoleBinding{} + err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.Object, binding) + if err != nil { + t.Fatal(err) + } + for _, s := range binding.Subjects { + if s.Namespace != "newns" { + t.Errorf("namespace of subject is not correct, got %v", s) + } + } + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + values := addonfactory.Values{} + if len(tc.namespace) > 0 { + values[InstallNamespacePrivateValueKey] = tc.namespace + } + + d := newNamespaceDecorator(values) + result, err := d.decorate(tc.object) + if err != nil { + t.Fatal(err) + } + tc.validateObject(t, result) + }) + } + +} diff --git a/pkg/addon/templateagent/template_agent.go b/pkg/addon/templateagent/template_agent.go index 6f19a0a44..aacab86df 100644 --- a/pkg/addon/templateagent/template_agent.go +++ b/pkg/addon/templateagent/template_agent.go @@ -24,8 +24,9 @@ import ( ) const ( - NodePlacementPrivateValueKey = "__NODE_PLACEMENT" - RegistriesPrivateValueKey = "__REGISTRIES" + NodePlacementPrivateValueKey = "__NODE_PLACEMENT" + RegistriesPrivateValueKey = "__REGISTRIES" + InstallNamespacePrivateValueKey = "__INSTALL_NAMESPACE" ) // templateBuiltinValues includes the built-in values for crd template agentAddon. @@ -33,8 +34,7 @@ const ( // to convert it to Values by JsonStructToValues. // the built-in values can not be overridden by getValuesFuncs type templateCRDBuiltinValues struct { - ClusterName string `json:"CLUSTER_NAME,omitempty"` - AddonInstallNamespace string `json:"INSTALL_NAMESPACE,omitempty"` + ClusterName string `json:"CLUSTER_NAME,omitempty"` } // templateDefaultValues includes the default values for crd template agentAddon. @@ -170,43 +170,35 @@ func (a *CRDTemplateAgentAddon) renderObjects( if err := object.UnmarshalJSON([]byte(manifestStr)); err != nil { return objects, err } - objects = append(objects, object) - } - objects, err = a.decorateObjects(template, objects, presetValues, configValues, privateValues) - if err != nil { - return objects, err + object, err = a.decorateObject(template, object, presetValues, privateValues) + if err != nil { + return objects, err + } + objects = append(objects, object) } return objects, nil } -func (a *CRDTemplateAgentAddon) decorateObjects( +func (a *CRDTemplateAgentAddon) decorateObject( template *addonapiv1alpha1.AddOnTemplate, - objects []runtime.Object, + obj *unstructured.Unstructured, orderedValues orderedValues, - configValues, privateValues addonfactory.Values) ([]runtime.Object, error) { - decorators := []deploymentDecorator{ - newEnvironmentDecorator(orderedValues), - newVolumeDecorator(a.addonName, template), - newNodePlacementDecorator(privateValues), - newImageDecorator(privateValues), + privateValues addonfactory.Values) (*unstructured.Unstructured, error) { + decorators := []decorator{ + newDeploymentDecorator(a.addonName, template, orderedValues, privateValues), + newNamespaceDecorator(privateValues), } - for index, obj := range objects { - deployment, err := utils.ConvertToDeployment(obj) - if err != nil { - continue - } - for _, decorator := range decorators { - err = decorator.decorate(deployment) - if err != nil { - return objects, err - } + var err error + for _, decorator := range decorators { + obj, err = decorator.decorate(obj) + if err != nil { + return obj, err } - objects[index] = deployment } - return objects, nil + return obj, nil } // getDesiredAddOnTemplateInner returns the desired template of the addon diff --git a/pkg/addon/templateagent/template_agent_test.go b/pkg/addon/templateagent/template_agent_test.go index a9e93742f..dca566220 100644 --- a/pkg/addon/templateagent/template_agent_test.go +++ b/pkg/addon/templateagent/template_agent_test.go @@ -7,10 +7,11 @@ import ( "time" "github.com/stretchr/testify/assert" - appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" kubeinformers "k8s.io/client-go/informers" @@ -145,15 +146,23 @@ func TestAddonTemplateAgentManifests(t *testing.T) { t.Errorf("expected 4 objects, but got %v", len(objects)) } - object, ok := objects[0].(*appsv1.Deployment) + unstructObject, ok := objects[0].(*unstructured.Unstructured) if !ok { t.Errorf("expected object to be *appsv1.Deployment, but got %T", objects[0]) } + object, err := utils.ConvertToDeployment(unstructObject) + if err != nil { + t.Fatal(err) + } image := object.Spec.Template.Spec.Containers[0].Image if image != "quay.io/ocm/addon-examples:v1" { t.Errorf("unexpected image %v", image) } + if object.Namespace != "test-install-namespace" { + t.Errorf("unexpected namespace %s", object.Namespace) + } + nodeSelector := object.Spec.Template.Spec.NodeSelector expectedNodeSelector := map[string]string{"host": "ssd"} if !equality.Semantic.DeepEqual(nodeSelector, expectedNodeSelector) { @@ -171,7 +180,6 @@ func TestAddonTemplateAgentManifests(t *testing.T) { {Name: "LOG_LEVEL", Value: "4"}, {Name: "HUB_KUBECONFIG", Value: "/managed/hub-kubeconfig/kubeconfig"}, {Name: "CLUSTER_NAME", Value: clusterName}, - {Name: "INSTALL_NAMESPACE", Value: "open-cluster-management-agent-addon"}, } if !equality.Semantic.DeepEqual(envs, expectedEnvs) { t.Errorf("unexpected envs %v", envs) @@ -215,64 +223,81 @@ func TestAddonTemplateAgentManifests(t *testing.T) { if !equality.Semantic.DeepEqual(volumeMounts, expectedVolumeMounts) { t.Errorf("expected volumeMounts %v, but got: %v", expectedVolumeMounts, volumeMounts) } + + // check clusterrole + unstructObject, ok = objects[2].(*unstructured.Unstructured) + if !ok { + t.Errorf("expected object to be unstructured, but got %T", objects[0]) + } + clusterRoleBinding := &rbacv1.ClusterRoleBinding{} + err = runtime.DefaultUnstructuredConverter.FromUnstructured(unstructObject.Object, clusterRoleBinding) + if err != nil { + t.Fatal(err) + } + if clusterRoleBinding.Subjects[0].Namespace != "test-install-namespace" { + t.Errorf("clusterRolebinding namespace does not match, got %s", clusterRoleBinding.Subjects[0].Namespace) + } }, }, } for _, tc := range cases { - var managedClusterAddon *addonapiv1alpha1.ManagedClusterAddOn - var objs []runtime.Object - if tc.managedClusterAddonBuilder != nil { - if tc.addonTemplate != nil { - tc.managedClusterAddonBuilder.withAddonTemplate(tc.addonTemplate) - objs = append(objs, tc.addonTemplate) - } - if tc.addonDeploymentConfig != nil { - tc.managedClusterAddonBuilder.withAddonDeploymentConfig(tc.addonDeploymentConfig) - objs = append(objs, tc.addonDeploymentConfig) + t.Run(tc.name, func(t *testing.T) { + var managedClusterAddon *addonapiv1alpha1.ManagedClusterAddOn + var objs []runtime.Object + if tc.managedClusterAddonBuilder != nil { + if tc.addonTemplate != nil { + tc.managedClusterAddonBuilder.withAddonTemplate(tc.addonTemplate) + objs = append(objs, tc.addonTemplate) + } + if tc.addonDeploymentConfig != nil { + tc.managedClusterAddonBuilder.withAddonDeploymentConfig(tc.addonDeploymentConfig) + objs = append(objs, tc.addonDeploymentConfig) + } + managedClusterAddon = tc.managedClusterAddonBuilder.build() + objs = append(objs, managedClusterAddon) } - managedClusterAddon = tc.managedClusterAddonBuilder.build() - objs = append(objs, managedClusterAddon) - } - hubKubeClient := fakekube.NewSimpleClientset() - addonClient := fakeaddon.NewSimpleClientset(objs...) + hubKubeClient := fakekube.NewSimpleClientset() + addonClient := fakeaddon.NewSimpleClientset(objs...) - addonInformerFactory := addoninformers.NewSharedInformerFactory(addonClient, 30*time.Minute) - if managedClusterAddon != nil { - mcaStore := addonInformerFactory.Addon().V1alpha1().ManagedClusterAddOns().Informer().GetStore() - if err := mcaStore.Add(managedClusterAddon); err != nil { - t.Fatal(err) + addonInformerFactory := addoninformers.NewSharedInformerFactory(addonClient, 30*time.Minute) + if managedClusterAddon != nil { + mcaStore := addonInformerFactory.Addon().V1alpha1().ManagedClusterAddOns().Informer().GetStore() + if err := mcaStore.Add(managedClusterAddon); err != nil { + t.Fatal(err) + } } - } - if tc.addonTemplate != nil { - atStore := addonInformerFactory.Addon().V1alpha1().AddOnTemplates().Informer().GetStore() - if err := atStore.Add(tc.addonTemplate); err != nil { - t.Fatal(err) + if tc.addonTemplate != nil { + atStore := addonInformerFactory.Addon().V1alpha1().AddOnTemplates().Informer().GetStore() + if err := atStore.Add(tc.addonTemplate); err != nil { + t.Fatal(err) + } } - } - kubeInformers := kubeinformers.NewSharedInformerFactoryWithOptions(hubKubeClient, 10*time.Minute) + kubeInformers := kubeinformers.NewSharedInformerFactoryWithOptions(hubKubeClient, 10*time.Minute) - agentAddon := NewCRDTemplateAgentAddon( - ctx, - addonName, - hubKubeClient, - addonClient, - addonInformerFactory, - kubeInformers.Rbac().V1().RoleBindings().Lister(), - addonfactory.GetAddOnDeploymentConfigValues( - utils.NewAddOnDeploymentConfigGetter(addonClient), - addonfactory.ToAddOnCustomizedVariableValues, - ToAddOnNodePlacementPrivateValues, - ToAddOnRegistriesPrivateValues, - ), - ) + agentAddon := NewCRDTemplateAgentAddon( + ctx, + addonName, + hubKubeClient, + addonClient, + addonInformerFactory, + kubeInformers.Rbac().V1().RoleBindings().Lister(), + addonfactory.GetAddOnDeploymentConfigValues( + utils.NewAddOnDeploymentConfigGetter(addonClient), + addonfactory.ToAddOnCustomizedVariableValues, + ToAddOnNodePlacementPrivateValues, + ToAddOnRegistriesPrivateValues, + ToAddOnInstallNamespacePrivateValues, + ), + ) - objects, err := agentAddon.Manifests(tc.managedCluster, managedClusterAddon) - if err != nil { - assert.Equal(t, tc.expectedErr, err.Error(), tc.name) - } else { - tc.validateObjects(t, objects) - } + objects, err := agentAddon.Manifests(tc.managedCluster, managedClusterAddon) + if err != nil { + assert.Equal(t, tc.expectedErr, err.Error(), tc.name) + } else { + tc.validateObjects(t, objects) + } + }) } } diff --git a/pkg/addon/templateagent/values.go b/pkg/addon/templateagent/values.go index 3465631b3..9c8bdb2d5 100644 --- a/pkg/addon/templateagent/values.go +++ b/pkg/addon/templateagent/values.go @@ -36,6 +36,15 @@ func ToAddOnRegistriesPrivateValues(config addonapiv1alpha1.AddOnDeploymentConfi }, nil } +func ToAddOnInstallNamespacePrivateValues(config addonapiv1alpha1.AddOnDeploymentConfig) (addonfactory.Values, error) { + if len(config.Spec.AgentInstallNamespace) == 0 { + return nil, nil + } + return addonfactory.Values{ + InstallNamespacePrivateValueKey: config.Spec.AgentInstallNamespace, + }, nil +} + type keyValuePair struct { name string value string @@ -57,11 +66,12 @@ func (a *CRDTemplateAgentAddon) getValues( if err != nil { return presetValues, overrideValues, privateValues, err } - overrideValues = addonfactory.MergeValues(overrideValues, defaultValues) + overrideValues = addonfactory.MergeValues(defaultValues, overrideValues) privateValuesKeys := map[string]struct{}{ - NodePlacementPrivateValueKey: {}, - RegistriesPrivateValueKey: {}, + NodePlacementPrivateValueKey: {}, + RegistriesPrivateValueKey: {}, + InstallNamespacePrivateValueKey: {}, } for i := 0; i < len(a.getValuesFuncs); i++ { @@ -87,6 +97,8 @@ func (a *CRDTemplateAgentAddon) getValues( if err != nil { return presetValues, overrideValues, privateValues, nil } + // builtinValues only contains CLUSTER_NAME, and it should override overrideValues if CLUSTER_NAME + // is also set in the overrideValues, since CLUSTER_NAME should not be set externally. overrideValues = addonfactory.MergeValues(overrideValues, builtinValues) for k, v := range overrideValues { @@ -109,16 +121,10 @@ func (a *CRDTemplateAgentAddon) getValues( func (a *CRDTemplateAgentAddon) getBuiltinValues( cluster *clusterv1.ManagedCluster, - addon *addonapiv1alpha1.ManagedClusterAddOn) ([]string, addonfactory.Values, error) { + _ *addonapiv1alpha1.ManagedClusterAddOn) ([]string, addonfactory.Values, error) { builtinValues := templateCRDBuiltinValues{} builtinValues.ClusterName = cluster.GetName() - installNamespace := addon.Spec.InstallNamespace - if len(installNamespace) == 0 { - installNamespace = addonfactory.AddonDefaultInstallNamespace - } - builtinValues.AddonInstallNamespace = installNamespace - value, err := addonfactory.JsonStructToValues(builtinValues) if err != nil { return nil, nil, err @@ -127,8 +133,8 @@ func (a *CRDTemplateAgentAddon) getBuiltinValues( } func (a *CRDTemplateAgentAddon) getDefaultValues( - cluster *clusterv1.ManagedCluster, - addon *addonapiv1alpha1.ManagedClusterAddOn, + _ *clusterv1.ManagedCluster, + _ *addonapiv1alpha1.ManagedClusterAddOn, template *addonapiv1alpha1.AddOnTemplate) ([]string, addonfactory.Values, error) { defaultValues := templateCRDDefaultValues{} @@ -161,7 +167,7 @@ func hubKubeconfigPath() string { func GetAddOnRegistriesPrivateValuesFromClusterAnnotation( logger klog.Logger, cluster *clusterv1.ManagedCluster, - addon *addonapiv1alpha1.ManagedClusterAddOn) (addonfactory.Values, error) { + _ *addonapiv1alpha1.ManagedClusterAddOn) (addonfactory.Values, error) { values := map[string]interface{}{} annotations := cluster.GetAnnotations() logger.V(4).Info("Try to get image registries from annotation", diff --git a/pkg/addon/templateagent/values_test.go b/pkg/addon/templateagent/values_test.go index 7c0bc053c..1ba713a58 100644 --- a/pkg/addon/templateagent/values_test.go +++ b/pkg/addon/templateagent/values_test.go @@ -1,11 +1,15 @@ package templateagent import ( + "context" + "fmt" "reflect" "testing" "github.com/stretchr/testify/assert" + apiequality "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/klog/v2" "k8s.io/klog/v2/ktesting" "open-cluster-management.io/addon-framework/pkg/addonfactory" @@ -100,3 +104,155 @@ func TestGetAddOnRegistriesPrivateValuesFromClusterAnnotation(t *testing.T) { }) } } + +func TestGetValues(t *testing.T) { + cases := []struct { + name string + templateSpec addonapiv1alpha1.AddOnTemplateSpec + values addonfactory.Values + expectedDefault orderedValues + expectedOverride map[string]interface{} + expectedPrivate map[string]interface{} + }{ + { + name: "no override", + templateSpec: addonapiv1alpha1.AddOnTemplateSpec{ + Registration: []addonapiv1alpha1.RegistrationSpec{ + { + Type: addonapiv1alpha1.RegistrationTypeKubeClient, + }, + }, + }, + expectedDefault: orderedValues{ + { + name: "HUB_KUBECONFIG", + value: "/managed/hub-kubeconfig/kubeconfig", + }, + { + name: "CLUSTER_NAME", + value: "test-cluster", + }, + }, + expectedOverride: map[string]interface{}{ + "CLUSTER_NAME": "test-cluster", + "HUB_KUBECONFIG": "/managed/hub-kubeconfig/kubeconfig", + }, + expectedPrivate: map[string]interface{}{}, + }, + { + name: "overrides builtin value", + templateSpec: addonapiv1alpha1.AddOnTemplateSpec{}, + values: addonfactory.Values{ + InstallNamespacePrivateValueKey: "default-ns", + }, + expectedDefault: orderedValues{ + { + name: "CLUSTER_NAME", + value: "test-cluster", + }, + }, + expectedOverride: map[string]interface{}{ + "CLUSTER_NAME": "test-cluster", + }, + expectedPrivate: map[string]interface{}{ + InstallNamespacePrivateValueKey: "default-ns", + }, + }, + { + name: "overrides public value", + templateSpec: addonapiv1alpha1.AddOnTemplateSpec{}, + values: addonfactory.Values{ + InstallNamespacePrivateValueKey: "default-ns", + "key1": "value1", + }, + expectedDefault: orderedValues{ + { + name: "CLUSTER_NAME", + value: "test-cluster", + }, + }, + expectedOverride: map[string]interface{}{ + "CLUSTER_NAME": "test-cluster", + "key1": "value1", + }, + expectedPrivate: map[string]interface{}{ + InstallNamespacePrivateValueKey: "default-ns", + }, + }, + { + name: "default should not be overridden", + templateSpec: addonapiv1alpha1.AddOnTemplateSpec{}, + values: addonfactory.Values{ + InstallNamespacePrivateValueKey: "default-ns", + "CLUSTER_NAME": "another-cluster", + }, + expectedDefault: orderedValues{ + { + name: "CLUSTER_NAME", + value: "test-cluster", + }, + }, + expectedOverride: map[string]interface{}{ + "CLUSTER_NAME": "test-cluster", + }, + expectedPrivate: map[string]interface{}{ + InstallNamespacePrivateValueKey: "default-ns", + }, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + getValueFunc := func( + cluster *clusterv1.ManagedCluster, + addon *addonapiv1alpha1.ManagedClusterAddOn) (addonfactory.Values, error) { + return c.values, nil + } + + agentAddon := &CRDTemplateAgentAddon{ + logger: klog.FromContext(context.TODO()), + getValuesFuncs: []addonfactory.GetValuesFunc{getValueFunc}, + } + + cluster := &clusterv1.ManagedCluster{ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"}} + addon := &addonapiv1alpha1.ManagedClusterAddOn{ObjectMeta: metav1.ObjectMeta{ + Name: "test-addon", + Namespace: "test-cluster", + }} + + addonTemplate := &addonapiv1alpha1.AddOnTemplate{ + ObjectMeta: metav1.ObjectMeta{Name: "test-addon"}, + Spec: c.templateSpec, + } + + defaultValue, overrideValue, builtInValue, err := agentAddon.getValues(cluster, addon, addonTemplate) + if err != nil { + t.Fatal(err) + } + fmt.Printf("defaultValue is %v\n", defaultValue) + if !orderedValueEquals(defaultValue, c.expectedDefault) { + t.Errorf("default value is not correct, expect %v, got %v", c.expectedDefault, defaultValue) + } + + if !apiequality.Semantic.DeepEqual(overrideValue, c.expectedOverride) { + t.Errorf("override value is not correct, expect %v, got %v", c.expectedOverride, overrideValue) + } + + if !apiequality.Semantic.DeepEqual(builtInValue, c.expectedPrivate) { + t.Errorf("builtin value is not correct, expect %v, got %v", c.expectedPrivate, builtInValue) + } + }) + } +} + +func orderedValueEquals(new, old orderedValues) bool { + if len(new) != len(old) { + return false + } + for index := range new { + if new[index] != old[index] { + return false + } + } + return true +} diff --git a/test/e2e/addonmanagement_test.go b/test/e2e/addonmanagement_test.go index 774bf5473..702d69e1a 100644 --- a/test/e2e/addonmanagement_test.go +++ b/test/e2e/addonmanagement_test.go @@ -28,6 +28,7 @@ import ( const ( nodePlacementDeploymentConfigName = "node-placement-deploy-config" imageOverrideDeploymentConfigName = "image-override-deploy-config" + namespaceOverrideConfigName = "namespace-override-config" originalImageValue = "quay.io/open-cluster-management/addon-examples:latest" overrideImageValue = "quay.io/ocm/addon-examples:latest" customSignerName = "example.com/signer-name" @@ -476,6 +477,55 @@ var _ = ginkgo.Describe("Enable addon management feature gate", ginkgo.Ordered, }) + ginkgo.It("Template type addon should be configured by addon deployment config for namespace", func() { + ginkgo.By("Prepare a AddOnDeploymentConfig for namespace config") + overrideNamespace := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "another-addon-namespace", + }, + } + _, err := t.SpokeKubeClient.CoreV1().Namespaces().Create(context.TODO(), overrideNamespace, metav1.CreateOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Eventually(func() error { + return prepareInstallNamespace(clusterName, overrideNamespace.Name) + }, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred()) + + ginkgo.By("Add the configs to ManagedClusterAddOn") + gomega.Eventually(func() error { + addon, err := t.AddOnClinet.AddonV1alpha1().ManagedClusterAddOns(clusterName).Get( + context.Background(), addOnName, metav1.GetOptions{}) + if err != nil { + return err + } + newAddon := addon.DeepCopy() + newAddon.Spec.Configs = []addonapiv1alpha1.AddOnConfig{ + { + ConfigGroupResource: addonapiv1alpha1.ConfigGroupResource{ + Group: "addon.open-cluster-management.io", + Resource: "addondeploymentconfigs", + }, + ConfigReferent: addonapiv1alpha1.ConfigReferent{ + Namespace: clusterName, + Name: namespaceOverrideConfigName, + }, + }, + } + _, err = t.AddOnClinet.AddonV1alpha1().ManagedClusterAddOns(clusterName).Update( + context.Background(), newAddon, metav1.UpdateOptions{}) + if err != nil { + return err + } + return nil + }, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred()) + + ginkgo.By("Make sure addon is configured") + gomega.Eventually(func() error { + _, err := t.SpokeKubeClient.AppsV1().Deployments(overrideNamespace.Name).Get( + context.Background(), "hello-template-agent", metav1.GetOptions{}) + return err + }, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred()) + }) + ginkgo.It("Template type addon's image should be overrode by cluster annotation", func() { ginkgo.By("Prepare cluster annotation for addon image override config") overrideRegistries := addonapiv1alpha1.AddOnDeploymentConfigSpec{ @@ -564,6 +614,32 @@ var _ = ginkgo.Describe("Enable addon management feature gate", ginkgo.Ordered, }) +func prepareInstallNamespace(namespace, installNamespace string) error { + _, err := t.AddOnClinet.AddonV1alpha1().AddOnDeploymentConfigs(namespace).Get( + context.Background(), namespaceOverrideConfigName, metav1.GetOptions{}) + if errors.IsNotFound(err) { + if _, err := t.AddOnClinet.AddonV1alpha1().AddOnDeploymentConfigs(namespace).Create( + context.Background(), + &addonapiv1alpha1.AddOnDeploymentConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: namespaceOverrideConfigName, + Namespace: namespace, + }, + Spec: addonapiv1alpha1.AddOnDeploymentConfigSpec{ + AgentInstallNamespace: installNamespace, + }, + }, + metav1.CreateOptions{}, + ); err != nil { + return err + } + + return nil + } + + return err +} + func prepareImageOverrideAddOnDeploymentConfig(namespace, installNamespace string) error { _, err := t.AddOnClinet.AddonV1alpha1().AddOnDeploymentConfigs(namespace).Get( context.Background(), imageOverrideDeploymentConfigName, metav1.GetOptions{}) @@ -588,14 +664,10 @@ func prepareImageOverrideAddOnDeploymentConfig(namespace, installNamespace strin return nil } - if err != nil { - return err - } - - return nil + return err } -func prepareNodePlacementAddOnDeploymentConfig(namespace, installNamespae string) error { +func prepareNodePlacementAddOnDeploymentConfig(namespace, installNamespace string) error { _, err := t.AddOnClinet.AddonV1alpha1().AddOnDeploymentConfigs(namespace).Get( context.Background(), nodePlacementDeploymentConfigName, metav1.GetOptions{}) if errors.IsNotFound(err) { @@ -611,7 +683,7 @@ func prepareNodePlacementAddOnDeploymentConfig(namespace, installNamespae string NodeSelector: nodeSelector, Tolerations: tolerations, }, - AgentInstallNamespace: installNamespae, + AgentInstallNamespace: installNamespace, }, }, metav1.CreateOptions{}, @@ -622,11 +694,7 @@ func prepareNodePlacementAddOnDeploymentConfig(namespace, installNamespae string return nil } - if err != nil { - return err - } - - return nil + return err } func copySignerSecret(ctx context.Context, kubeClient kubernetes.Interface, srcNs, srcName, dstNs, dstName string) error {