Handle request_id variable correctly in auth requests

[leki75]

What this PR does / why we need it:

Ingress-nginx sets X-Request-ID HTTP header for every proxied request to support distributed tracing. Whenever we use nginx.ingress.kubernetes.io/auth-url with nginx.ingress.kubernetes.io/auth-keepalive the generated LUA-based authentication request will contain a different request-id than the one that reaches the upstream application.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• CVE Report (Scanner found CVE and adding report)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation only

How Has This Been Tested?

Deployed an ingress controller with the modified image. I used the following Kubernetes resources for the test:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    nginx.ingress.kubernetes.io/auth-keepalive: "60"
    nginx.ingress.kubernetes.io/auth-response-headers: x-lekitest
    nginx.ingress.kubernetes.io/auth-url: http://lekiauth.staging.svc.cluster.local:8080/auth
  name: lekitest
  namespace: staging
spec:
  ingressClassName: staging
  rules:
  - host: example.com
    http:
      paths:
      - backend:
          service:
            name: lekitest
            port:
              number: 8080
        path: /lekitest
        pathType: Prefix

---
apiVersion: v1
data:
  default.conf: |
    server {
        listen 8080;
        server_name localhost;
        location / {
            content_by_lua_block {
                local hdrs = ngx.req.get_headers()
                ngx.say("x-request-id ", hdrs["x-request-id"])
                ngx.say("x-lekitest ", hdrs["x-lekitest"])
            }
        }
    }
kind: ConfigMap
metadata:
  name: lekitest
  namespace: staging

---
apiVersion: v1
data:
  default.conf: |
    server {
        listen 8080;
        server_name localhost;
        location / {
            content_by_lua_block {
                local req_id = ngx.req.get_headers()["x-request-id"]
                if req_id == nil then
                    ngx.header["x-lekitest"] = "err"
                else
                    ngx.header["x-lekitest"] = req_id .. "-lekitest"
                end
                ngx.say("ok")
            }
        }
    }
kind: ConfigMap
metadata:
  name: lekiauth
  namespace: staging

---
apiVersion: v1
kind: Pod
metadata:
  creationTimestamp: null
  labels:
    run: lekitest
  name: lekitest
  namespace: staging
spec:
  containers:
  - image: openresty/openresty
    name: lekitest
    resources: {}
    volumeMounts:
    - name: lekitest
      mountPath: /etc/nginx/conf.d
  dnsPolicy: ClusterFirst
  restartPolicy: Always
  volumes:
  - name: lekitest
    configMap:
      name: lekitest
status: {}

---
apiVersion: v1
kind: Pod
metadata:
  creationTimestamp: null
  labels:
    run: lekiauth
  name: lekiauth
  namespace: staging
spec:
  containers:
  - image: openresty/openresty
    name: lekiauth
    resources: {}
    volumeMounts:
    - name: lekiauth
      mountPath: /etc/nginx/conf.d
  dnsPolicy: ClusterFirst
  restartPolicy: Always
  volumes:
  - name: lekiauth
    configMap:
      name: lekiauth
status: {}

---
apiVersion: v1
kind: Service
metadata:
  name: lekitest
  namespace: staging
spec:
  ports:
  - name: http
    port: 8080
    protocol: TCP
  selector:
    run: lekitest
  type: ClusterIP

---
apiVersion: v1
kind: Service
metadata:
  name: lekiauth
  namespace: staging
spec:
  ports:
  - name: http
    port: 8080
    protocol: TCP
  selector:
    run: lekiauth
  type: ClusterIP

Results without the patch:

❯ curl -i http://example.com/lekitest
HTTP/1.1 200 OK
Date: Wed, 26 Oct 2022 23:49:22 GMT
Content-Type: application/octet-stream
Transfer-Encoding: chunked
Connection: keep-alive
Strict-Transport-Security: max-age=15724800; includeSubDomains

x-request-id 761994d5db3c123bcd92e18ff2967f09
x-lekitest 2751bf4b6374242be1c17bba0d7716c5-lekitest

Results with the patch:

❯ curl -i http://example.com/lekitest
HTTP/1.1 200 OK
Date: Wed, 26 Oct 2022 23:51:32 GMT
Content-Type: application/octet-stream
Transfer-Encoding: chunked
Connection: keep-alive
Strict-Transport-Security: max-age=15724800; includeSubDomains

x-request-id 9bc4b90688a3fd68e9e83c856347a1d7
x-lekitest 9bc4b90688a3fd68e9e83c856347a1d7-lekitest

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I've read the CONTRIBUTION guide
• I have added unit and/or e2e tests to cover my changes.
• All new and existing tests passed.
• Added Release Notes.

Fixed a bug to use the same request-id for authentication requests and upstream requests when `nginx.ingress.kubernetes.io/auth-url` annotation is used with `nginx.ingress.kubernetes.io/auth-keepalive`.

[k8s-ci-robot]

@leki75: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

Hi @leki75. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ElvinEfendi]

the generated LUA-based authentication request will contain a different request-id than the one that reaches the upstream application

This sounds like a feature to me not a bug. The request to the external auth server is an another request, so it should have a different request id.

[leki75]

This sounds like a feature to me not a bug. The request to the external auth server is an another request, so it should have a different request id.

If you remove the nginx.ingress.kubernetes.io/auth-keepalive: "60" annotation (changing back to the auth_request module instead of the LUA version), then the 2 request IDs are the same. I think we should have the same output whether to use keepalives or not.

[leki75]

@ElvinEfendi @strongjz Any updates on this, please?

[leki75]

@ElvinEfendi @strongjz Happy new year! Any updates, please?

[raja]

@tao12345666333 @strongjz @ElvinEfendi Is it possible to get this small change reviewed?

[tao12345666333]

/ok-to-test

[ekovacs]

@tao12345666333 @strongjz @ElvinEfendi
looks like all the CI has passed. could we get this merged?

[tao12345666333]

@ekovacs Since we've had some issues recently, we'll work on higher priority things first and pick this up after a new release

[leki75]

@tao12345666333 @strongjz @ElvinEfendi Is it possible to get this change merged?

[leki75]

@tao12345666333 @strongjz @ElvinEfendi Any updates on merging this change, please?

[tao12345666333]

Sorry for the delay. Let me take a look

[tao12345666333]

The reason why you see different X-Request-ID headers in this scenario is because of how Ingress-nginx handles these requests internally:

1. When a client sends an HTTP request, Ingress-nginx generates an X-Request-ID header for that incoming request if it doesn't already have one.
2. Before forwarding the request to your upstream application, Ingress-nginx first sends an internal subrequest (Lua-based) to the specified auth-url for authentication.
3. This internal subrequest is treated as a separate and independent HTTP transaction by Nginx, so it gets assigned its own unique X-Request-ID header value.

As a result, when comparing both requests (the original client's and Lua-based authentication), they will have different X-Request-ID values since they are considered distinct transactions within Nginx.

I suggest adding a new configuration item to allow users to control whether to merge or separate request-id to avoid confusion

[leki75]

Thanks for your reply @tao12345666333. This is all true, but the X-Request-ID header will be the same whenever you change back to the original ngx_http_auth_request_module (non Lua-based) version. This is confusing that using nginx.ingress.kubernetes.io/auth-keepalive changes the behavior.

[tao12345666333]

We can list all scenarios and regulate their behavior, right? @leki75

[leki75]

We can list all scenarios and regulate their behavior, right? @leki75

Unfortunately, we cannot regulate the behavior of the non-Lua version. @tao12345666333 Do you think that we should make it configurable on Lua-only?

[tao12345666333]

Yes. I think that's enough.

[netlify]

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	9066776
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/64ca3e7ec596f1000807fb5f

[leki75]

@tao12345666333 could you take a look, please?

[tao12345666333]

👌 I will add this to my list, will finish on Friday

[tao12345666333]

/lgtm

/retest

Thanks

[tao12345666333]

I like this name

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leki75, tao12345666333

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:

• OWNERS [tao12345666333]

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