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

* check return err for customized agent install namespace Signed-off-by: zhujian <[email protected]> * Fix e2e Signed-off-by: zhujian <[email protected]> * Use default namespace if addon deployment config is not configured Signed-off-by: zhujian <[email protected]> --------- Signed-off-by: zhujian <[email protected]>
open-cluster-management-io · Apr 2, 2024 · be542a6 · be542a6
1 parent c37bed7
commit be542a6
Show file tree

Hide file tree

Showing 8 changed files with 74 additions and 34 deletions.
diff --git a/pkg/addonfactory/addonfactory.go b/pkg/addonfactory/addonfactory.go
@@ -40,7 +40,7 @@ type AgentAddonFactory struct {
 	// Deprecated: use clusterClient to get the hosting cluster.
 	hostingCluster        *clusterv1.ManagedCluster
 	clusterClient         clusterclientset.Interface
-	agentInstallNamespace func(addon *addonapiv1alpha1.ManagedClusterAddOn) string
+	agentInstallNamespace func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error)
 	helmEngineStrict      bool
 }
 
@@ -164,7 +164,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
@@ -55,7 +55,7 @@ type HelmAgentAddon struct {
 	// Deprecated: use clusterClient to get the hosting cluster.
 	hostingCluster        *clusterv1.ManagedCluster
 	clusterClient         clusterclientset.Interface
-	agentInstallNamespace func(addon *addonapiv1alpha1.ManagedClusterAddOn) string
+	agentInstallNamespace func(addon *addonapiv1alpha1.ManagedClusterAddOn) (string, error)
 	helmEngineStrict      bool
 }
 
@@ -204,8 +204,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
@@ -214,18 +218,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(
@@ -234,7 +245,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, _ = a.agentAddonOptions.HostedModeInfoFunc(addon, cluster)
 
@@ -302,11 +317,16 @@ 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
 }
 
 // manifest represents a manifest file, which has a name and some content.

diff --git a/pkg/addonfactory/template_agentaddon.go b/pkg/addonfactory/template_agentaddon.go
@@ -41,7 +41,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 {
@@ -108,15 +108,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()
 
@@ -125,16 +128,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, _ = a.agentAddonOptions.HostedModeInfoFunc(addon, cluster)
 
-	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
@@ -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
@@ -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
@@ -120,7 +120,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
@@ -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
@@ -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")
 		})
 	}