Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gRPC health checks creation #4996

Closed
jasonwilliams14 opened this issue Jan 27, 2024 · 3 comments
Closed

gRPC health checks creation #4996

jasonwilliams14 opened this issue Jan 27, 2024 · 3 comments
Assignees
Labels
backlog Pull requests/issues that are backlog items bug An issue reporting a potential bug
Milestone

Comments

@jasonwilliams14
Copy link
Contributor

When creating a gRPC health check for NGINX Ingress controller, the NGINX .conf being generated is incorrect.
Snippet from the error log:

[emerg] 320#320: invalid grpc parameter \"keepalive_time=60s\" in /etc/nginx/conf.d/vs_default_virtual-server.conf:132\n"

This is occuring when you specify the type: grpc in active health check section, or use grpcStatus, grpcService: ( I tested all of them in different variations, but behavior is the same)

  upstreams:
  - name: grpc1
    service: grpc-svc
    port: 50051
    type: grpc
    healthCheck:
      enable: true
      interval: 20s
      port: 50051

The generated .conf looks like the following:

        health_check port=50051 interval=1s jitter=2s
            fails=1 passes=1

             type=grpc grpc_status=12
             grpc_service=helloworld.Greeter keepalive_time=60s;

The last bit, keepalive_time=60s is a HTTP check and not allowed in gRPC.

https://docs.nginx.com/nginx/admin-guide/load-balancer/grpc-health-check/#grpc-servers-that-accept-health-checking-protocol

Expected behavior
Defining a gRPC health check should correctly generate a NGINX .conf with the gRPC specific checks.
When using any of the gRPC specific checks, we should validate and ensure we are not adding HTTP checks.

Your environment
Tested in k3d and kind:
Tested NIC 3.3.0 to 3.4.2.

@jasonwilliams14 jasonwilliams14 added the bug An issue reporting a potential bug label Jan 27, 2024
Copy link

Hi @jasonwilliams14 thanks for reporting!

Be sure to check out the docs and the Contributing Guidelines while you wait for a human to take a look at this 🙂

Cheers!

@danielnginx
Copy link
Collaborator

We have tests that currently pass with this scenario, we need to investigate them.

@danielnginx danielnginx added the backlog Pull requests/issues that are backlog items label Jan 29, 2024
@danielnginx danielnginx moved this from Todo ☑ to Prioritized Backlog in NGINX Ingress Controller Jan 29, 2024
@brianehlert brianehlert added this to the v3.6.0 milestone Apr 8, 2024
@brianehlert brianehlert added the ready for refinement An issue that was triaged and it is ready to be refined label May 8, 2024
@jasonwilliams14
Copy link
Contributor Author

To provide some more context, we need to remove keepalive_time from the configured output in the above thread . keepalive_time is not a valid setting when using gRPC health checks with NGINX.

https://docs.nginx.com/nginx/admin-guide/load-balancer/grpc-health-check/#grpc-servers-that-accept-health-checking-protocol

@shaun-nx shaun-nx removed the ready for refinement An issue that was triaged and it is ready to be refined label Jun 26, 2024
@shaun-nx shaun-nx moved this from Prioritized Backlog to Todo ☑ in NGINX Ingress Controller Jun 26, 2024
@shaun-nx shaun-nx modified the milestones: v3.6.0, v3.7.0 Jun 26, 2024
@j1m-ryan j1m-ryan moved this from Todo ☑ to In Progress 🛠 in NGINX Ingress Controller Jun 26, 2024
@j1m-ryan j1m-ryan modified the milestones: v3.7.0, v3.6.1 Jul 1, 2024
@j1m-ryan j1m-ryan moved this from In Progress 🛠 to In Review 👀 in NGINX Ingress Controller Jul 1, 2024
@j1m-ryan j1m-ryan moved this from In Review 👀 to Done 🚀 in NGINX Ingress Controller Jul 2, 2024
@j1m-ryan j1m-ryan closed this as completed by moving to Done 🚀 in NGINX Ingress Controller Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Pull requests/issues that are backlog items bug An issue reporting a potential bug
Projects
Archived in project
Development

No branches or pull requests

5 participants