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

fix: fix a panic when receiving a broken config from Gateway #5003

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

pmalek
Copy link
Member

@pmalek pmalek commented Oct 27, 2023

What this PR does / why we need it:

When KIC receives a broken config from Gateway (e.g. like the one described in #4990) with route's priority in scientific notation:

routes:
  - request_buffering: true
    expression: ((http.path == "/echo") || (http.path ^= "/echo/")) && (http.method == "GET")
    preserve_host: true
    name: default.echo.echo..1027
    response_buffering: true
    updated_at: 1698340425
    https_redirect_status_code: 426
    ws_id: 0dc6f45b-8f8d-40d2-a504-473544ee190b
    id: 6ece5df4-2c97-5372-a840-47e222d4ea9d
    created_at: 1698340425
    tags:
      - k8s-name:echo
      - k8s-namespace:default
      - k8s-kind:Ingress
      - k8s-uid:6eb54997-0c70-4a68-918b-935a356f14cd
      - k8s-group:networking.k8s.io
      - k8s-version:v1
    priority: 3.3820977671045e+15 # <- this
    service: 4a3125fc-7d51-5575-b0f3-978d9a44a49b
    protocols:
      - http
      - https
    strip_path: true

then it cannot handle an unparsed configuration and it panics:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x18 pc=0x1037d8a34]

goroutine 138 [running]:
github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/configfetcher.KongRawStateToKongState(0x0)
        kubernetes-ingress-controller/internal/dataplane/configfetcher/kongrawstate.go:19 +0x84
github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/configfetcher.(*DefaultKongLastGoodConfigFetcher).TryFetchingValidConfigFromGateways(0x14000aad1d0, {0x1040464d0, 0x14000aacff0}, {{0x104049b88?, 0x140000db230?}, 0x0?}, {0x14000bd3100, 0x2, 0x0?})
        kubernetes-ingress-controller/internal/dataplane/configfetcher/config_fetcher.go:85 +0x438
github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane.(*KongClient).Update(0x140009ec1e0, {0x1040464d0, 0x14000aacff0})
        kubernetes-ingress-controller/internal/dataplane/kong_client.go:397 +0x124
github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane.(*Synchronizer).startUpdateServer(0x14000579340, {0x1040464d0, 0x14000aacff0})
        kubernetes-ingress-controller/internal/dataplane/synchronizer.go:183 +0xa0
created by github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane.(*Synchronizer).Start in goroutine 136
        kubernetes-ingress-controller/internal/dataplane/synchronizer.go:120 +0x1c8

This PR allows KIC to handle this error and print a log about it:

2023-10-27T19:52:11+02:00     error   failed to fetch last good configuration from gateways   {"error": "routes: json: cannot unmarshal number 3.379898743849e+15 into Go struct field Route.priority of type int\nfailed to fetch configuration from \"https://10.244.0.241:8444\", got nil kong raw state\nroutes: json: cannot unmarshal number 3.379898743849e+15 into Go struct field Route.priority of type int\nfailed to fetch configuration from \"https://10.244.0.242:8444\", got nil kong raw state"}

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@pmalek pmalek added the bug Something isn't working label Oct 27, 2023
@pmalek pmalek self-assigned this Oct 27, 2023
@pmalek pmalek force-pushed the config-fetcher-panic branch from a1028a4 to e0c4a2e Compare October 27, 2023 17:57
@pull-request-size pull-request-size bot added size/M and removed size/S labels Oct 27, 2023
@pmalek pmalek marked this pull request as ready for review October 27, 2023 17:58
@pmalek pmalek requested a review from a team as a code owner October 27, 2023 17:58
Copy link
Member

@programmer04 programmer04 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎖️

@pmalek pmalek added this to the KIC v3.0.0 milestone Oct 27, 2023
Copy link
Contributor

@czeslavo czeslavo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@pmalek pmalek enabled auto-merge (squash) October 27, 2023 18:18
@pmalek pmalek merged commit 477e932 into main Oct 27, 2023
@pmalek pmalek deleted the config-fetcher-panic branch October 27, 2023 18:24
@team-k8s-bot
Copy link
Collaborator

The backport to release/2.12.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release/2.12.x release/2.12.x
# Navigate to the new working tree
cd .worktrees/backport-release/2.12.x
# Create a new branch
git switch --create backport-5003-to-release/2.12.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 477e93229d3d29ac0ef918333468b3e1ed0b5b81
# Push it to GitHub
git push --set-upstream origin backport-5003-to-release/2.12.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release/2.12.x

Then, create a pull request where the base branch is release/2.12.x and the compare/head branch is backport-5003-to-release/2.12.x.

czeslavo pushed a commit that referenced this pull request Nov 8, 2023
czeslavo added a commit that referenced this pull request Nov 8, 2023
…5120)

(cherry picked from commit 477e932)

Co-authored-by: Patryk Małek <patryk.malek@konghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants