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

[kubernetes-ingress] Allow HPA's apiVersion to be specified via chart value #202

Closed
mecampbellsoup opened this issue Jul 25, 2023 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@mecampbellsoup
Copy link
Contributor

Currently the HorizontalPodAutoscaler template hard-codes the value for apiVersion:

{{- if .Capabilities.APIVersions.Has "autoscaling/v2" }}
apiVersion: autoscaling/v2
{{- else if .Capabilities.APIVersions.Has "autoscaling/v2beta2" }}
apiVersion: autoscaling/v2beta2
{{- else }}
  {{- fail "ERROR: You must have autoscaling/v2 or autoscaling/v2beta2 to use HorizontalPodAutoscaler" }}
{{- end }}
kind: HorizontalPodAutoscaler

We use ArgoCD to deploy our Helm charts, which does a helm teplate and then kubectl apply of the templates that are out of sync. As such we cannot rely on .Capabilites Helm object. Moreover, we are on a very old k8s version and so our HPA resource version is autoscaling/v1:

» kubectl api-resources | grep -e autoscaler -e hpa -i
horizontalpodautoscalers                                                                                                  hpa                                    autoscaling/v1                              true         HorizontalPodAutoscaler

Would you accept a PR that modifies the IC Helm chart to enable templating in a value for this instead? Seems like a reasonable/good pattern in general because technically these templates having constraints for the k8s version and should be specifying that in the Chart.yaml anyway. By allowing chart users to specify apiVersion fields on all templates you can punt on having to evaluate those k8s API resource/version constraints in your Helm chart versioning.

Here is the proposed change to the template:

{{- if .Capabilities.APIVersions.Has "autoscaling/v2" }}
apiVersion: autoscaling/v2
{{- else if .Capabilities.APIVersions.Has "autoscaling/v2beta2" }}
apiVersion: autoscaling/v2beta2
{{- else if .Values.controller.autoscaling.apiVersion }}
apiVersion: {{ .Values.controller.autoscaling.apiVersion }}
{{- else }}
  {{- fail "ERROR: You must have autoscaling/v2 or autoscaling/v2beta2 to use HorizontalPodAutoscaler" }}
{{- end }}
kind: HorizontalPodAutoscaler
@dkorunic dkorunic self-assigned this Aug 2, 2023
@dkorunic dkorunic added the enhancement New feature or request label Aug 2, 2023
@dkorunic
Copy link
Member

dkorunic commented Nov 7, 2023

Sadly we don't support autoscaling/v1 any more due due to v1 having only CPU metrics (which is not really usable) and different autoscaling spec format. We will however fix .Capabilities.APIVersions probing in the next commit.

dkorunic added a commit that referenced this issue Nov 7, 2023
Changes in kubernetes-ingress:
- Fixes for .Capabilities.APIVersions issues (issues #202 and #211)
- semverCompare fixes for appProtocol

Signed-off-by: Dinko Korunic <dkorunic@haproxy.com>
@dkorunic dkorunic closed this as completed Nov 7, 2023
@mecampbellsoup
Copy link
Contributor Author

Thanks @dkorunic - what do you mean fix in this case?

@dkorunic
Copy link
Member

dkorunic commented Nov 7, 2023

This commit fixes your issue in regards to ArgoCD not correctly filling out .Capabilities.APIVersions by checking K8s semver, so this should work in your case as long as you have fairly recent K8s with either autoscaling/v2 or autoscaling/v2beta2. Sadly, as we said, autoscaling/v1 is not going to be supported.

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

No branches or pull requests

2 participants