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

Root context factory indirection #79

Open
casimiro opened this issue Oct 13, 2023 · 0 comments
Open

Root context factory indirection #79

casimiro opened this issue Oct 13, 2023 · 0 comments

Comments

@casimiro
Copy link

Hello,

I noticed the SDK expects registerRootContext to receive a name parameter that's used as a key to store context_factory in the global map root_context_factory_map:

let root_context_factory_map = new Map<string, (context_id: u32) => RootContext>();
// ...
export function registerRootContext(
  root_context_factory: (context_id: u32) => RootContext,
  name: string): void {
  root_context_factory_map.set(name, root_context_factory);
}

When the host application creates the root context for the filter it calls proxy_on_context_create, which invokes ensureRootContext:

export function ensureRootContext(root_context_id: u32): RootContext {
  log(LogLevelValues.debug, "ensureRootContext(root_context_id: " + root_context_id.toString() + ")");
  log(LogLevelValues.debug, "Current context_map: " + context_map.keys().join(", "));
  if (context_map.has(root_context_id)) {
    log(LogLevelValues.debug, "Returning root context for id: " + root_context_id.toString());
    return getRootContext(root_context_id);
  }
  const root_id = get_plugin_root_id();
  log(LogLevelValues.debug, "Registering new root context for " + root_id + " with id: " + root_context_id.toString());
  if (!root_context_factory_map.has(root_id)) {
    throw new Error("Missing root context factory for root id: " + root_id);
  }
  const root_context_func = root_context_factory_map.get(root_id);
  const root_context = root_context_func(root_context_id);
  root_context.context_id = root_context_id;
  context_map.set(root_context_id, root_context);
  return root_context;
}

ensureRootContext calls get_plugin_root_id() (i.e., get_property("plugin_root_id") expecting it to return the name of the filter as it's set in envoy configuration file. It then uses this value to retrieve the context factory from root_context_factory_map which is then used to build the root context that's later returned.

It's not clear to me, however, the necessity of storing the root context factory in a map. Why can't it be stored directly in a variable, e.g. let root_context_factory: (context_id: u32) => RootContext;?

Does the SDK expect filters to implement more than one root context factory? Is there a use case requiring multiple root contexts?

I stumble upon this behaviour while trying to run the addHeader example filter on ngx_wasm_module which returns a numeric value for the property plugin_root_id -- instead of the expected addHeader value.

If the authors agree this indirection can be safely removed, I'm happy to open a PR with a patch that removes it while keeping registerRootContext compatible with existing filters.

Thank you for working on this proxy-wasm SDK.

Kind regards,
Caio.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant