Closed Szpadel closed 1 year ago
I have three ingress classes (all nginx ingresses) on my home lab cluster, only one was impacted by this.
Looks like related to https://github.com/kubernetes/ingress-nginx/pull/9543
Looks like related to #9543
It looks like you are correct
Both my broken ingress classes had pathType: ImplementationSpecific
and changing it to Prefix
resolved the issue
Can confirm. Happened to my clusters as well. Pinned the chart version to 4.4.2 for now
Affected by this as well.. However all my pathTypes are set to Prefix. Going back to 4.4.2 for now.
Can confirm, pinned version 4.4.2
@strongjz can you check this please? this change #9543 somehow broke it.
I just pinned to previous version 4.4.2 in all my clusters.
well, I think this chart version shouldn't have been created yet because of the many changes inside: https://github.com/kubernetes/ingress-nginx/commit/d80d4d4eca42ff9ec6ad231f187df454eec85321 version changed from 1.5.1 to 1.6.1 and more...
We can confirm this bug on all our nginx ingress controllers
Please don't release this as a minor chart update
The chart we released by accident and was not ready for consumption; please pin your charts to 4.4.2. I apologize for this confusion and the issues this caused. The CI was updated to add linting and testing before a release/push to the main branch. And I agree the ingress-controller version was bumped to 1.6.1 but only a minor on the chart.
The implementation-specific changes should also have the path validation boolean on them.
Thank you for confirming and working on remediations. #HugOps
/reopen
The pr automatically closed, I'll ;eave it open till 1.6.2 is rolled out.
@strongjz: Reopened this issue.
/priority critical-urgent /triage accepted /kind bug
1.6.2 is building, once its completed, we move it to production k8s registry then we can release a new controller and chart
I too experienced the crash loops. Perhaps an opportunity to improve the E2E testing pipeline? 😄
I do have a question for those who are crashing, do you have the admission controller turned on? It should validate the path for those using implementation specific.
the e2e for turning on path validation will return an error if the implementation specific had regex on it not included by default or by the user. This all was going to be sent out in kdev email and EnablePathValidation is set to false by default. It would be helpful if you provided the ingress objects that caused a failure.
It would be helpful if you provided the ingress objects that caused a failure.
Yes, please provide some Ingress objects so that we can confirm that the issue is indeed resolved, thanks
all my ingresses are like the following one:
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
name: api
spec:
ingressClassName: nginx
rules:
- host: api.example.com
http:
paths:
- backend:
service:
name: api
port:
name: http-main
path: /
pathType: Prefix
I manually/locally added @ricosega test
{
name: "should accept valid path on pathType Prefix on slash",
wantErr: false,
EnablePathTypeValidation: false,
copyIng: generateDumbIngressforPathTest(&pathTypePrefix, "/"),
},
And the go unit tests passed
--- PASS: TestValidateIngressPath (0.00s)
--- PASS: TestValidateIngressPath/should_return_nil_when_ingress_=_nil (0.00s)
--- PASS: TestValidateIngressPath/should_accept_valid_path_on_pathType_Exact (0.00s)
--- PASS: TestValidateIngressPath/should_accept_valid_path_on_pathType_Prefix (0.00s)
--- PASS: TestValidateIngressPath/should_accept_valid_path_on_pathType_Prefix_on_slash (0.00s)
--- PASS: TestValidateIngressPath/should_accept_valid_simple_path_on_pathType_Impl_Specific (0.00s)
--- PASS: TestValidateIngressPath/should_accept_valid_path_on_pathType_nil (0.00s)
--- PASS: TestValidateIngressPath/should_accept_empty_path (0.00s)
--- PASS: TestValidateIngressPath/should_deny_path_with_bad_characters_and_pathType_not_implementationSpecific (0.00s)
--- PASS: TestValidateIngressPath/should_accept_path_with_regex_characters_and_pathType_implementationSpecific (0.00s)
--- PASS: TestValidateIngressPath/should_accept_path_with_regex_characters_and_pathType_exact,_but_pathType_validation_disabled (0.00s)
--- PASS: TestValidateIngressPath/should_reject_path_when_the_allowed_additional_set_does_not_match (0.00s)
--- PASS: TestValidateIngressPath/should_accept_path_when_the_allowed_additional_set_does_match (0.00s)
--- PASS: TestValidateIngressPath/should_block_if_at_least_one_path_is_bad (0.00s)
--- PASS: TestValidateIngressPath/should_block_if_at_least_one_path_is_bad#01 (0.00s)
I do have a question for those who are crashing, do you have the admission controller turned on? It should validate the path for those using implementation specific.
In my case, it was on (at least it should be as I didn't explicitly have it disabled in the helm values). It looks like it has been out there for a while, too:
❯ k -n kube-system get ValidatingWebhookConfiguration ingress-nginx-admission
NAME WEBHOOKS AGE
ingress-nginx-admission 1 2y78d
TLDR: only ingresses with ImplementationSpecific type were affected.
I have two controllers and classes: public and private (eks and alb controller).
So only few of private ingresses were set following:
path: /
pathType: ImplementationSpecific
And all public had type Prefix. As a result of chart auto update (which I guess I really do need to disable) - public controller updated just fine but private failed to update because new pod went into crash loop with error described in this ticket. Fix was to manually change all ingresses to Prefix and restart pod and update again.
Haven't tested though what would happen if path was set to smth like /* - I believe that's the intended usage and valid case for ImplementationSpecific?
TLDR: only ingresses with ImplementationSpecific type were affected.
I would have sworn up and down that I only had Prefix
types. Alas, my affected staging cluster:
[17:27:27][cayla@wopr]% k get ing -A -o yaml | grep pathType: | sort | uniq -c
1 pathType: ImplementationSpecific
627 pathType: Prefix
Must be some odd 3rd party tool that snuck it in there 😠
EDIT: it was logstash in my case.
I've mentioned it as well, all my ingress are pathType: Prefix
(https://github.com/kubernetes/ingress-nginx/issues/9569#issuecomment-1413371485). Also this: https://github.com/kubernetes/ingress-nginx/issues/9569#issuecomment-1414391704 says the same.
Maybe there's something else going on?
Longhorn at least seems to be using ImplementationSpecific
in the UI ingress.
I too experienced the crash loops. Perhaps an opportunity to improve the E2E testing pipeline? smile
The E2E for this is on in the admission controller, so if folks have that disabled it would cause issues. We may need to add checks in the controller so it doesnt actually crash.
The E2E for this is on in the admission controller, so if folks have that disabled it would cause issues. We may need to add checks in the controller so it doesnt actually crash.
I faced a race condition between the admission controller and the pods that were crashing (as it wasn't able to process the validations due to crashing pods). The error log on the crashed pods didin't tell me what the issue is so it would be good if it also included the ingress name that's causing it to crash.
I've mentioned it as well, all my ingress are
pathType: Prefix
(#9569 (comment)). Also this: #9569 (comment) says the same.Maybe there's something else going on?
the issue should be on implementationspecific, as that didnt have the enable pathtype check.
Exact and Prefix would have failed to be admitted if the pathtypevalidation was enabled and it had regex chars https://github.com/kubernetes/ingress-nginx/blob/main/pkg/util/ingress/ingress.go#L294
regex is checked, and is controlled by
// Defaults to: "^%$[](){}*+?"
PathAdditionalAllowedChars
/*
/* is in the default of PathAdditionalAllowedChars
so yes it should have been allowed, even with validation.
I do have a question for those who are crashing, do you have the admission controller turned on? It should validate the path for those using implementation specific.
In my case, it was on (at least it should be as I didn't explicitly have it disabled in the helm values). It looks like it has been out there for a while, too:
❯ k -n kube-system get ValidatingWebhookConfiguration ingress-nginx-admission NAME WEBHOOKS AGE ingress-nginx-admission 1 2y78d
That is...interesting. How do you to your deployments?
@tbondarchuk not only ingresses with ImplementationSpecific type were affected, all mine are Prefix and also crashes as I said before.
@strongjz this is my values.yaml config
controller:
kind: DaemonSet
metrics:
enabled: true
serviceMonitor:
enabled: false
additionalLabels:
release: kube-prometheus-stack
podAnnotations:
prometheus.io.scrape: 'true'
prometheus.io.port: '10254'
service:
type: NodePort
resources:
limits:
cpu: 200m
memory: 200Mi
requests:
cpu: 200m
memory: 200Mi
config:
client-body-buffer-size: "100M"
client-header-timeout: "3600"
client_body_timeout: "3600"
enable-underscores-in-headers: "true"
keep-alive: "65"
proxy-body-size: "100M"
proxy-buffering: "off"
proxy-read-timeout: "3600"
proxy-redirect: "off"
proxy-redirect-from: "off"
proxy-send-timeout: "3600"
server-name-hash-bucket-size: "256"
server-name-hash-max-size: "512"
server-snippet: |
add_header X-Frame-Options DENY;
add_header X-Content-Type-Options 'nosniff';
add_header X-XSS-Protection '1; mode=block' ;
add_header Strict-Transport-Security 'max-age=31536000; includeSubDomains; preload';
add_header Content-Security-Policy 'frame-ancestors \'self\' https://*.s3.eu-west-1.amazonaws.com;';
server-tokens: "false"
use-forwarded-headers: "true"
use-proxy-protocol: "false"
so the rest is by the default values and the controller is enabled in my case:
$ k -n kube-system get ValidatingWebhookConfiguration ingress-nginx-admission
NAME WEBHOOKS AGE
ingress-nginx-admission 1 17d
and here is the error log:
I0202 12:55:18.122134 7 event.go:285] Event(v1.ObjectReference{Kind:"Pod", Namespace:"ingress-nginx", Name:"ingress-nginx-controller-r9sb5", UID:"d54a11c3-14ea-474c-843e-c40891f04952", APIVersion:"v1", ResourceVersion:"34418200", FieldPath:""}): type: 'Warning' reason: 'RELOAD' Error reloading NGINX:
-------------------------------------------------------------------------------
Error: exit status 1
2023/02/02 12:55:16 [warn] 126#126: the "http2_max_field_size" directive is obsolete, use the "large_client_header_buffers" directive instead in /tmp/nginx/nginx-cfg352643530:150
nginx: [warn] the "http2_max_field_size" directive is obsolete, use the "large_client_header_buffers" directive instead in /tmp/nginx/nginx-cfg352643530:150
2023/02/02 12:55:16 [warn] 126#126: the "http2_max_header_size" directive is obsolete, use the "large_client_header_buffers" directive instead in /tmp/nginx/nginx-cfg352643530:151
nginx: [warn] the "http2_max_header_size" directive is obsolete, use the "large_client_header_buffers" directive instead in /tmp/nginx/nginx-cfg352643530:151
2023/02/02 12:55:16 [warn] 126#126: the "http2_max_requests" directive is obsolete, use the "keepalive_requests" directive instead in /tmp/nginx/nginx-cfg352643530:152
nginx: [warn] the "http2_max_requests" directive is obsolete, use the "keepalive_requests" directive instead in /tmp/nginx/nginx-cfg352643530:152
2023/02/02 12:55:16 [emerg] 126#126: duplicate location "/" in /tmp/nginx/nginx-cfg352643530:6080
nginx: [emerg] duplicate location "/" in /tmp/nginx/nginx-cfg352643530:6080
nginx: configuration file /tmp/nginx/nginx-cfg352643530 test failed
-------------------------------------------------------------------------------
I0202 12:55:19.092376 7 controller.go:188] "Configuration changes detected, backend reload required"
@tbondarchuk not only ingresses with ImplementationSpecific type were affected, all mine are Prefix and also crashes as I said before.
@strongjz this is my values.yaml config
controller: kind: DaemonSet metrics: enabled: true serviceMonitor: enabled: false additionalLabels: release: kube-prometheus-stack podAnnotations: prometheus.io.scrape: 'true' prometheus.io.port: '10254' service: type: NodePort resources: limits: cpu: 200m memory: 200Mi requests: cpu: 200m memory: 200Mi config: client-body-buffer-size: "100M" client-header-timeout: "3600" client_body_timeout: "3600" enable-underscores-in-headers: "true" keep-alive: "65" proxy-body-size: "100M" proxy-buffering: "off" proxy-read-timeout: "3600" proxy-redirect: "off" proxy-redirect-from: "off" proxy-send-timeout: "3600" server-name-hash-bucket-size: "256" server-name-hash-max-size: "512" server-snippet: | add_header X-Frame-Options DENY; add_header X-Content-Type-Options 'nosniff'; add_header X-XSS-Protection '1; mode=block' ; add_header Strict-Transport-Security 'max-age=31536000; includeSubDomains; preload'; add_header Content-Security-Policy 'frame-ancestors \'self\' https://*.s3.eu-west-1.amazonaws.com;'; server-tokens: "false" use-forwarded-headers: "true" use-proxy-protocol: "false"
so the rest is by the default values and the controller is enabled in my case:
$ k -n kube-system get ValidatingWebhookConfiguration ingress-nginx-admission NAME WEBHOOKS AGE ingress-nginx-admission 1 17d
and here is the error log:
I0202 12:55:18.122134 7 event.go:285] Event(v1.ObjectReference{Kind:"Pod", Namespace:"ingress-nginx", Name:"ingress-nginx-controller-r9sb5", UID:"d54a11c3-14ea-474c-843e-c40891f04952", APIVersion:"v1", ResourceVersion:"34418200", FieldPath:""}): type: 'Warning' reason: 'RELOAD' Error reloading NGINX: ------------------------------------------------------------------------------- Error: exit status 1 2023/02/02 12:55:16 [warn] 126#126: the "http2_max_field_size" directive is obsolete, use the "large_client_header_buffers" directive instead in /tmp/nginx/nginx-cfg352643530:150 nginx: [warn] the "http2_max_field_size" directive is obsolete, use the "large_client_header_buffers" directive instead in /tmp/nginx/nginx-cfg352643530:150 2023/02/02 12:55:16 [warn] 126#126: the "http2_max_header_size" directive is obsolete, use the "large_client_header_buffers" directive instead in /tmp/nginx/nginx-cfg352643530:151 nginx: [warn] the "http2_max_header_size" directive is obsolete, use the "large_client_header_buffers" directive instead in /tmp/nginx/nginx-cfg352643530:151 2023/02/02 12:55:16 [warn] 126#126: the "http2_max_requests" directive is obsolete, use the "keepalive_requests" directive instead in /tmp/nginx/nginx-cfg352643530:152 nginx: [warn] the "http2_max_requests" directive is obsolete, use the "keepalive_requests" directive instead in /tmp/nginx/nginx-cfg352643530:152 2023/02/02 12:55:16 [emerg] 126#126: duplicate location "/" in /tmp/nginx/nginx-cfg352643530:6080 nginx: [emerg] duplicate location "/" in /tmp/nginx/nginx-cfg352643530:6080 nginx: configuration file /tmp/nginx/nginx-cfg352643530 test failed ------------------------------------------------------------------------------- I0202 12:55:19.092376 7 controller.go:188] "Configuration changes detected, backend reload required"
Can you share your ingress objects in a gist or a dm on slack.
Still do not understand why on other path types regex pattern has to be checked when enable-path validation is false. https://github.com/kubernetes/ingress-nginx/blob/0a0a3fe258cacd79d6c966c8126421de653e484c/pkg/util/ingress/ingress.go#L302
Also if the user has to update regex list, then it cannot be a patch release.
in my case it was
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
annotations:
cert-manager.io/cluster-issuer: letsencrypt
name: example
spec:
rules:
- host: example.com
http:
paths:
- backend:
service:
name: nginx
port:
name: http
path: /
pathType: ImplementationSpecific
tls:
- hosts:
- example.com
secretName: example.com-tls
The chart we released by accident and was not ready for consumption; please pin your charts to 4.4.2.
@strongjz could you please delete the faulty chart version 4.4.3 from the Helm repo index? Most Flux users have patch upgrades enabled and this broke lots of clusters.
This is indeed a quick fix. Many people can be prevented from being affected.
I'll start working on it
remove 4.4.3 Helm chart . https://github.com/kubernetes/ingress-nginx/pull/9576
tao@moelove:~$ helm repo update ingress-nginx
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "ingress-nginx" chart repository
Update Complete. ⎈Happy Helming!⎈
tao@moelove:~$ helm search repo ingress-nginx
NAME CHART VERSION APP VERSION DESCRIPTION
ingress-nginx/ingress-nginx 4.4.2 1.5.1 Ingress controller for Kubernetes using NGINX a...
Thanks @tao12345666333 🥇
@strongjz https://gist.github.com/tbondarchuk/cc5ff7111871c0a1e2a273fb36b069b2
@ricosega your ingress are set to port name, mine to port number. perhaps issue is triggered by port.name + Prefix and by ImplementationSpecific ?
P.S. Edit: tagging correct user
@ricosega just tested with port.number => port.name - works properly with Prefix on 4.4.3, controller 1.6.1 🤷 Perhaps you have some sneaky ingress with ImplementationSpecific somewhere? I had kubernetes-dashboard creating such ingress by default, all others (prometheus charts) were set explicitly by me in values files.
@strongjz so since I'm still have 4.4.3 installed did a few quick tests:
path: /
pathType: ImplementationSpecific
causes constant errors in controller logs:
nginx: [emerg] duplicate location "/" in /tmp/nginx/nginx-cfg950722431:1289
nginx: configuration file /tmp/nginx/nginx-cfg950722431 test failed
path: /*
pathType: ImplementationSpecific
@ricosega could you maybe dump /tmp/nginx/nginx-cfg*
file and check for with domain this was duplicated and then check ingress for it?
https://github.com/kubernetes/ingress-nginx/pull/9576
% helm search repo ingress-nginx
NAME CHART VERSION APP VERSION DESCRIPTION
ingress-nginx/ingress-nginx 4.4.3 1.6.1 Ingress controller for Kubernetes using NGINX a...
[~]
% helm repo update
Hang tight while we grab the latest from your chart repositories...
...Successfully got an update from the "ingress-nginx" chart repository
Update Complete. ⎈Happy Helming!⎈
[~]
% helm search repo ingress-nginx
NAME CHART VERSION APP VERSION DESCRIPTION
ingress-nginx/ingress-nginx 4.4.2 1.5.1 Ingress controller for Kubernetes using NGINX a...
[~]
/close
@longwuyuan: Closing this issue.
already checked it, its been for the kubernetes-dashboard as @tbondarchuk said. Is the only one with ImplementationSpecific, my apologies for the confusion.
just in case somebody else needs it - here's how to override prefix type for kubernetes-dashboard chart:
ingress:
enabled: true
annotations:
dns.alpha.kubernetes.io/external: "true"
cert-manager.io/cluster-issuer: self-signed
className: nginx-private
hosts:
- dashboard.example.com
tls:
- secretName: kubernetes-dashboard-tls
hosts:
- dashboard.example.com
customPaths:
- path: /
pathType: Prefix
backend:
service:
name: kubernetes-dashboard
port:
number: 443
/reopen
The pr automatically closed, I'll ;eave it open till 1.6.2 is rolled out.
I will reopen this one, until we get the problem out of the way @longwuyuan /reopen
@tao12345666333: Reopened this issue.
What happened:
generated nginx config adds
location /
blocks for some cases twice, config does not pass validation and ingress stucks in crash loop. It found this only for 2 domains (ingresses), one of them have basic auth and letsencrypt tls, and the other just letsencrypt (most basic setup), other similar ingresses were not affectedReverting to 4.4.2 resolves issue
What you expected to happen: It should not crash
NGINX Ingress controller version (exec into the pod and run nginx-ingress-controller --version.):
Kubernetes version (use
kubectl version
):Environment:
helm ls -A | grep -i ingress
helm -n <ingresscontrollernamepspace> get values <helmreleasename>