if request id was not supplied in header, generate uuid

[nirrozenbaum]

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR fixes a bug of EPP crashing in high scale. the bug happens due to requestID being empty and then prefix plugin maps different requests to the same PluginState entry, which results in different go routines trying to write to the same map concurrently. more details in issue #1489.
This PR guarantees requestID is always set. if it's not supplied as a header, we generate it while handling the request headers, before starting to handle the request body.

Which issue(s) this PR fixes:

Fixes #1489

Does this PR introduce a user-facing change?:

NONE

[netlify]

✅ Deploy Preview for gateway-api-inference-extension ready!

Name	Link
🔨 Latest commit	b100b9f
🔍 Latest deploy log	https://app.netlify.com/projects/gateway-api-inference-extension/deploys/68b1366f4bc5a90008e04154
😎 Deploy Preview	https://deploy-preview-1490--gateway-api-inference-extension.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nirrozenbaum

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 [nirrozenbaum]

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

[nirrozenbaum]

/cc @kfswain

[kfswain]

I notice this deals with the requestID problem, but leave the pluginState concurrent unsafe still.

I don't think we have any guarantees that a plugin will provide the appropriate keys when calling Write, leading to a similar issue

[nirrozenbaum]

I notice this deals with the requestID problem, but leave the pluginState concurrent unsafe still.

I don't think we have any guarantees that a plugin will provide the appropriate keys when calling Write, leading to a similar issue

I'm not sure I followed the problem you're describing.
I'll state here the assumptions of PluginState code:

• a state is maintained per request. as long as a plugin uses PluginState of its own (it's not shared with another plugin), which is the current state, and as long as different requests get different requestID, that means that in different requests we fetch different values from the sync.map.
• a plugin never runs two extension points (or more) in parallel for the same request.

the sync map is used to map between requestID -> map[string]any (just a [key, value] pairs for that request).
each extension point of the prefix plugin can do with this map whatever we want, without the need to lock it, because the code is contention-free (based on the above assumptions).
the map is relevant only for current plugin and only for current request.

a plugin can reuse the same StateKey as many times as we want for different requests. this is not an issue.
in fact this is even verified in the unit tests.
a plugin can also use the same StateKey in the same request. but that would be serial and not in parallel, so again no issue.

[nirrozenbaum]

I notice this deals with the requestID problem, but leave the pluginState concurrent unsafe still.
I don't think we have any guarantees that a plugin will provide the appropriate keys when calling Write, leading to a similar issue

I'm not sure I followed the problem you're describing. I'll state here the assumptions of PluginState code:

• a state is maintained per request. as long as a plugin uses PluginState of its own (it's not shared with another plugin), which is the current state, and as long as different requests get different requestID, that means that in different requests we fetch different values from the sync.map.
• a plugin never runs two extension points (or more) in parallel for the same request.

the sync map is used to map between requestID -> map[string]any (just a [key, value] pairs for that request). each extension point of the prefix plugin can do with this map whatever we want, without the need to lock it, because the code is contention-free (based on the above assumptions). the map is relevant only for current plugin and only for current request.

a plugin can reuse the same StateKey as many times as we want for different requests. this is not an issue. in fact this is even verified in the unit tests. a plugin can also use the same StateKey in the same request. but that would be serial and not in parallel, so again no issue.

was your intention that the internal map should also be using sync map because we don't trust the caller?

[kfswain]

was your intention that the internal map should also be using sync map because we don't trust the caller?

Yes, if a plugin implemented absentmindedly passes in an empty string for the requestID, that shouldn't crash the entire EPP, and thats an easy one to miss in testing

[nirrozenbaum]

Yes, if a plugin implemented absentmindedly passes in an empty string for the requestID, that shouldn't crash the entire EPP, and thats an easy one to miss in testing

yeah, sure. I can change that.
do you prefer in this PR or a separate one?

[kfswain]

This PR works for me

[nirrozenbaum]

This PR works for me

Done 💪🏻

[kfswain]

/lgtm

Thanks!