open-cluster-management-io / addon-framework

addon apis
Apache License 2.0
23 stars 40 forks source link

Add support to configure helm-agent-addon namespace. #217

Closed Imtiaz246 closed 3 days ago

Imtiaz246 commented 11 months ago

Problem Statement:

Proposed Solution:

Use Case Example:

zhujian7 commented 11 months ago

@Imtiaz246 thanks for reporting this

we can't configure the namespace dynamically as per the https://github.com/open-cluster-management-io/ocm/issues/298 describes.

PR https://github.com/open-cluster-management-io/addon-framework/pull/218 is trying to fix this.

additionally, we want to leverage helm chart built-in values such as {{ Release.Namespace }} and {{ Release.Name }}

there are 3 helm built-in values are supported, fortunately, {{ Release.Namespace }} and {{ Release.Name }} are included.

zhujian7 commented 10 months ago

@Imtiaz246 I submitted a PR to support configuring the namespace for the helm addons, if you want you can try it with the latest addon-framework, example here

Imtiaz246 commented 10 months ago

Great @zhujian7. Thank you for your quick response. Things are working as expected now.

tamalsaha commented 10 months ago

@zhujian7 , I think there is still one thing left to fix

https://github.com/open-cluster-management-io/addon-framework/blob/674dd8271a0870560a39829e189f56d13d7a6dd5/pkg/addonmanager/controllers/addoninstall/controller.go#L97-L113

addon.GetAgentAddonOptions().InstallStrategy.InstallNamespace is configured correctly

tamalsaha commented 10 months ago

There is also one more issue here. The ManagedClusterAddon crd defaults installNamespace to open-cluster-management-agent-addon.

https://github.com/open-cluster-management-io/api/blob/658739714f84467fa4e057c24d594fd6d416bed9/addon/v1alpha1/types_managedclusteraddon.go#L39

This causes problem because it does not take the correct installNamespace from the ClusterManagementAddon. https://github.com/open-cluster-management-io/ocm/blob/8a4c834ebf0f3c71ab9209572fb0b11cc942f5d5/pkg/addon/controllers/addonmanagement/addon_install_reconciler.go#L72

qiujian16 commented 6 months ago

/assign @zhujian7

zhujian7 commented 6 months ago

@tamalsaha we are going to deprecate the managedclusteraddon.spec.instllNamespace, if you want to make the ns configurable, I suggest you use the AgentInstallNamespace func to fully control the namespace value, for example, you can implement this func to read the value from the API addondeploymentconfig.spec.agentInstallNamespace, then any value changed in the field addondeploymentconfig.spec.agentInstallNamespace will be reflected on where the addon agent is installed.

qiujian16 commented 3 days ago

/close

openshift-ci[bot] commented 3 days ago

@qiujian16: Closing this issue.

In response to [this](https://github.com/open-cluster-management-io/addon-framework/issues/217#issuecomment-2367121226): >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.