ingress-nginx icon indicating copy to clipboard operation
ingress-nginx copied to clipboard

NGINX: Add `nginx-go-crossplane` rendering.

Open rikatz opened this issue 1 year ago • 14 comments

What this PR does / why we need it:

This is the Xnd part of moving to use nginx go crossplane. Now adding mirrors, custom error, initial server bootstrap, etc Also stream and mod_security are removed from here.

rikatz avatar Sep 15 '24 22:09 rikatz

Deploy Preview for kubernetes-ingress-nginx canceled.

Name Link
Latest commit 290943419d7a5fb2f00133628d15fa057252677c
Latest deploy log https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/67e9ae3f03b1dc000828b17e

netlify[bot] avatar Sep 29 '24 23:09 netlify[bot]

Missing stuff also:

geo $remote_addr $allowlist_{{ $rl.ID }} { default 0; {{ range $ip := $rl.Allowlist }} {{ $ip }} 1;{{ end }} }

# Ratelimit {{ $rl.Name }}
map $allowlist_{{ $rl.ID }} $limit_{{ $rl.ID }} {
    0 {{ $cfg.LimitConnZoneVariable }};
    1 "";
}

{{ range $zone := (buildRateLimitZones $servers) }}
{{ $zone }}
{{ end }}

auth_upstream

rikatz avatar Oct 06 '24 20:10 rikatz

All failing tests now are related to auth, but the "http2-push-preload" one that was removed because it was deprecated and needs to be checked.

rikatz avatar Oct 14 '24 23:10 rikatz

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: rikatz / name: Ricardo Katz (290943419d7a5fb2f00133628d15fa057252677c)

Fixed auth, but external auth which needs some rework on lua side:

Summarizing 29 Failures:
  [FAIL] [Setting] [Security] global-auth-url when global external authentication is configured [It] should add custom error page when global-auth-signin url is configured
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] satisfy [It] should allow multiple auth with satisfy any
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication with caching is configured [BeforeEach] should redirect to signin url when not signed in
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication is configured [BeforeEach] should not create additional upstream block when auth-keepalive is negative
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Setting] [Security] global-auth-url when global external authentication is configured [It] should set snippet when global external auth is configured
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication is configured [BeforeEach] should create additional upstream block when auth-keepalive is set with HTTP/1.x
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication is configured with a custom redirect param [BeforeEach] should redirect to signin url when not signed in
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication is configured [BeforeEach] should overwrite Foo header with auth response
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* [It] should set "proxy_set_header 'My-Custom-Header' '42';" when auth-headers are set
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication is configured with a custom redirect param [BeforeEach] should return status code 200 when signed in
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication is configured [BeforeEach] should not create additional upstream block when auth-keepalive is set with HTTP/2
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication is configured [BeforeEach] should return status code 200 when signed in
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication is configured with a custom redirect param [BeforeEach] keeps processing new ingresses even if one of the existing ingresses is misconfigured
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication is configured [BeforeEach] should not create additional upstream block when auth-keepalive is not set
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* [It] should set snippet "proxy_set_header My-Custom-Header 42;" when external auth is configured
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* cookie set by external authentication server [BeforeEach] user with annotated ingress retains cookie if upstream returns error status code
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Flag] custom HTTP and HTTPS ports with a TLS enabled ingress when external authentication is configured [It] should set the X-Forwarded-Port header to 443
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/ssl.go:149
  [FAIL] [Annotations] auth-* when external authentication is configured [BeforeEach] should disable set_all_vars when auth-keepalive-share-vars is not set
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication is configured [BeforeEach] should not create additional upstream block when host part of auth-url contains a variable
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Setting] [Security] global-auth-url when global external authentication is configured [It] should proxy_method method when global-auth-method is configured
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication with caching is configured [BeforeEach] should return status code 200 when signed in after auth backend is deleted
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication is configured [BeforeEach] should enable set_all_vars when auth-keepalive-share-vars is true
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication is configured [BeforeEach] should redirect to signin url when not signed in
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* cookie set by external authentication server [BeforeEach] user does not retain cookie if upstream returns error status code
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication is configured [BeforeEach] keeps processing new ingresses even if one of the existing ingresses is misconfigured
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication with caching is configured [BeforeEach] should deny login for different servers
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication with caching is configured [BeforeEach] should deny login for different location on same server
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* cookie set by external authentication server [BeforeEach] user retains cookie by default
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* with invalid auth-url should deny whole location [It] should add error to the config
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280

Ran 399 of 410 Specs in 3331.425 seconds
FAIL! -- 370 Passed | 29 Failed | 1 Flaked | 0 Pending | 11 Skipped

rikatz avatar Oct 28 '24 02:10 rikatz

latest:

Summarizing 5 Failures:
  [FAIL] [Setting] [Security] global-auth-url when global external authentication is configured [It] should proxy_method method when global-auth-method is configured
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Setting] [Security] global-auth-url when global external authentication is configured [It] should set snippet when global external auth is configured
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* when external authentication is configured [It] should overwrite Foo header with auth response
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* with invalid auth-url should deny whole location [It] should add error to the config
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280
  [FAIL] [Annotations] auth-* [It] should set "proxy_set_header 'My-Custom-Header' '42';" when auth-headers are set
  /go/src/k8s.io/ingress-nginx/test/e2e/framework/framework.go:280

Ran 397 of 410 Specs in 2585.659 seconds
FAIL! -- 392 Passed | 5 Failed | 0 Pending | 13 Skipped

rikatz avatar Nov 05 '24 15:11 rikatz

Could you maybe move the golang lint bump to a separate PR? In general it would be nice to have everything we could potentially cherry-pick to release branches in a separate PR.

Also: Can we postpone merging this after KubeCon? I'd really like to review it, but don't have the time now.

Gacko avatar Nov 13 '24 01:11 Gacko

/triage accepted /kind feature /priority backlog /hold

Gacko avatar Nov 13 '24 17:11 Gacko

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rikatz Once this PR has been reviewed and has the lgtm label, please assign cpanato for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot avatar Jan 12 '25 20:01 k8s-ci-robot

@strongjz as we have discussed

/hold cancel

rikatz avatar Jan 13 '25 10:01 rikatz

/hold

Gacko avatar Jan 13 '25 11:01 Gacko

There are still changes in this PR not related to its purpose. As we first want to get other PRs merged (adding NJS to the NGINX base image and migrating some Lua their) and probably a patch release out (for the reason you probably discussed with James about), I couldn't review this one in detail and remove the unrelated changes (go.work.sum, additional flag in Helm chart not require as there already is a generic way to add parameters, changes in the go.sum of kube-webhook-certgen and others), yet.

Gacko avatar Jan 13 '25 11:01 Gacko

@Gacko instead of changing others PRs, please point what you want to change.

Also I've been waiting for some long time now, you keep saying about the unrelated changes without pointing, so go ahead and do a review with the changes you want me to do.

Thanks

rikatz avatar Jan 13 '25 11:01 rikatz

I had to make a full squash, otherwise my life would be a hell. But I want to acknowledge @MrErlison and Gabriel Dutra for helping me on this implementation!

rikatz avatar Mar 28 '25 21:03 rikatz