From 4684b4c073547c55b4a4ef71667baaa2d0f3dc3d Mon Sep 17 00:00:00 2001 From: Jian Zhu Date: Tue, 2 Apr 2024 09:38:59 +0800 Subject: [PATCH] :warning: check return err for customized agent install namespace (#257) * check return err for customized agent install namespace Signed-off-by: zhujian * Fix e2e Signed-off-by: zhujian * Use default namespace if addon deployment config is not configured Signed-off-by: zhujian --------- Signed-off-by: zhujian --- pkg/addonfactory/addonfactory.go | 4 +- pkg/addonfactory/helm_agentaddon.go | 40 ++++++++++++++----- pkg/addonfactory/template_agentaddon.go | 20 +++++++--- .../controllers/registration/controller.go | 9 +++-- .../registration/controller_test.go | 8 ++-- pkg/agent/inteface.go | 2 +- pkg/utils/addon_config.go | 23 ++++++----- pkg/utils/addon_config_test.go | 2 +- 8 files changed, 74 insertions(+), 34 deletions(-) diff --git a/pkg/addonfactory/addonfactory.go b/pkg/addonfactory/addonfactory.go index 91e8df4e1..e806be6a0 100644 --- a/pkg/addonfactory/addonfactory.go +++ b/pkg/addonfactory/addonfactory.go @@ -36,7 +36,7 @@ type AgentAddonFactory struct { // trimCRDDescription flag is used to trim the description of CRDs in manifestWork. disabled by default. trimCRDDescription bool hostingCluster *clusterv1.ManagedCluster - agentInstallNamespace func(addon *addonapiv1alpha1.ManagedClusterAddOn) string + agentInstallNamespace func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error) } // NewAgentAddonFactory builds an addonAgentFactory instance with addon name and fs. @@ -150,7 +150,7 @@ func (f *AgentAddonFactory) WithAgentDeployTriggerClusterFilter( // override the default built-in namespace value; And if the registrationOption is not nil but the // registrationOption.AgentInstallNamespace is nil, this will also set it to this. func (f *AgentAddonFactory) WithAgentInstallNamespace( - nsFunc func(addon *addonapiv1alpha1.ManagedClusterAddOn) string, + nsFunc func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error), ) *AgentAddonFactory { f.agentInstallNamespace = nsFunc return f diff --git a/pkg/addonfactory/helm_agentaddon.go b/pkg/addonfactory/helm_agentaddon.go index c8e8f9917..fbb9b8c5e 100644 --- a/pkg/addonfactory/helm_agentaddon.go +++ b/pkg/addonfactory/helm_agentaddon.go @@ -48,7 +48,7 @@ type HelmAgentAddon struct { agentAddonOptions agent.AgentAddonOptions trimCRDDescription bool hostingCluster *clusterv1.ManagedCluster - agentInstallNamespace func(addon *addonapiv1alpha1.ManagedClusterAddOn) string + agentInstallNamespace func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error) } func newHelmAgentAddon(factory *AgentAddonFactory, chart *chart.Chart) *HelmAgentAddon { @@ -177,8 +177,12 @@ func (a *HelmAgentAddon) getValues( overrideValues = MergeValues(overrideValues, builtinValues) + releaseOptions, err := a.releaseOptions(addon) + if err != nil { + return nil, err + } values, err := chartutil.ToRenderValues(a.chart, overrideValues, - a.releaseOptions(addon), a.capabilities(cluster, addon)) + releaseOptions, a.capabilities(cluster, addon)) if err != nil { klog.Errorf("failed to render helm chart with values %v. err:%v", overrideValues, err) return values, err @@ -187,18 +191,25 @@ func (a *HelmAgentAddon) getValues( return values, nil } -func (a *HelmAgentAddon) getValueAgentInstallNamespace(addon *addonapiv1alpha1.ManagedClusterAddOn) string { +func (a *HelmAgentAddon) getValueAgentInstallNamespace(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error) { installNamespace := addon.Spec.InstallNamespace if len(installNamespace) == 0 { installNamespace = AddonDefaultInstallNamespace } if a.agentInstallNamespace != nil { - ns := a.agentInstallNamespace(addon) + ns, err := a.agentInstallNamespace(addon) + if err != nil { + klog.Errorf("failed to get agentInstallNamespace from addon %s. err: %v", addon.Name, err) + return "", err + } if len(ns) > 0 { installNamespace = ns + } else { + klog.InfoS("Namespace for addon returned by agent install namespace func is empty", + "addonNamespace", addon.Namespace, "addonName", addon) } } - return installNamespace + return installNamespace, nil } func (a *HelmAgentAddon) getBuiltinValues( @@ -207,7 +218,11 @@ func (a *HelmAgentAddon) getBuiltinValues( builtinValues := helmBuiltinValues{} builtinValues.ClusterName = cluster.GetName() - builtinValues.AddonInstallNamespace = a.getValueAgentInstallNamespace(addon) + addonInstallNamespace, err := a.getValueAgentInstallNamespace(addon) + if err != nil { + return nil, err + } + builtinValues.AddonInstallNamespace = addonInstallNamespace builtinValues.InstallMode, _ = constants.GetHostedModeInfo(addon.GetAnnotations()) @@ -254,9 +269,14 @@ func (a *HelmAgentAddon) capabilities( // only support Release.Name, Release.Namespace func (a *HelmAgentAddon) releaseOptions( - addon *addonapiv1alpha1.ManagedClusterAddOn) chartutil.ReleaseOptions { - return chartutil.ReleaseOptions{ - Name: a.agentAddonOptions.AddonName, - Namespace: a.getValueAgentInstallNamespace(addon), + addon *addonapiv1alpha1.ManagedClusterAddOn) (chartutil.ReleaseOptions, error) { + releaseOptions := chartutil.ReleaseOptions{ + Name: a.agentAddonOptions.AddonName, + } + namespace, err := a.getValueAgentInstallNamespace(addon) + if err != nil { + return releaseOptions, err } + releaseOptions.Namespace = namespace + return releaseOptions, nil } diff --git a/pkg/addonfactory/template_agentaddon.go b/pkg/addonfactory/template_agentaddon.go index 57584ac19..ed2977ac1 100644 --- a/pkg/addonfactory/template_agentaddon.go +++ b/pkg/addonfactory/template_agentaddon.go @@ -42,7 +42,7 @@ type TemplateAgentAddon struct { getValuesFuncs []GetValuesFunc agentAddonOptions agent.AgentAddonOptions trimCRDDescription bool - agentInstallNamespace func(addon *addonapiv1alpha1.ManagedClusterAddOn) string + agentInstallNamespace func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error) } func newTemplateAgentAddon(factory *AgentAddonFactory) *TemplateAgentAddon { @@ -109,7 +109,10 @@ func (a *TemplateAgentAddon) getValues( overrideValues = MergeValues(overrideValues, userValues) } } - builtinValues := a.getBuiltinValues(cluster, addon) + builtinValues, err := a.getBuiltinValues(cluster, addon) + if err != nil { + return overrideValues, err + } overrideValues = MergeValues(overrideValues, builtinValues) return overrideValues, nil @@ -117,7 +120,7 @@ func (a *TemplateAgentAddon) getValues( func (a *TemplateAgentAddon) getBuiltinValues( cluster *clusterv1.ManagedCluster, - addon *addonapiv1alpha1.ManagedClusterAddOn) Values { + addon *addonapiv1alpha1.ManagedClusterAddOn) (Values, error) { builtinValues := templateBuiltinValues{} builtinValues.ClusterName = cluster.GetName() @@ -126,16 +129,23 @@ func (a *TemplateAgentAddon) getBuiltinValues( installNamespace = AddonDefaultInstallNamespace } if a.agentInstallNamespace != nil { - ns := a.agentInstallNamespace(addon) + ns, err := a.agentInstallNamespace(addon) + if err != nil { + klog.Errorf("failed to get agent install namespace for addon %s: %v", addon.Name, err) + return nil, err + } if len(ns) > 0 { installNamespace = ns + } else { + klog.InfoS("Namespace for addon returned by agent install namespace func is empty", + "addonNamespace", addon.Namespace, "addonName", addon) } } builtinValues.AddonInstallNamespace = installNamespace builtinValues.InstallMode, _ = constants.GetHostedModeInfo(addon.GetAnnotations()) - return StructToValues(builtinValues) + return StructToValues(builtinValues), nil } func (a *TemplateAgentAddon) getDefaultValues( diff --git a/pkg/addonmanager/controllers/registration/controller.go b/pkg/addonmanager/controllers/registration/controller.go index 650be3931..db9bef351 100644 --- a/pkg/addonmanager/controllers/registration/controller.go +++ b/pkg/addonmanager/controllers/registration/controller.go @@ -163,12 +163,15 @@ func (c *addonRegistrationController) sync(ctx context.Context, syncCtx factory. } if registrationOption.AgentInstallNamespace != nil { - ns := registrationOption.AgentInstallNamespace(managedClusterAddonCopy) + ns, err := registrationOption.AgentInstallNamespace(managedClusterAddonCopy) + if err != nil { + return err + } if len(ns) > 0 { managedClusterAddonCopy.Status.Namespace = ns } else { - klog.Infof("Namespace for addon %s/%s returned by agent install namespace func is empty", - managedClusterAddonCopy.Namespace, managedClusterAddonCopy.Name) + klog.InfoS("Namespace for addon returned by agent install namespace func is empty", + "addonNamespace", managedClusterAddonCopy.Namespace, "addonName", managedClusterAddonCopy.Name) } } diff --git a/pkg/addonmanager/controllers/registration/controller_test.go b/pkg/addonmanager/controllers/registration/controller_test.go index 38126700a..1e59c4ff0 100644 --- a/pkg/addonmanager/controllers/registration/controller_test.go +++ b/pkg/addonmanager/controllers/registration/controller_test.go @@ -26,7 +26,7 @@ type testAgent struct { name string namespace string registrations []addonapiv1alpha1.RegistrationConfig - agentInstallNamespace func(addon *addonapiv1alpha1.ManagedClusterAddOn) string + agentInstallNamespace func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error) } func (t *testAgent) Manifests(cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) ([]runtime.Object, error) { @@ -211,8 +211,10 @@ func TestReconcile(t *testing.T) { } }, testaddon: &testAgent{name: "test", namespace: "default", - registrations: []addonapiv1alpha1.RegistrationConfig{{SignerName: "test"}}, - agentInstallNamespace: func(addon *addonapiv1alpha1.ManagedClusterAddOn) string { return "default3" }, + registrations: []addonapiv1alpha1.RegistrationConfig{{SignerName: "test"}}, + agentInstallNamespace: func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error) { + return "default3", nil + }, }, }, } diff --git a/pkg/agent/inteface.go b/pkg/agent/inteface.go index 1dd5d7b00..5fc8c08ab 100644 --- a/pkg/agent/inteface.go +++ b/pkg/agent/inteface.go @@ -128,7 +128,7 @@ type RegistrationOption struct { // Note: Set this very carefully. If this is set, the addon agent must be deployed in the same namespace, which // means when implementing Manifests function in AgentAddon interface, the namespace of the addon agent manifest // must be set to the same value returned by this function. - AgentInstallNamespace func(addon *addonapiv1alpha1.ManagedClusterAddOn) string + AgentInstallNamespace func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error) // CSRApproveCheck checks whether the addon agent registration should be approved by the hub. // Addon hub controller can implement this func to auto-approve all the CSRs. A better CSR check is diff --git a/pkg/utils/addon_config.go b/pkg/utils/addon_config.go index ecdb98ad0..eaa91a9f0 100644 --- a/pkg/utils/addon_config.go +++ b/pkg/utils/addon_config.go @@ -7,7 +7,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/klog/v2" addonapiv1alpha1 "open-cluster-management.io/api/addon/v1alpha1" addonv1alpha1client "open-cluster-management.io/api/client/addon/clientset/versioned" @@ -37,23 +36,29 @@ func (g *defaultAddOnDeploymentConfigGetter) Get( // matched addon deployment config, it will return an empty string. func AgentInstallNamespaceFromDeploymentConfigFunc( adcgetter AddOnDeploymentConfigGetter, -) func(*addonapiv1alpha1.ManagedClusterAddOn) string { - return func(addon *addonapiv1alpha1.ManagedClusterAddOn) string { +) func(*addonapiv1alpha1.ManagedClusterAddOn) (string, error) { + return func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error) { if addon == nil { - utilruntime.HandleError(fmt.Errorf("failed to get addon install namespace, addon is nil")) - return "" + return "", fmt.Errorf("failed to get addon install namespace, addon is nil") } config, err := GetDesiredAddOnDeploymentConfig(addon, adcgetter) if err != nil { - utilruntime.HandleError(fmt.Errorf("failed to get deployment config for addon %s: %v", addon.Name, err)) - return "" + return "", fmt.Errorf("failed to get deployment config for addon %s: %v", addon.Name, err) } + + // For now, we have no way of knowing if the addon depleoyment config is not configured, or + // is configured but not yet been added to the managedclusteraddon status config references, + // we expect no error will be returned when the addon deployment config is not configured + // so we can use the default namespace. + // TODO: Find a way to distinguish between the above two cases if config == nil { - return "" + klog.InfoS("Addon deployment config is nil, return an empty string for agent install namespace", + "addonNamespace", addon.Namespace, "addonName", addon.Name) + return "", nil } - return config.Spec.AgentInstallNamespace + return config.Spec.AgentInstallNamespace, nil } } diff --git a/pkg/utils/addon_config_test.go b/pkg/utils/addon_config_test.go index 160cf061a..874dd7906 100644 --- a/pkg/utils/addon_config_test.go +++ b/pkg/utils/addon_config_test.go @@ -168,7 +168,7 @@ func TestAgentInstallNamespaceFromDeploymentConfigFunc(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { nsFunc := AgentInstallNamespaceFromDeploymentConfigFunc(c.getter) - ns := nsFunc(c.mca) + ns, _ := nsFunc(c.mca) assert.Equal(t, c.expected, ns, "should be equal") }) }