lua: add requestInfo():dynamicMetadata() API

[dio]

This patch allows to get and set the dynamic metadata of a request info object via Lua.

Setting metadata,

request_handle:requestInfo():dynamicMetadata():set("envoy.lb", "foo", "bar")

Getting metadata,

-- returns a table
request_handle:requestInfo():dynamicMetadata():get("envoy.lb")
request_handle:requestInfo():dynamicMetadata():get("envoy.lb")["foo"]

Risk Level: Low

Testing: Unit and integration tests

Docs Changes:

• Added dynamic metadata object API

Release Notes:

• Added request info dynamic metadata Lua API

Fixes #3618

Signed-off-by: Dhi Aurrahman dio@rockybars.com

[mattklein123]

Generally looks great. 1 question. Thank you!

[mattklein123]

Is this safe during iteration? Do you need to block a set operation while iterating via the stored iterator which might be invalidated?

[dio]

To be honest, I'm not sure. Will have a test regarding that.

[dio]

Sorry, messed up with DCO. Rebased, but I think the review comment is still there.

[mattklein123]

@dio sorry do you mean you still need to work on the comment? Or I should review again and you fixed it?

[dio]

@mattklein123 apology for not being clear, I meant to say: the comment was not removed (de-referenced) by the forced pushing.

I have added a test regarding that case to address the comment. Please help to review it. Thank you.

[mattklein123]

I'm still not convinced this is safe. Looking quickly here: https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.map it's not clear from these docs when the iterator is invalidated, but if it's anything like unordered_map I think that iterators can get invalidated sometimes: https://stackoverflow.com/questions/16781886/can-we-store-unordered-maptiterator. Can you maybe poke around the proto map implementation to understand whether this safe in all cases and guaranteed to be safe in the future? I'm mainly wondering if it would be safer/easier to block modification during iteration like we do for the header map currently?

[dio]

I'm mainly wondering if it would be safer/easier to block modification during iteration like we do for the header map currently?

One question/clarification, do you mean by throwing an error like the following?

https://github.com/dio/envoy/blob/36f01b8c35c9b3decff7c43ef2ba8e5e2ee0e2ed/source/extensions/filters/http/lua/wrappers.cc#L153-L155

[mattklein123]

Yeah that's what I was referring to. If it's safe in all cases is fine, I'm just not sure it is. Up to you whether you want to block or investigate more.

[dio]

Got it, I will explore more then.

[dio]

@mattklein123 based on this: https://github.com/google/protobuf/blob/88795f64e4c5dc870c63b3d4d39924cc91590b2f/src/google/protobuf/map.h#L326-L327

// 8. Mutations to a map do not invalidate the map's iterators, pointers to
//    elements, or references to elements.

It seems the iterator is not invalidated during mutation/rehash. Does it mean we can allow mutation during iteration?

[dio]

I'm not sure I'm following. Do you mean deletion from Lua?

Since we don't have that API yet (only set and get for now, I think I will try to set current_ to point to something else, i.e. a struct with the same key but a different value will give the same effect?).

[dio]

FWIW, I'm thinking to just block any mutation in mutation API like set as your suggestion just to be safe.

[mattklein123]

What I meant is right now when you iterate in the __pairs function, you set current_ to the next set member before you return the value to Lua. What if Lua then deletes that next member? What happens to the iterator? I have to think that it's no longer valid and bad things will happen?

Yeah, TBH, I think for now we should just do the block so we don't have to keep thinking about all of these edge cases. Is that already implemented or do you want to change it?

[dio]

What I meant is right now when you iterate ...

Thank you for this. This really makes me think about this all. Should be careful next time.

I think for now we should just do the block

If I understand you correctly,

I have the following before the setting a metadata entry. https://github.com/envoyproxy/envoy/pull/3800/files#diff-cb2388822be823ac2236f97bc5cc776dR153

if (iterator_.get() != nullptr) {
  luaL_error(state, "dynamic metadata map cannot be modified while iterating");
}

Do I need to check somewhere else? Thanks.

[mattklein123]

No I think that's fine. Will review again.

[mattklein123]

very nice