-
Notifications
You must be signed in to change notification settings - Fork 183
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
H4HIP: Plugin-first architecture #388
base: main
Are you sure you want to change the base?
Conversation
To-do: - [ ] Finish simplifying language in this HIP - [ ] Create follow-up H4HIP to deprecate legacy CLI plugins (require converting to CLI-type Wasm plugins) Signed-off-by: Scott Rigby <scott@r6by.com>
Signed-off-by: Scott Rigby <scott@r6by.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I point out a couple sub-items below, here is my quick high level review ....
- The scope here seems like it's larger than the time we have to deliver. Feature freeze is in August. Can we get something documented where the feature functionality can be delivered by then?
- There seems to be a concept that SDK authors will want to have plugins as WASM for core functionality. The WASM runtime delivered with Helm will not be a high performance one. The high performance ones require cgo and are not portable to the extent we want. For an SDK user (who knows Go) to take functionality, compile it WASM, and then have that executed by a low performance runtime seems like a lower quality UX than an SDK user having clean interfaces and components all in Go. Am I wrong?
- There is a lot open for discussion in this proposed change. What is the design being proposed for Helm v4? I think the rest can be cut out so that we do not go down rabbit holes discussing the parts not for Helm v4.
- I like the idea of swappable components. For example, rendering. I can see that being a cross platform CLI plugin (and WASM could work well here).
- What about the idea of chart plugins being packaged with the chart itself? This is a trade-off. You have increased chart size but consumers of the chart do not need to find the plugins needed to use the chart.
hips/hip-9999.md
Outdated
1. initially, Helm will only work with plugins developed by and/or under the control of the Helm org until we get the interfaces more fully established. | ||
1. A verification process will be developed to ensure trusted plugins. | ||
1. A process will be established to ensure secure discovery and distribution of plugins. | ||
1. we can allow community plugins |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few thoughts on this...
- How will Helm verify where a plugin came from and stop it from running? Is this going to be with some cryptographic key for signing/validation? Writing this isn't going to be straight forward.
- Who will do the verification for trusted plugins? And, how do we do this in a way that's vendor neutral? There is going to be a fair amount of people/process here. If there is a verification process, this is going to be required. Where do the people come from to execute on this?
- Companies are going to want to have private plugins. How is this going to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I've removed the verification process line in fb797e5. That was a (very conservative) stopgap for discussion, from a concern that a bad actor chart author may try to take advantage of users testing this out during development of the feature and before a secure process is established for discovery and distribution of plugins. I think that was overkill for this HIP, and yes not realistic for our timeline.
hips/hip-9999.md
Outdated
1. **Explore WASM Integration**: Evaluate WASM runtime options and integrate WASM support into the plugin framework. | ||
1. **Create Working Protopype:** Prove the viability of this proposed architecture with an initial, functional prototype | ||
1. **Build Plugin Framework**: Implement the core framework for plugin management and invocation. | ||
1. **Define Plugin Interface and Types**: Develop standardized interface for plugin execution, and define the initial plugin types. | ||
1. **Build Initial default Plugins:** Move Helm 3's Chart API v2 functionality into initial plugins, which will be the default for Chart API v2 charts. | ||
1. **Ensure Secure Distribution**: Establish a secure process for plugin discovery and distribution. | ||
1. **Develop Documentation**: Publish comprehensive guides for users and plugin developers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feature freeze is in approximately 6 months. Can you put this implementation plan into a timeline that meets the freeze time? Note, docs don't need to be completed in that window but the other parts do. I'm skeptical that we have the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions based on the initial draft proposal
hips/hip-9999.md
Outdated
WIP | ||
|
||
- **Legacy CLI plugins**: these are the traditional subprocess plugins that Helm 3 would call as user-specified binaries, and which weould extend the Helm CLI as new subcommands. | ||
- **Helm 4 default plugins**: these are of 2 kinds: Chart API v2 and Chart API v3 the default plugins. These should be bundled with Helm 4 so that chart API v2 charts still function exactly like they did in Helm 3. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the default plugins" as described by the two kinds is somewhat confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok thanks—will update the HIP to make sure this part is self-explanatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sabre1041 does this make more sense? bbdca4e
- move to a smaller Helm core, offloading functionality to default plugins, which can be either swapped out or extended with additional plugins to support advanced use cases | ||
|
||
### Goals | ||
- define different types of plugins for each part of existing Helm functionality (we could summarize all in-scope Helm functionality as fitting into different phases of the Helm packaging and release lifecycle) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- define different types of plugins for each part of existing Helm functionality (we could summarize all in-scope Helm functionality as fitting into different phases of the Helm packaging and release lifecycle) | |
- define different types of plugins for each part of the existing Helm functionality (we could summarize all in-scope Helm functionality as fitting into different phases of the Helm packaging and release lifecycle) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we want to define different categories of plugins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely. This is what's meant by this step under "Implementation plan":
Define Plugin Interface and Types: Develop standardized interface for plugin execution, and define the initial plugin types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think this is clear in the first goal at the top:
- define different types of plugins for each part of existing Helm functionality (we could summarize all in-scope Helm functionality as fitting into different phases of the Helm packaging and release lifecycle)
@sabre1041 what do you think?
- define different types of plugins for each part of existing Helm functionality (we could summarize all in-scope Helm functionality as fitting into different phases of the Helm packaging and release lifecycle) | ||
- ultimately move as much existing functionality/code as possible out of Helm core into default plugins | ||
- this will allow us to more clearly define the core value proposition of Helm (which users must accept otherwise don't use it) | ||
- and will let users modify nearly all Helm functionality except that core value proposition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a separate bullet item? Also, what is the definition of Helm's "core value proposition"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These indented bullets below the goal to "ultimately move as much existing functionality/code as possible out of Helm core into default plugins" are meant to be a list of benefits of doing that. I will clarify this.
- Thanks to Extism and Wazero maintainers for guidance | ||
|
||
<!-- Ideally we'll also be able to say: | ||
- Thanks also to Fermyon, Cosmonic, and Dylibso for guidance and collaboration on Wasm integration in Helm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Thanks also to Fermyon, Cosmonic, and Dylibso for guidance and collaboration on Wasm integration in Helm. | |
- Thanks also to Fermyon, Cosmonic, and Dylibso for guidance and collaboration on WASM integration in Helm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be Wasm not WASM, even though the doc already contains lots of the latter. Will fix all of these after I commit your other applicable suggestions.
@mattfarina - it's not uncommon to think CGO is required for high-performance Wasm execution in Go programs, but thanks to the many great contributors to the Wazero pure-go runtime, it's not the case. Wazero does not require CGO, and in the same way other high-performance Wasm runtimes do, compiles Wasm bytecode to native x86 and arm64 instructions. On other architectures, it would run the interpreter, which is less performant, but quite uncommon to encounter. Extism's Go SDK simply provides a more accessible & generic interface to Wazero so that plugin developers are not burdened with lower-level memory management like they would be directly using Wazero. In effect, you get high-performance plugin code execution, with all the benefits of the sandboxed environment brought by Wasm, without the cross-compilation and linking complexity of CGO. |
Signed-off-by: Scott Rigby <scott@r6by.com>
Signed-off-by: Scott Rigby <scott@r6by.com>
Co-authored-by: Andrew Block <andy.block@gmail.com> Signed-off-by: Scott Rigby <scott@r6by.com>
Co-authored-by: Andrew Block <andy.block@gmail.com> Signed-off-by: Scott Rigby <scott@r6by.com>
Signed-off-by: Scott Rigby <scott@r6by.com>
Co-authored-by: Andrew Block <andy.block@gmail.com> Signed-off-by: Scott Rigby <scott@r6by.com>
c4faf31
to
332a982
Compare
… just one of the examples, not necessary for using wazero Signed-off-by: Scott Rigby <scott@r6by.com>
Co-authored-by: Andrew Block <andy.block@gmail.com> Signed-off-by: Scott Rigby <scott@r6by.com>
Co-authored-by: Andrew Block <andy.block@gmail.com> Signed-off-by: Scott Rigby <scott@r6by.com>
Signed-off-by: Scott Rigby <scott@r6by.com>
Signed-off-by: Scott Rigby <scott@r6by.com>
Signed-off-by: Scott Rigby <scott@r6by.com>
Signed-off-by: Scott Rigby <scott@r6by.com>
Signed-off-by: Scott Rigby <scott@r6by.com>
…mplications section Signed-off-by: Scott Rigby <scott@r6by.com>
Signed-off-by: Scott Rigby <scott@r6by.com>
…tioning legacy plugins using subprocess Signed-off-by: Scott Rigby <scott@r6by.com>
Signed-off-by: Scott Rigby <scott@r6by.com>
…s with immediately actionable items Signed-off-by: Scott Rigby <scott@r6by.com>
…architecture Signed-off-by: Scott Rigby <scott@r6by.com>
Preview: https://github.com/scottrigby/community/blob/h4hip-plugin-first-architecture/hips/hip-9999.md
To-do: