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

Helm: Adapt Hubble ingress template to the new Ingress API #13682

Merged
merged 1 commit into from
Oct 24, 2020

Conversation

youssefazrak
Copy link
Contributor

Ingress and IngressClass resources have graduated to networking.k8s.io/v1 in Kubernetes v1.19. This also introduced changes to the objects.

Also in Helm, GitVersion is being deprecated and has thus been replaced with Version.

Signed-off-by: Youssef Azrak yazrak.tech@gmail.com

Fixes: #13611

Helm: Adapt Hubble ingress template to the new Ingress API

@youssefazrak youssefazrak requested review from a team as code owners October 21, 2020 20:47
@youssefazrak youssefazrak requested review from a team October 21, 2020 20:47
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Oct 21, 2020
@youssefazrak youssefazrak force-pushed the helm_hubble_ingress_fix branch from 533d849 to 694045d Compare October 21, 2020 20:57
@youssefazrak youssefazrak changed the title WIP Helm: Adapt Hubble ingress template to the new Ingress API Helm: Adapt Hubble ingress template to the new Ingress API Oct 21, 2020
@youssefazrak
Copy link
Contributor Author

Hey guys,
Not sure if there is a more elegant solution but I've tested it and we get the right fields.

@sayboras sayboras added area/helm Impacts helm charts and user deployment experience sig/hubble Impacts hubble server or relay release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Oct 21, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 21, 2020
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments I have right now. I haven't tested it out yet, will do later the day.

install/kubernetes/cilium/templates/hubble-ui-ingress.yaml Outdated Show resolved Hide resolved
@sayboras sayboras requested review from a team October 21, 2020 22:51
@@ -9,9 +9,9 @@ Create chart name and version as used by the chart label.
Return the appropriate apiVersion for ingress.
*/}}
{{- define "ingress.apiVersion" -}}
{{- if semverCompare ">=1.4-0, <1.14-0" .Capabilities.KubeVersion.GitVersion -}}
{{- if semverCompare ">=1.4-0, <1.19-0" .Capabilities.KubeVersion.Version -}}
Copy link
Member

@sayboras sayboras Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good pick on version 1.14 here 💯 . For other reviewer, Ingress in networking.k8s.io/v1beta1 is available from from 1.14, but Ingress is available in networking.k8s.io/v1 from 1.19+ only.

Copy link
Member

@aanm aanm Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So where is the networking.k8s.io/v1beta1? I see extensions/v1beta1 and networking.k8s.io/v1 but not networking.k8s.io/v1beta1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aanm if I understand correctly you want to use latest api version in relation to each Kubernetes version?
Right now this works fine, but if needed we can have better versioning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm trying to says is that extensions/v1beta1 will be used for up until k8s 1.19 for non-ingress resources, why is that? Non-ingress resources can use networking.k8s.io/v1 since 1.14.

Copy link
Contributor Author

@youssefazrak youssefazrak Oct 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aanm I've introduced networking.k8s.io/v1beta1 starting from 1.14 as documented here. I didn't change the field for the ingress as they only changed from networking.k8s.io/v1.
Let me know if there is anything else.

@sayboras sayboras requested review from a team October 21, 2020 23:01
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nothing else besides what Tam pointed out.

Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modulo @sayboras' comments, LGTM.

For other reviewers/reference: helm/charts#19242

@youssefazrak youssefazrak force-pushed the helm_hubble_ingress_fix branch 2 times, most recently from b20ff21 to 8df6073 Compare October 22, 2020 13:46
@@ -9,9 +9,9 @@ Create chart name and version as used by the chart label.
Return the appropriate apiVersion for ingress.
*/}}
{{- define "ingress.apiVersion" -}}
{{- if semverCompare ">=1.4-0, <1.14-0" .Capabilities.KubeVersion.GitVersion -}}
{{- if semverCompare ">=1.4-0, <1.19-0" .Capabilities.KubeVersion.Version -}}
Copy link
Member

@aanm aanm Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So where is the networking.k8s.io/v1beta1? I see extensions/v1beta1 and networking.k8s.io/v1 but not networking.k8s.io/v1beta1.

Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💯

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Ingress and IngressClass resources have graduated to networking.k8s.io/v1
in Kubernetes v1.19. This also introduced changes to the objects.

Using Helm `.Capabilities`, the correct fields are chosen.
Also in Helm, `GitVersion` is being deprecated and has thus been
replaced with `Version`.

See Cilium issue cilium#13611
See Kubernetes issue #89778

Signed-off-by: Youssef Azrak <yazrak.tech@gmail.com>
@youssefazrak youssefazrak force-pushed the helm_hubble_ingress_fix branch from 8df6073 to fc8a029 Compare October 24, 2020 10:03
@aanm aanm added needs-backport/1.9 release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Oct 24, 2020
@aanm aanm merged commit aec71a9 into cilium:master Oct 24, 2020
@youssefazrak youssefazrak deleted the helm_hubble_ingress_fix branch March 19, 2021 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Impacts helm charts and user deployment experience release-note/bug This PR fixes an issue in a previous release of Cilium. sig/hubble Impacts hubble server or relay
Projects
None yet
Development

Successfully merging this pull request may close these issues.

helm: 1.9.0rc1 hubble.ui.ingress and k8s 1.19 not working together
7 participants