⚠️ check return err for customized agent install namespace (#257) (#264)

* check return err for customized agent install namespace



* Fix e2e



* Use default namespace if addon deployment config is not configured



---------

Signed-off-by: zhujian <jiazhu@redhat.com>
Signed-off-by: Jian Qiu <jqiu@redhat.com>
Co-authored-by: Jian Zhu <jiazhu@redhat.com>

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

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
 }

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,15 +109,18 @@ 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
 }
 
 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(

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)
 		}
 	}
 

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
+				},
 			},
 		},
 	}

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

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
 	}
 }
 

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")
 		})
 	}