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

Upgrade/kubernetes v1.22 #171

Merged
merged 9 commits into from
Mar 31, 2022

Conversation

nikokon
Copy link
Contributor

@nikokon nikokon commented Dec 25, 2021

Was setting up dev environment in order to get cracking on implementing some features when I noticed I was having trouble deploying a new app with ingress in my local minikube cluster.

Some of the beta-apis have been deprecated in kubernetes v1.22 and started following this "guide".

https://kubernetes.io/blog/2021/07/14/upcoming-changes-in-kubernetes-1-22/

I think there are some more charts that need updating. mongodb, porteragent, prometheus for example use the "authorization.k8s.io/v1beta1"

While this PR does not adress those, I wanted to submit it as I'm not that experienced in best practices etc when it comes to versioning and deprecations using helm.

When do we allow breaking changes? when do we stop supporting kubernetes EOL versions etc? 🤷

{{- if .Capabilities.APIVersions.Has "networking.k8s.io/v1" }}
apiVersion: networking.k8s.io/v1
{{- else if .Capabilities.APIVersions.Has "networking.k8s.io/v1beta" -}}
apiVersion: networking.k8s.io/v1beta
{{- else -}}
apiVersion: extensions/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.

was this removed in v1.14? when should we remove this from the chart definition? :)
I'm just keeping it as a defense mechanism but would love to learn of what our thoughts are regarding this.

@lenn4rd
Copy link

lenn4rd commented Feb 1, 2022

Anything I can do to expedite this and finish up this pull request so it can be merged into Porter?

@nikokon
Copy link
Contributor Author

nikokon commented Feb 8, 2022

Anything I can do to expedite this and finish up this pull request so it can be merged into Porter?

I could have another look and just clean it up a bit.
@abelanger5 or anyone from porter have any input on this? With kubernetes release and deprecation schedule this issue must be quite pressing?

There are other templates in the repo that needs updating as I mentioned in my original comment

@abelanger5
Copy link
Contributor

Hi @nikokon, thanks for this PR and apologies for the delay in response.

Requested @rudimk to review this PR and test that it's backwards-compatible.

@nikokon
Copy link
Contributor Author

nikokon commented Feb 9, 2022

No worries! :)

@rudimk
Copy link
Contributor

rudimk commented Feb 21, 2022

Hey @nikokon - so very sorry I haven't been able to get around to reviewing this. I'll be sure to take care of it either today or early tomorrow. Thanks ever so much!

@nikokon
Copy link
Contributor Author

nikokon commented Mar 17, 2022

ping @rudimk :)

@rudimk
Copy link
Contributor

rudimk commented Mar 22, 2022

Hey @nikokon - sitting on this today, the aim is to spin up a couple of testbed clusters running K8s v1.22 and test your changes out.

Copy link
Contributor

@abelanger5 abelanger5 left a comment

Choose a reason for hiding this comment

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

Hey @nikokon, thanks again for this PR, and apologies for the delay. We've been extremely busy with version deprecations on various cloud providers, so it's been hard to prioritize 1.22 until now. That said, we are targeting full support on 1.22 across all addons in the next couple weeks, starting with the web chart.

I've added a code review, please let me know if there are any questions :)

To answer your query:

When do we allow breaking changes? when do we stop supporting kubernetes EOL versions etc? 🤷

I think in our case we will remove support for the old extensions/v1beta1 once 1.21 has officially been deprecated by the major providers (EKS, GKE, DOKS). This is a much longer timeline than the Kubernetes deprecation schedule but our primary concern is supporting users on the cloud providers, who might not upgrade their clusters until the last minute.

applications/web/templates/ingress.yaml Outdated Show resolved Hide resolved
applications/web/templates/ingress.yaml Outdated Show resolved Hide resolved
applications/web/templates/ingress.yaml Outdated Show resolved Hide resolved
applications/web/templates/ingress.yaml Outdated Show resolved Hide resolved
applications/web/templates/ingress.yaml Outdated Show resolved Hide resolved
applications/web/templates/ingress.yaml Outdated Show resolved Hide resolved
nikokon and others added 5 commits March 29, 2022 20:44
Co-authored-by: abelanger5 <alexander.b138@gmail.com>
Co-authored-by: abelanger5 <alexander.b138@gmail.com>
Co-authored-by: abelanger5 <alexander.b138@gmail.com>
Co-authored-by: abelanger5 <alexander.b138@gmail.com>
@nikokon nikokon marked this pull request as ready for review March 29, 2022 18:53
@nikokon
Copy link
Contributor Author

nikokon commented Mar 29, 2022

hey @abelanger5 I applied your suggestions from the code review. took a while before I realized I could do it in bulk 🥲
First time I've come across that functionality, with code suggestions ready to be applied in the review process. Really nice!

Feel free to merge if all looks good!

Copy link
Contributor

@abelanger5 abelanger5 left a comment

Choose a reason for hiding this comment

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

Hey @nikokon, I've added a few more code review comments after testing on various clusters.

applications/web/templates/ingress.yaml Outdated Show resolved Hide resolved
applications/web/templates/ingress.yaml Outdated Show resolved Hide resolved
applications/web/templates/ingress.yaml Outdated Show resolved Hide resolved
servicePort: {{ .servicePort }}
{{- end }}
{{- end }}
{{ else }}
- backend:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- backend:

I think this lingering backend: block is throwing things off. Here's the block that worked for me:

            - {{- if $.Capabilities.APIVersions.Has "networking.k8s.io/v1/Ingress" }}
              pathType: ImplementationSpecific
              {{- end }}
              backend:
              {{- if $.Capabilities.APIVersions.Has "networking.k8s.io/v1/Ingress" }}
                service:
                  name: {{ $fullName }}
                  port:
                    number: {{ $svcPort }}
              {{- else }}
                serviceName: {{ $fullName }}
                servicePort: {{ $svcPort }}
              {{- end }}

@abelanger5
Copy link
Contributor

Additionally -- upgrades once a user is on 1.22 will throw the following error:

UPGRADE FAILED: current release manifest contains removed kubernetes api(s)
for this kubernetes version and it is therefore unable to build the kubernetes
objects for performing the diff

Even though GKE will support the deprecated extensions/v1beta1 API until 1.23, this error will still be thrown when the user attempts to upgrade.

According to the Helm docs (https://helm.sh/docs/topics/kubernetes_apis/#helm-users), we will need to patch an existing Helm release with the new API versions before the user can upgrade. As a result, we cannot merge this in until we're able to perform this patch from the Porter server. Hopefully this won't take too long to get added.

Co-authored-by: abelanger5 <alexander.b138@gmail.com>
@nikokon
Copy link
Contributor Author

nikokon commented Mar 31, 2022

@abelanger5
I've commited your suggestions.

@abelanger5 abelanger5 merged commit 84d3605 into porter-dev:master Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants