Add support for IPv6 #2576
Add support for IPv6 #2576
Conversation
There was a problem hiding this comment.
Hi @jjngx
Please see my comments and suggestions.
Note that the review doesn't cover the changes added to the branch and covered in this PR #2577
Could we also double check that the examples that provision NGINX backends or configure NGINX using snippets (grep -R "listen" examples) also work in IPv6 mode?
build/Dockerfile
Outdated
| @@ -50,7 +50,7 @@ RUN --mount=type=secret,id=nginx-repo.crt,dst=/etc/ssl/nginx/nginx-repo.crt,mode | |||
| && apt-get install -y zlib1g \ | |||
| && curl -fsSL https://cs.nginx.com/static/keys/nginx_signing.key | gpg --dearmor > /etc/apt/trusted.gpg.d/nginx_signing.gpg \ | |||
| && curl -fsSL -o /etc/apt/apt.conf.d/90pkgs-nginx https://cs.nginx.com/static/files/90pkgs-nginx \ | |||
| && DEBIAN_VERSION=$(awk -F '=' '/^VERSION_CODENAME=/ {print $2}' /etc/os-release) \ | |||
| && DEBIAN_VERSION=bullseye \ | |||
There was a problem hiding this comment.
Can we revert this change?
build/Dockerfile
Outdated
| @@ -59,7 +59,16 @@ RUN --mount=type=secret,id=nginx-repo.crt,dst=/etc/ssl/nginx/nginx-repo.crt,mode | |||
| && rm -rf /var/lib/apt/lists/* | |||
|
|
|||
|
|
|||
| ############################################# Base image for Debian with NGINX Plus and App Protect WAF/DoS ############################################# | |||
There was a problem hiding this comment.
Do we need this commented out part?
There was a problem hiding this comment.
@pleshakov commented out code removed, Dockerfile cleaned
There was a problem hiding this comment.
I think there's still something left in line 62
| @@ -197,6 +199,7 @@ http { | |||
| # NGINX Plus APIs | |||
| server { | |||
| listen {{.NginxStatusPort}}; | |||
| listen [::]:{{.NginxStatusPort}}; | |||
There was a problem hiding this comment.
I think it makes sense to change the default value of https://github.com/nginxinc/kubernetes-ingress/blob/main/cmd/nginx-ingress/main.go#L134 to include the IPV6 localhost. That value is used in this server in the allow directive:
{{range $value := .NginxStatusAllowCIDRs}}
allow {{$value}};{{end}}
deny all;
location /api {
api write=off;
}
currently, the default value is 127.0.0.1.
this endpoint is used to access the dashboard https://docs.nginx.com/nginx-ingress-controller/logging-and-monitoring/status-page/#accessing-live-activity-monitoring-dashboard
same for NGINX OSS.
|
|
||
| {{if .TLSPassthrough}} | ||
| listen unix:/var/lib/nginx/passthrough-https.sock ssl default_server{{if .HTTP2}} http2{{end}} proxy_protocol; | ||
| set_real_ip_from unix:; | ||
| real_ip_header proxy_protocol; | ||
| {{else}} | ||
| listen 443 ssl default_server{{if .HTTP2}} http2{{end}}{{if .ProxyProtocol}} proxy_protocol{{end}}; | ||
| listen [::]:443 ssl default_server{{if .HTTP2}} http2{{end}}{{if .ProxyProtocol}} proxy_protocol{{end}}; | ||
| {{end}} | ||
|
|
||
| {{if .SSLRejectHandshake}} |
There was a problem hiding this comment.
listen directive in {{- if .NginxStatus}} needs to be updated similarly to NGINX Plus.
tests/kind/kind-config-template.yaml
Outdated
| networking: | ||
| ipFamily: {{.IPFamily}} | ||
| apiServerAddress: {{.APIServerAddress}} | ||
| #apiServerPort: 6443 |
There was a problem hiding this comment.
do we need this parameter commented out? perhaps better to remove all together?
Codecov Report
@@ Coverage Diff @@
## main #2576 +/- ##
==========================================
- Coverage 53.50% 53.49% -0.02%
==========================================
Files 49 49
Lines 14314 14299 -15
==========================================
- Hits 7659 7649 -10
+ Misses 6412 6407 -5
Partials 243 243
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
Proposed changes
Add support for IPv6
Checklist
Before creating a PR, run through this checklist and mark each as complete.