Update chart requirements #6167
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
approved
|
/hold |
k8s-ci-robot
added
the
do-not-merge/hold
|
/assign @ChiefAlexander |
| @@ -12,10 +12,10 @@ webhooks: | |||
| - name: validate.nginx.ingress.kubernetes.io | |||
| rules: | |||
| - apiGroups: | |||
| - extensions | |||
| - networking.k8s.io | |||
| apiVersions: | |||
| - v1beta1 | |||
There was a problem hiding this comment.
cant you remove this as well?
There was a problem hiding this comment.
No, if we want to support k8s between 1.16 and 1.19
| clientConfig: | ||
| service: | ||
| namespace: {{ .Release.Namespace }} | ||
| name: {{ include "ingress-nginx.controller.fullname" . }}-admission | ||
| path: /extensions/v1beta1/ingresses | ||
| path: /networking/v1beta1/ingresses |
There was a problem hiding this comment.
is this still v1beta1? My short googling failed me in finding that answer.
There was a problem hiding this comment.
v1 is available since v1.19.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, ChiefAlexander The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
New changes are detected. LGTM label has been removed. |
k8s-ci-robot
removed
the
lgtm
| @@ -293,7 +293,7 @@ controller: | |||
| ## Enable mimalloc as a drop-in replacement for malloc. | |||
| ## ref: https://github.com/microsoft/mimalloc | |||
| ## | |||
| enableMimalloc: false | |||
| enableMimalloc: true | |||
There was a problem hiding this comment.
@aledbf was there any special reason to enable mimalloc by default as part of raising min requirement to k8s 1.16 APIs?
If not, wish enabling mimalloc by default was a separate PR (or part of default config changes PR) and had changelog entry. I guess changelog entry can be added still for previous release.
There was a problem hiding this comment.
Thinking again, I guess it makes sense, default config changes were for ingress-nginx binary defaults, while chart defaults are separate.
And also grouping multiple chart default changes into one PR is consistent with grouping multiple binary config defaults changes in one PR.
| @@ -12,10 +12,10 @@ webhooks: | |||
| - name: validate.nginx.ingress.kubernetes.io | |||
| rules: | |||
| - apiGroups: | |||
| - extensions | |||
There was a problem hiding this comment.
@aledbf what's rationale to drop validating Ingresses with extensions/v1beta1? networking.k8s.io is available since k8s 1.14 but while extensions is supported why not keep validating those too? Ingress extensions/v1beta1 API will be dropped only in v1.22 (see https://kubernetes.io/blog/2019/07/18/api-deprecations-in-1-16/)
There was a problem hiding this comment.
@sslavic because the client-go v1.19 removed the extensions package.
If you have ingresses in the old extensions/v1beta1 will be converted to networking.k8s.io, the version used in ingress-nginx, by the API server
There was a problem hiding this comment.
but while extensions is supported why not keep validating those too?
Because networking.k8s.io/v1 only works in v1.19 and we need to support 1.16, 1.17, 1.18, and 1.19
(not sure we can keep doing this long term)
What this PR does / why we need it:
jettech/kube-webhook-certgentov1.3.0Types of changes
Which issue/s this PR fixes
How Has This Been Tested?
Checklist: