Upgrade ingress addon

[dud225]

• Switch to a Helm-based installation. That will help to relieve the
  project of the maintenance burden as by default the latest chart
  version is picked. If case of trouble the user can override the chart
  version to install.
• Set up the admission controller by default. This helps to detect
  invalid configurations early, see
  https://kubernetes.github.io/ingress-nginx/how-it-works/#avoiding-outage-from-wrong-configuration.
  It can be disabled at addon installation time to get a configuration
  similar to what is set up so far.
• breaking change: the metrics endpoint is no longer exposed on the host
  (hostPort), instead it is exposed internally only through a ClusterIP
  service. Note that it is fairly easy to expose it externally though an
  Ingress.

[neoaggelos]

Thank you for this PR @dud225.

I have not reviewed this yet, will do after CI is successful. In general, I am not opposed to using the nginx-ingress helm chart for this, but I am a bit wary about the implicit dependency on https://kubernetes.github.io/ingress-nginx being reachable when enabling the ingress addon. This might not be the case for enterprise, firewall-ed, airgap environments etc, and this change would break existing flows.

I am not sure how best to tackle this, including the helm chart could be an option.

[dud225]

I have not reviewed this yet, will do after CI is successful. In general, I am not opposed to using the nginx-ingress helm chart for this, but I am a bit wary about the implicit dependency on https://kubernetes.github.io/ingress-nginx being reachable when enabling the ingress addon. This might not be the case for enterprise, firewall-ed, airgap environments etc, and this change would break existing flows.

I am not sure how best to tackle this, including the helm chart could be an option.

I do agree with you and, to be honest, I'm also concerned by the fact that the latest chart version is pulled by default, which means that depending on the time the user installs the addon, 2 different users may not get the same result despite running on the same Microk8s version.

However this approach has benefits for the Microk8s project : much less maintenance cost since the latest chart is pulled and most importantly much more flexibility for the user since custom values can be passed along.

I thought that there was an agreement on this approach as this is currently what has been done on the observability addon:

microk8s-core-addons/addons/observability/enable

Lines 39 to 56 in f9d2f4b

	$HELM repo add prometheus-community https://prometheus-community.github.io/helm-charts
	$HELM repo add grafana https://grafana.github.io/helm-charts
	$HELM repo update
	if [ -z "$VALUES" ]
	then
	$HELM upgrade --install kube-prom-stack -f ${SCRIPT_DIR}/grafana.yaml \
	--set kubeControllerManager.endpoints={$NODE_ENDPOINTS} \
	--set kubeScheduler.endpoints={$NODE_ENDPOINTS} \
	prometheus-community/kube-prometheus-stack -n $NAMESPACE_PTR
	else
	$HELM upgrade --install kube-prom-stack -f ${SCRIPT_DIR}/grafana.yaml -f "$2" \
	--set kubeControllerManager.endpoints={$NODE_ENDPOINTS} \
	--set kubeScheduler.endpoints={$NODE_ENDPOINTS} \
	prometheus-community/kube-prometheus-stack -n $NAMESPACE_PTR
	fi
	$HELM upgrade --install loki grafana/loki-stack -n $NAMESPACE_PTR --set="grafana.sidecar.datasources.enabled=false"
	$HELM upgrade --install tempo grafana/tempo -n $NAMESPACE_PTR

I also brought this topic on Slack and, albeit this specific point wasn't discussed, the answer I got made me believe this approach was palatable.

I'd suggest that you discuss this internally to clarify that matter.

[dud225]

The CI has been fixed.

[neoaggelos]

I also brought this topic on Slack and, albeit this specific point wasn't discussed, the answer I got made me believe this approach was palatable.
I'd suggest that you discuss this internally to clarify that matter.

Looking at the conversation, using upstream Helm charts is indeed something we consider and are positive towards. The issue is with implicitly adding dependencies that might break existing workflows. :)

Any thoughts @ktsakalozos?

[dud225]

We could store a copy of the chart to install, much as the manifest, but then the addon will constantly trail behind the latest upstream version. For a small and mature addon this is OK, but for a more active one like this one it is suboptimal.

[ktsakalozos]

Thank you for this work @dud225. You an @neoaggelos bring up valid concerns.

The biggest show stopper for me is the deployment of the "latest" stable ingress release. Looking at the ingress releases I see the support for k8s v1.19 was dropped with the v1.24 release. This makes sense however when looking at what people deploy we see a significant amount of users still using/deploying v1.19. Following the "latest" would have made more sense in rolling k8s distributions but MicroK8s is not one.

[dud225]

Thanks for your acknowledgements.
When you wrote "using/deploying v1.19" you mean MicroK8s version 1.19 don't you? If so the addon repository is frozen alongside MicroK8s when the snap is built, so that the users using this version will still be pointed to the ingress addon that was in the repo at that time, isn't that correct? Tell me if I'm missing something.
Otherwise I can fix the ingress version to one that suits if pulling a chart is fine for you.

[ktsakalozos]

Sorry my point was not clear.

Lets assume we merge this PR and from the 1.26 on wards we always deploy the latest ingress. In about 14 months upstream will not be supported anymore so the latest ingress at that time will drop support of 1.26. this means that there may be a risk of the latest ingress at that point breaking on the 1.26 deployments. Unfortunately there will be still be users deploying 1.26 even though it is out of support.

So yes we should be shipping a pinned version of ingress and to address airgapped usecases the helm packages should be in the snap.

[dud225]

Thanks the comment that's now clear.

to address airgapped usecases the helm packages should be in the snap.

How to do this? Shall I copy/paste the chart into the repo? Or is there a build script that should be modified to handle this?

[ktsakalozos]

How to do this?

The mayastor addon on this repo is a good example.

[dud225]

I've vendored the ingress-nginx Helm chart version 4.4.0.