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

Add proxy_log_destination ABI #38

Conversation

vikaschoudhary16
Copy link

This new ABI will help in usecase where it is intended to separate logs from the plugins based on the domain for example audit logs.
xref: envoyproxy/envoy#22669

Signed-off-by: Vikas Choudhary <choudharyvikas16@gmail.com>
@kyessenov
Copy link

Doesn't this need a new ABI version?
@mpwarres

@mathetake
Copy link
Contributor

mathetake commented May 4, 2023

We have also a lot of issues in the current ABI (e.g. #32) which I definitely hope we could solve.

So if you are up for it, @mpwarres, let's just do the ABI version bump and let's resume the iterative improvement of Proxy-Wasm overall. (I could spare a portion of my time into Proxy-Wasm project again).

@mathetake
Copy link
Contributor

At least the current project status is not healthy at all, and we definitely should communicate that we will resume the dev or the project is in maintenance mode.

@kyessenov
Copy link

kyessenov commented May 4, 2023

I think ABI stability is actually one of the most important things for Wasm, so doing incremental ABI changes is not ideal. There's a backdoor foreign function call to unblock changes without ABI breakage.

@mathetake
Copy link
Contributor

yeah I understand that, sorry I just meant the incremental dev with the stability in mind.

@PiotrSikora
Copy link
Contributor

Yes, this would need a new ABI version.

(FWIW, I'm working on the existing & "the minimal bugfix" ABI specs right now. I'll have them out later this month.)

@mathetake
Copy link
Contributor

@PiotrSikora sounds good, thank you so much.

@vikaschoudhary16 so let's wait for @PiotrSikora to finish the existing and minimal bugfix spec, and then let's get back to the discussion of the ABI proposed here. sg?

@vikaschoudhary16
Copy link
Author

sounds good. Thanks a lot @kyessenov @mathetake and @PiotrSikora for bringing clarity on path forward.

While waiting for new spec in the meantime I would update implementation PR to address Piotr's comments and get more early feedback.

@mathetake
Copy link
Contributor

@PiotrSikora could you share the status of the spec doc work?

@mathetake
Copy link
Contributor

ping @PiotrSikora

@vikaschoudhary16
Copy link
Author

Hi @PiotrSikora, addressed your feedback on implementation PRs at proxy_wasm_cpp_host and at envoy. Though tested manually, waiting for new spec and your go-ahead to start working on ci and tests.

@mathetake
Copy link
Contributor

mathetake commented Jun 4, 2023

@mpwarres I thought you are taking over the responsibility on the Google side from @PiotrSikora on Proxy-Wasm. What do you think is the best course of action we could take here? The current state is undeniably unhealthy as a project especially because the specification hasn't been appropriately documented (IIRC, more than approx. 2yrs have passed since @PiotrSikora said they would update the doc for the first time), and that's the only reason why we cannot resolve the issues which have existed in the first place.

@mpwarres
Copy link

mpwarres commented Jun 6, 2023

From offline discussion with @PiotrSikora and @martijneken , the plan WRT ABI is to:

  1. Remove the vNEXT ABI documentation, which was never implemented.
  2. Properly document the current 0.2.1 ABI. I believe Piotr was planning to do this, but if other work has gotten in the way, someone on my team can create a first draft for review.
  3. Create a new minor version of the ABI with incremental improvements / bugfixes (including this PR).

My team will aim to step up support for proxy-wasm-cpp-host, C++ SDK, and Envoy Wasm filter, though the ABI is shared surface that it would be good to get Piotr's input on.

WRT project health, we'd also like to start having a periodic meeting where contributors can discuss project direction and technical issues.

@vikaschoudhary16
Copy link
Author

thanks a lot @mpwarres and @mathetake !!!!

@mathetake
Copy link
Contributor

From offline discussion with @PiotrSikora and @martijneken , the plan WRT ABI is to:

  1. Remove the vNEXT ABI documentation, which was never implemented.
  2. Properly document the current 0.2.1 ABI. I believe Piotr was planning to do this, but if other work has gotten in the way, someone on my team can create a first draft for review.
  3. Create a new minor version of the ABI with incremental improvements / bugfixes (including this PR).

sounds awesome and agreed. I will take care of 1. shortly.

As for 2., how long do you think your team would take to make it up? let us know if you think it could take more than a month or so since in that case, my team (me or whoever it is) might be able to help.

WRT project health, we'd also like to start having a periodic meeting where contributors can discuss project direction and technical issues.

I would prefer having an async rather than a sync meeting for various reasons especially because me and my team members are all in different time zones. But anyway it's good to have a general sync (regardless meeting is sync or not) among contributors on the project direction, which I believe will be good for anyone involved vs before. Can we discuss this topic on a separate issue in this repository so that we could hear opinions from other people since this issue is off-topic? If you are ok with it, I will create the issue in this repo and ask for comments in various channels.

@mathetake
Copy link
Contributor

#40

@vikaschoudhary16
Copy link
Author

closing it since vNext is being deleted.

@jcchavezs
Copy link

I hope we can bring in other implementors besides envoy. I know Kong is working on support this and I believe mosn already supports proxy wasm. From my experience, there are a couple of things in the flow that could be improved to reflect more the http lifecycle, hence it is also important to bring those writing filters to give input. Personally I am in charge of coraza-proxy-wasm, a Web Application Firewall which uses many of the functions of the spec.

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

Successfully merging this pull request may close these issues.

None yet

6 participants