Skip to content
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

Open
8 tasks done
dusatvoj opened this issue Jan 13, 2024 · 17 comments
Open
8 tasks done

Can't deploy fully configured MetalLB via ArgoCD via helm #2241

dusatvoj opened this issue Jan 13, 2024 · 17 comments
Labels

Comments

@dusatvoj
Copy link

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 and L2Advertisement.

I found out there's bug describing the same but it's closed (#1691) and also closed but unmerged PR (#1484).

To Reproduce

  1. Deploy emtpy cluster
  2. Deploy argocd with main helmchart
  3. Everything works but MetalLB (as critical component) doesn't have defined IPAddressPool and L2Advertisement
  4. You have to manually deploy these manifests

Expected Behavior

Can define IPAddressPool and L2Advertisement (and probably BGPAdvertisement ) via helm values as optional object

Additional 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 checked all open and closed issues and my request is not there.
  • I've checked all open and closed pull requests and my request is not there.

I've read and agree with the following

  • I've checked all open and closed issues and my issue is not there.
  • This bug is reproducible when deploying MetalLB from the main branch
  • I have read the troubleshooting guide and I am still not able to make it work
  • I checked the logs and MetalLB is not discarding the configuration as not valid
  • I enabled the debug logs, collected the information required from the cluster using the collect script and will attach them to the issue
  • I will provide the definition of my service and the related endpoint slices and attach them to this issue
@dusatvoj dusatvoj added the bug label Jan 13, 2024
@fedepaol
Copy link
Member

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

@dusatvoj
Copy link
Author

Not sure, maybe there could be a list with ipAddressPools, l2Advertisement and bgpAdvertisement and in foreach there could be generated CRs.

@fedepaol
Copy link
Member

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.

@tamcore
Copy link
Contributor

tamcore commented Jan 17, 2024

Use an umbrella chart in ArgoCD.

$ cat Chart.yaml
apiVersion: v2
name: metallb
description: A Helm chart for Kubernetes

type: application

version: 0.1.0

appVersion: "0.1.0"

dependencies:
- name: metallb
  version: 0.13.12
  repository: https://metallb.github.io/metallb

and then put your desired templates / manifest into the templates sub-folder. Like

$ cat templates/metallb_config.yaml
---
apiVersion: metallb.io/v1beta1
kind: IPAddressPool
metadata:
  name: metallb-default
spec:
  addresses:
  - 192.168.69.30-192.168.69.39
  autoAssign: true
---
apiVersion: metallb.io/v1beta1
kind: L2Advertisement
metadata:
  name: metallb-default
  namespace: metallb-system
spec:
  ipAddressPools:
  - metallb-default

@dusatvoj
Copy link
Author

@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 🤔

@disco-stu
Copy link
Contributor

@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: helm.go:84: [debug] resource mapping not found for name: "metallb-default" namespace: "metallb-system" from "": no matches for kind "IPAddressPool" in version "[metallb.io/v1beta1“](http://metallb.io/v1beta1%E2%80%9C%60).

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 —wait. This aproch worked in the end but has other side effects if one runs an update later on.

Here are some examples based on popular helm charts:

  • Cert-manager with —installCRDs=true: Treat CRDs like normal resources. That’s basically the same behavior as the current MetalLB Helm chart.
  • Kube-prometheus-stack: Handle CRDs as CRDs in the sense of helm. This would prevent updates on future if one runs helm upgrade calls as well as the the removal of CRDs if the helm chart gets deleted. Therefore, it involves manual actions to update CRDs.

Beside the helm install —wait option I mentioned above there are is another option: Install CRDs through a pre-install,pre-upgrade hook via a dedicated Kubernetes Job that does something like `kubectl apply´. That might work but I didn’t tested it so far.

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.

@tamcore
Copy link
Contributor

tamcore commented Mar 14, 2024

Helm has a special crds folder. That way helm will install the CRDs before other any other resources: https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-1-let-helm-do-it-for-you

@disco-stu
Copy link
Contributor

Helm has a special crds folder. That way helm will install the CRDs before other any other resources: https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-1-let-helm-do-it-for-you

Thats what i meant by "treat CRDs as CRDs in the helm way".

@dusatvoj
Copy link
Author

@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"? 🤔

@disco-stu
Copy link
Contributor

@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.

disco-stu added a commit to credativ/metallb that referenced this issue Mar 26, 2024
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>
@fedepaol
Copy link
Member

Helm has a special crds folder. That way helm will install the CRDs before other any other resources: https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-1-let-helm-do-it-for-you

The problem with this is "These CRDs are not templated," while because of the conversion webhook we must inject the namespace here

@fedepaol
Copy link
Member

@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.

@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?

@disco-stu
Copy link
Contributor

@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.

@tamcore
Copy link
Contributor

tamcore commented Mar 27, 2024

Helm has a special crds folder. That way helm will install the CRDs before other any other resources: https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-1-let-helm-do-it-for-you

The problem with this is "These CRDs are not templated," while because of the conversion webhook we must inject the namespace here

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

@fedepaol
Copy link
Member

Helm has a special crds folder. That way helm will install the CRDs before other any other resources: https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-1-let-helm-do-it-for-you

The problem with this is "These CRDs are not templated," while because of the conversion webhook we must inject the namespace here

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?

@disco-stu
Copy link
Contributor

Helm has a special crds folder. That way helm will install the CRDs before other any other resources: https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-1-let-helm-do-it-for-you

The problem with this is "These CRDs are not templated," while because of the conversion webhook we must inject the namespace here

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.

@disco-stu
Copy link
Contributor

IMHO we have two reasonable options:

  1. Use a two step approach: Use the metallb-chart as it is right now and use a second chart to configure metalllb (thats what I'm working on right now).

  2. Build a POC based on the velero and nginx-ingress approach. This might include the following changes: Move the CRDs into the Helm crds folder, which would include removing all templated variables. Within a pre-xxx hook: Patch the webhook namespace to point to the right webhook receiver. This pre-xxx hook job could also update the CRDs to overcome the helm upgrade CRD issue.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants