New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Can't deploy fully configured MetalLB via ArgoCD via helm #2241
Comments
Thanks for raising this again! We just need to understand what is the canonical way for helm to handle this / how other projects are handling it. I am not super helm-savvy. @dusatvoj do you have references of other charts that allow embedding the CRs directly with helm? Otherwise we'll try to look for answers, but I don't feel allowing a blob in our chart is the right way (I might be wrong). |
Not sure, maybe there could be a list with ipAddressPools, l2Advertisement and bgpAdvertisement and in foreach there could be generated CRs. |
I'll try to do an assessment on other projects and will ask on the helm channel on slack. I just want to avoid doing anything too esotic. |
Use an umbrella chart in ArgoCD.
and then put your desired templates / manifest into the
|
@tamcore , good point - thank you for suggestion but still I think this is just workaround, bcs MetalLB should have option to be configured via it's own helm chart 🤔 |
@dusatvoj as discussed via Slack I had a look into the current helm chart(s) as well as other helm charts in order to find a solution that involves managing CRDs as well as the corresponding custom resources for MetalLB. Unfortunately, I didn’t found a way to keep the CRDs updated and mange CRs at the same time within one helm chart (that doesn’t involve Kubernetes jobs to setup the CRDs). What we have right now: The current helm setup involves a dependent chart that contains all CRDs. However, those CRDs are handled as normal resources. The downside of this approach is that helm doesn’t treat them as CRDs. On the other site the current implementation involves a full lifecycle management of those CRDs. Involving updates and removals. Which one is better is part of another discussion. If we stick to the current implementation, we will run into issues at installation time. This is because helm tries to render the manifests against the API. Since our CRDs are not yet applied this process runs into errors saying: Initially I’ve tried to install the CRDs within a pre-install,pre-upgrade helm hook (… this, however, would lead to other downsides). Unfortunately, I’ve ran unto the same issue. My second thought was to install the CRs like ipAddressPool within a post-install hook. That kinda worked, but only if the validatingWebhook was ready. In order to overcome issues involving an unready validatingWebhook I had to run helm install with Here are some examples based on popular helm charts:
Beside the The last thing that comes to my mind isn’t a full fledged all-in-one solution but might make the live of users at least easier: Create a dedicated metallb-config helm chart, that includes all templates that one might apply in the MetalLB context. As a side node: ArgoCD e.g. handles the upgrades of CRDs just fine. Even if a manual helm upgrade call wouldn't touch those. |
Helm has a special |
Thats what i meant by "treat CRDs as CRDs in the helm way". |
@disco-stu , many thank's for taking care of this issue and also explanation. So what are next steps? Close this issue and keep it as "how to"? 🤔 |
@dusatvoj in my opinion we should try to come up with a dedicated metallb-config helm chart. Even if it will not solve the initial issue in the first place, it prevents uses from having to write there own helm charts or additional configs. Later on, if one decides to use the usual way in helm to deploy CRDs, we could reuse that config helm chart and reuse it. I would be happy to support and would try to come up with such a metallb-config helm chart. |
This commit adds a new helm chart that can be used to configure metallb. See: metallb#2241. Signed-off-by: Adrian Vondendriesch <adrian.vondendriesch@netapp.com>
The problem with this is "These CRDs are not templated," while because of the conversion webhook we must inject the namespace here |
@disco-stu if I understand your suggestion is to separate the config part from the CRD part and have an umbrella chart that may contain both? |
@fedepaol not exactly. I've already created a draft for a new "standalone" helm chart. https://github.com/credativ/metallb/tree/metallb-config The metallb-config chart could be used to configure metallb after it's first installation. |
Then i would suggest to have a look at how other projects do it. ingress-nginx for example, deploys a Job that patches the their webhook: https://github.com/kubernetes/ingress-nginx/blob/main/charts/ingress-nginx/templates/admission-webhooks/job-patch/job-patchWebhook.yaml. Other projects, like Velero, have a similar approach: https://github.com/vmware-tanzu/helm-charts/blob/main/charts/velero/templates/upgrade-crds/upgrade-crds.yaml |
Interesting! But we'd still have the issue of crds not upgrading automatically right? |
The solution that velero implemented overcomes this issue by running a pre-install,pre-upgrade,pre-rollback hook that manually applies the CRDs. It uses the velero binary to get the right version of those CRDs and applies those with the help of kubectl. This would also solve this issue, but in my opinion it feels a little bit hacky. |
IMHO we have two reasonable options:
However, even if solution 2 would solve the various issues: template in crds, update crds on helm upgrade and maybe rollback, we would still have the problem on a "NotReady" webhook pod at installation time, if we want to deploy CRs in the same run. Any thoughts? Did I missed something? |
MetalLB Version
0.13.12
Deployment method
Charts
Main CNI
Cillium
Kubernetes Version
1.27.9
Cluster Distribution
rancher
Describe the bug
I want to use ArgoCD as configurator of whole cluster so I have per-cluster helm chart with argocd apps. All apps so far can be completely configured via helm chart values.
MetalLB can't be fully configured. After creating the whole setup I have tu run raw kubernetes manifest with
IPAddressPool
andL2Advertisement
.I found out there's bug describing the same but it's closed (#1691) and also closed but unmerged PR (#1484).
To Reproduce
IPAddressPool
andL2Advertisement
Expected Behavior
Can define
IPAddressPool
andL2Advertisement
(and probablyBGPAdvertisement
) via helm values as optional objectAdditional Context
I just submitted everything below just for allowing me to submit this issue. The closed issue has ignored comments for months, so I don't want to be just +1 ignored comment. I think this is important thing to be completed.
I've read and agree with the following
I've read and agree with the following
The text was updated successfully, but these errors were encountered: