Skip to content

golang: add callback method for http plugin config destruction (fixes #38557) #38597

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Mar 25, 2025
Merged

golang: add callback method for http plugin config destruction (fixes #38557) #38597

merged 13 commits into from
Mar 25, 2025

Conversation

hypnoce
Copy link
Contributor

@hypnoce hypnoce commented Feb 27, 2025

Commit Message: [#38557] golang: add callback method for http plugin config destruction

Additional Description:

This enables fine grained control over the lifecycle of golang filter config in sync with C++.
Some use cases store states and resources in the config object that needs to be cleaned when config is deleted or renewed.
The current design uses a Config interface, to minimise changes and avoid breaking existing code.
I have an alternative design that adds a Destroy function in the StreamFilterConfigParser interface instead of introducing an interface. Let me know what you think, given the current go api is not considered stable and breaking change should be acceptable.

Risk Level: Low

Testing: A test in config_test.cc was added to ensure Destroy function is being called on the config instance.

Docs Changes: none

Release Notes:

added http golang filter config destroy callback. When a config gets deleted from envoy, the go plugin calls the Destroy function on the config instance.
config should implement the new github.com/envoyproxy/envoy/contrib/golang/common/go/api.Config interface, implementing the Destroy function.

Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@hypnoce
Copy link
Contributor Author

hypnoce commented Feb 27, 2025

/retest

@hypnoce
Simplify test
0d023aa 
Signed-off-by: Francois JACQUES <fjacques@murex.com>
@hypnoce
cleanup BUILD file
8026b81 
Signed-off-by: Francois JACQUES <fjacques@murex.com>
@hypnoce
fix memory leak
4aca781 
Signed-off-by: Francois JACQUES <fjacques@murex.com>
@hypnoce
Fix include
a007382 
Signed-off-by: Francois JACQUES <fjacques@murex.com>
@hypnoce
Add symlink to api.h
d15daf9 
Signed-off-by: Francois JACQUES <fjacques@murex.com>
@hypnoce
fix formating
9111aa6 
Signed-off-by: Francois JACQUES <fjacques@murex.com>
@hypnoce
Fix formating
19c0383 
Signed-off-by: Francois JACQUES <fjacques@murex.com>
@hypnoce
fix changelog formating
5114d5c 
Signed-off-by: Francois JACQUES <fjacques@murex.com>
@hypnoce
Fix formating
b8bbcd0 
Signed-off-by: Francois JACQUES <fjacques@murex.com>
doujiang24
doujiang24 reviewed Mar 2, 2025
View reviewed changes
doujiang24
doujiang24 reviewed Mar 2, 2025
View reviewed changes
Copy link
Member

@doujiang24 doujiang24 left a comment

Choose a reason for hiding this comment

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

lgtm, no backward compatibility issues.

@hypnoce
Add documentation
ed82383 
Signed-off-by: Francois JACQUES <fjacques@murex.com>
doujiang24
doujiang24 approved these changes Mar 3, 2025
View reviewed changes
Copy link
Member

@doujiang24 doujiang24 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @hypnoce

@hypnoce
Copy link
Contributor Author

hypnoce commented Mar 4, 2025 •
edited
Loading

@doujiang24 I have this alternative design : https://github.com/hypnoce/envoy/commits/go_destroy_config_2/
It's more consistent with existing functions for config lifecycle but introduces breaking changes. Since golang plugin api is still in alpha, breaking changes should be acceptable at this stage.
WDYT ?

@hypnoce
Copy link
Contributor Author

hypnoce commented Mar 10, 2025

@doujiang24 any through on this PR ?

@doujiang24
Copy link
Member

Thanks @hypnoce I'll take a look at the alternative design this weekend.

@doujiang24
Copy link
Member

@doujiang24 I have this alternative design : https://github.com/hypnoce/envoy/commits/go_destroy_config_2/ It's more consistent with existing functions for config lifecycle but introduces breaking changes. Since golang plugin api is still in alpha, breaking changes should be acceptable at this stage. WDYT ?

Since the Destroy is not a widely used API, I think the current PR is good enough, thoughts?

@hypnoce
Copy link
Contributor Author

hypnoce commented Mar 16, 2025

From an API point of view, I think consistency is a good thing, so design #2 seems to me a better one. Technically both solutions are working. I'd rather go with the second design and do a PR for it unless you prefer the first one. You are the maintainer :)

@hypnoce
Copy link
Contributor Author

hypnoce commented Mar 16, 2025

@doujiang24 here the draft PR for alt design: #38767

@RyanTheOptimist
Copy link
Contributor

Looks like this needs a main merge

@doujiang24
Copy link
Member

@hypnoce Okay, I prefer to merge the current PR, since the Destroy is not a widely used API.

It's good enough to merge after a main merge cc @RyanTheOptimist

@hypnoce
Merge branch 'main' into go_destroy_config
12276d9 
Signed-off-by: François JACQUES <fjacques@murex.com>
@hypnoce
Copy link
Contributor Author

hypnoce commented Mar 20, 2025

/retest

@doujiang24
Copy link
Member

It's ready to merge, cc @phlax @wbpcode Thanks.

@hypnoce
Merge branch 'main' into go_destroy_config
4f836a5 
Signed-off-by: François JACQUES <fjacques@murex.com>
@ggreenway ggreenway self-assigned this Mar 25, 2025
ggreenway
ggreenway approved these changes Mar 25, 2025
View reviewed changes
Copy link
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

Approved by senior extension maintainer. I am approving without review.

@ggreenway ggreenway merged commit 11ab50a into envoyproxy:main Mar 25, 2025
24 checks passed
johnlanni pushed a commit to johnlanni/envoy that referenced this pull request Mar 27, 2025
@hypnoce @johnlanni
golang: add callback method for http plugin config destruction (envoy…
ed671c8 
…proxy#38597)

This enables fine grained control over the lifecycle of golang filter
config in sync with C++.
Some use cases store states and resources in the config object that
needs to be cleaned when config is deleted or renewed.
The current design uses a Config interface, to minimise changes and
avoid breaking existing code.
I have an alternative design that adds a Destroy function in the
StreamFilterConfigParser interface instead of introducing an interface.
Let me know what you think, given the current go api is not considered
stable and breaking change should be acceptable.

Fixes envoyproxy#38557

Signed-off-by: François JACQUES <fjacques@murex.com>
johnlanni added a commit to higress-group/envoy that referenced this pull request Mar 27, 2025
@johnlanni @hypnoce
golang: add callback method for http plugin config destruction (envoy…
3d93dd6 
…proxy#38597) (#7)

Signed-off-by: François JACQUES <fjacques@murex.com>
Co-authored-by: François JACQUES <fjacques@murex.com>
jewertow pushed a commit to jewertow/envoy that referenced this pull request Apr 2, 2025
@hypnoce @jewertow
golang: add callback method for http plugin config destruction (envoy…
8b74547 
…proxy#38597)

This enables fine grained control over the lifecycle of golang filter
config in sync with C++.
Some use cases store states and resources in the config object that
needs to be cleaned when config is deleted or renewed.
The current design uses a Config interface, to minimise changes and
avoid breaking existing code.
I have an alternative design that adds a Destroy function in the
StreamFilterConfigParser interface instead of introducing an interface.
Let me know what you think, given the current go api is not considered
stable and breaking change should be acceptable.

Fixes envoyproxy#38557

Signed-off-by: François JACQUES <fjacques@murex.com>
agrawroh pushed a commit to agrawroh/envoy that referenced this pull request Apr 9, 2025
@hypnoce @agrawroh
golang: add callback method for http plugin config destruction (envoy…
d523838 
…proxy#38597)

This enables fine grained control over the lifecycle of golang filter
config in sync with C++.
Some use cases store states and resources in the config object that
needs to be cleaned when config is deleted or renewed.
The current design uses a Config interface, to minimise changes and
avoid breaking existing code.
I have an alternative design that adds a Destroy function in the
StreamFilterConfigParser interface instead of introducing an interface.
Let me know what you think, given the current go api is not considered
stable and breaking change should be acceptable.

Fixes envoyproxy#38557

Signed-off-by: François JACQUES <fjacques@murex.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggreenway ggreenway ggreenway approved these changes

+1 more reviewer

@doujiang24 doujiang24 doujiang24 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@doujiang24 doujiang24

@ggreenway ggreenway

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hypnoce @doujiang24 @RyanTheOptimist @ggreenway