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(wasm): do not call attach() on re-entrancy #12402

Merged
merged 1 commit into from
Jan 29, 2024
Merged

Conversation

flrgh
Copy link
Contributor

@flrgh flrgh commented Jan 23, 2024

Summary

kong.runloop.wasm.attach() may be called more than once when filters utilize the dispatch_http_call proxy-wasm SDK function. Previously this was assumed to be false, which can yield errors like this one:

2024-01-22 16:39:16 2024/01/22 22:39:16 [crit] 2464#0: *340 [lua] wasm.lua:790: attach(): failed attaching service(6bc44ccb-30c9-5f3d-acfa-017fa1b2ef82) filter chain to request: previous plan already attached, client: 172.17.0.1, server: kong, request: "GET /anything HTTP/1.1", host: "localhost:8000", request_id: "112ec236dcf487391a579a6f6f63fc0b"

This adds a ngx.ctx check and avoids calling proxy_wasm.attach() in this case.

Unfortunately it is incredibly non-trivial to add a test case for this at the moment, but the behavior change/correctness has been validated by hand.

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR

Issue reference

KAG-3603

@flrgh flrgh added this to the 3.6.0 milestone Jan 23, 2024
@flrgh flrgh requested review from hishamhm and locao January 23, 2024 19:34
@github-actions github-actions bot added core/proxy core/wasm Everything relevant to [proxy-]wasm labels Jan 23, 2024
@flrgh flrgh added cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee backport release/3.5.x labels Jan 23, 2024
kong/runloop/wasm.lua Outdated Show resolved Hide resolved
@flrgh flrgh force-pushed the fix/wasm-re-entry branch from e4e9770 to 379e9ba Compare January 23, 2024 19:53
@flrgh flrgh marked this pull request as draft January 23, 2024 22:00
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jan 24, 2024
@flrgh flrgh force-pushed the fix/wasm-re-entry branch 8 times, most recently from fcfd6be to aa8b8a1 Compare January 25, 2024 01:02
@flrgh flrgh force-pushed the fix/wasm-re-entry branch from aa8b8a1 to ae76e6c Compare January 29, 2024 18:19
@flrgh flrgh marked this pull request as ready for review January 29, 2024 18:20
Copy link
Contributor

@hishamhm hishamhm left a comment

Choose a reason for hiding this comment

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

I'm still a bit surprised that calling proxy_wasm.start a second time works (i.e. that the last bit doesn't need to be inside the if block as well), but the changes done here definitely don't hurt, so if you managed to test this manually and get an improvement in behavior with this branch as-is, it's an LGTM from me!

@locao locao merged commit 4d03ca4 into master Jan 29, 2024
25 checks passed
@locao locao deleted the fix/wasm-re-entry branch January 29, 2024 21:53
@team-gateway-bot
Copy link
Collaborator

Backport failed for release/3.4.x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release/3.4.x
git worktree add -d .worktree/backport-12402-to-release/3.4.x origin/release/3.4.x
cd .worktree/backport-12402-to-release/3.4.x
git switch --create backport-12402-to-release/3.4.x
git cherry-pick -x 4d03ca45edfa54c76e5d2d4271892f37b7524ddd

@team-gateway-bot
Copy link
Collaborator

Backport failed for release/3.5.x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release/3.5.x
git worktree add -d .worktree/backport-12402-to-release/3.5.x origin/release/3.5.x
cd .worktree/backport-12402-to-release/3.5.x
git switch --create backport-12402-to-release/3.5.x
git cherry-pick -x 4d03ca45edfa54c76e5d2d4271892f37b7524ddd

@team-gateway-bot
Copy link
Collaborator

Successfully created cherry-pick PR for master:

@team-gateway-bot
Copy link
Collaborator

Successfully created backport PR for release/3.6.x:

flrgh added a commit that referenced this pull request Jan 29, 2024
flrgh added a commit that referenced this pull request Jan 29, 2024
flrgh added a commit that referenced this pull request Jan 29, 2024
(cherry picked from commit 4d03ca4)

Co-authored-by: Michael Martin <[email protected]>
@thibaultcha
Copy link
Member

thibaultcha commented Jan 30, 2024

Calling proxy_wasm.start() several times probably work since the underlying implementation uses ngx_wasm_ops and the filter chain is phase-aware; when invoked a second time, the filter chain loop will be hit but not entered, so ngx_http_wasm_ffi_start sort of becomes a NOP. This is the expected behavior since ngx_wasm_ops_resume is re-entrant. At least this would be my guess as to why it works.

This was referenced Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/proxy core/wasm Everything relevant to [proxy-]wasm size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants