Skip to content

Conversation

dhurley
Copy link
Contributor

@dhurley dhurley commented Jul 5, 2024

Proposed changes

According to the docs here https://nginx.org/en/docs/http/ngx_http_api_module.html#def_nginx_http_upstream, the keepalive field is called keepalive not keepalives.
This PR fixes the field name in the Upstream struct.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@dhurley dhurley requested a review from a team as a code owner July 5, 2024 15:23
@dhurley dhurley changed the title fix: update keepalive field for http upstreams Update keepalive field for http upstreams Jul 8, 2024
@lucacome
Copy link
Contributor

lucacome commented Jul 8, 2024

@dhurley are you not able to see the number of keepalive without this fix? I don't see how it was able to unmarshal it with the wrong field name 🤔

@dhurley
Copy link
Contributor Author

dhurley commented Jul 9, 2024

@dhurley are you not able to see the number of keepalive without this fix? I don't see how it was able to unmarshal it with the wrong field name 🤔

From my testing, if a field defined in your struct that isn't in the json payload then it just defaults to zero. It doesn't error out.

@lucacome
Copy link
Contributor

lucacome commented Jul 9, 2024

From my testing, if a field defined in your struct that isn't in the json payload then it just defaults to zero. It doesn't error out.

Sorry I didn't phrase it the best way, I was just wondering if you confirmed that this never worked before (since it always had the extra s) 😅

@dhurley
Copy link
Contributor Author

dhurley commented Jul 10, 2024

From my testing, if a field defined in your struct that isn't in the json payload then it just defaults to zero. It doesn't error out.

Sorry I didn't phrase it the best way, I was just wondering if you confirmed that this never worked before (since it always had the extra s) 😅

Yes I can confirm it never worked. Here is an example curl request I made against the Plus API confirming that it is keepalive and not keepalives.

curl localhost:80/api/9/http/upstreams/ | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1390  100  1390    0     0  1357k      0 --:--:-- --:--:-- --:--:-- 1357k
{
  "test_backend": {
    "peers": [
      {
        "id": 0,
        "server": "[2a00:1450:4001:827::200e]:80",
        "name": "google.com:80",
        "backup": false,
        "weight": 1,
        "state": "up",
        "active": 0,
        "requests": 0,
        "responses": {
          "1xx": 0,
          "2xx": 0,
          "3xx": 0,
          "4xx": 0,
          "5xx": 0,
          "codes": {},
          "total": 0
        },
        "sent": 0,
        "received": 0,
        "fails": 0,
        "unavail": 0,
        "health_checks": {
          "checks": 0,
          "fails": 0,
          "unhealthy": 0
        },
        "downtime": 0
      },
      {
        "id": 1,
        "server": "172.217.16.206:80",
        "name": "google.com:80",
        "backup": false,
        "weight": 1,
        "state": "up",
        "active": 0,
        "requests": 0,
        "responses": {
          "1xx": 0,
          "2xx": 0,
          "3xx": 0,
          "4xx": 0,
          "5xx": 0,
          "codes": {},
          "total": 0
        },
        "sent": 0,
        "received": 0,
        "fails": 0,
        "unavail": 0,
        "health_checks": {
          "checks": 0,
          "fails": 0,
          "unhealthy": 0
        },
        "downtime": 0
      },
      {
        "id": 2,
        "server": "[2a00:1450:4001:81c::2003]:80",
        "name": "google.ie:80",
        "backup": false,
        "weight": 1,
        "state": "up",
        "active": 0,
        "requests": 0,
        "responses": {
          "1xx": 0,
          "2xx": 0,
          "3xx": 0,
          "4xx": 0,
          "5xx": 0,
          "codes": {},
          "total": 0
        },
        "sent": 0,
        "received": 0,
        "fails": 0,
        "unavail": 0,
        "health_checks": {
          "checks": 0,
          "fails": 0,
          "unhealthy": 0
        },
        "downtime": 0
      },
      {
        "id": 3,
        "server": "142.250.185.67:80",
        "name": "google.ie:80",
        "backup": false,
        "weight": 1,
        "state": "up",
        "active": 0,
        "requests": 0,
        "responses": {
          "1xx": 0,
          "2xx": 0,
          "3xx": 0,
          "4xx": 0,
          "5xx": 0,
          "codes": {},
          "total": 0
        },
        "sent": 0,
        "received": 0,
        "fails": 0,
        "unavail": 0,
        "health_checks": {
          "checks": 0,
          "fails": 0,
          "unhealthy": 0
        },
        "downtime": 0
      }
    ],
    "keepalive": 0,
    "queue": {
      "size": 0,
      "max_size": 10,
      "overflows": 0
    },
    "zombies": 0,
    "zone": "my_stream_backend3"
  }
}

@lucacome lucacome added the bug An issue reporting a potential bug label Jul 10, 2024
@lucacome lucacome added this to the v1.2.2 milestone Jul 10, 2024
@lucacome lucacome mentioned this pull request Jul 10, 2024
@github-actions github-actions bot removed the bug An issue reporting a potential bug label Jul 11, 2024
@oliveromahony
Copy link
Contributor

@lucacome is this ok to be merged?

@lucacome lucacome added the bug An issue reporting a potential bug label Jul 11, 2024
@lucacome
Copy link
Contributor

@oliveromahony it's ok for me

@oliveromahony oliveromahony merged commit 56570a6 into nginx:main Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue reporting a potential bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants