-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
RFC - Support safe subdirectory-based plugin conf loading #1052
RFC - Support safe subdirectory-based plugin conf loading #1052
Conversation
86d1bcf
to
455c465
Compare
One question here - we are still carrying non-conflist format support in Otherwise things get quite messy with essentially 3 generations of config file support implemented in libcni, which I don't think we need to carry. |
Basic impl up for people to concretely argue with, which should help. Note that there are no tests yet, I want to get a feeling for what people think before I add tests. |
Does this means CNI no longer support old CNI version which supports conf file? If so, all version will be deprecated other than 1.0.0. |
I'm soliciting good arguments around why we should keep In semver, you increase the major version to indicate to consumers of your {spec, library} that you are breaking backwards compatibility. We did that three years ago with the spec, but kept back compat parsing in libcni to ease the transition to 1.0.0. Anyone still using the non-list format hasn't been following the spec for the last 3 years. Enabling that makes No one is writing conf files in the old format AFAICT, and shouldn't have been since 1.0.0. We only support spec version 1.0.0 in |
@squeed & others: this should be closer to what you wanted, PTAL. As per discussion in the Jan 29, 2024 CNI WG:
|
4f4a7c0
to
a33b3b3
Compare
25c089f
to
791fbb6
Compare
@squeed PTAL - inverted flag as discussed. |
48401bc
to
5e3175a
Compare
23ffb6f
to
9a1d54e
Compare
9a1d54e
to
8e80e5c
Compare
Also threw up #1081 based on some of the discuss here, as a start. |
dd2f82b
to
2d76833
Compare
This would need to be coordinated :-/ However we should do this unless people have a case for a CNI network configuration that executes a single CNI binary. |
9882532
to
a805036
Compare
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.
Looks basically good, but please split in to 3-or-so commits (spec changes, go renames, and implementation). Thanks!
af69a5a
to
2339558
Compare
Yep, had it like that at one point. Fixed up, ty! |
Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
…orking#928) Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
2339558
to
7657c85
Compare
Resynced - since this already adds a type alias (NetConf -> PluginConf) it replaces some of #1097 (but continues to follow the same basic approach). |
…g#928) Signed-off-by: Benjamin Leggett <benjamin.leggett@solo.io>
7657c85
to
4254258
Compare
Designed to address #928 (and related issues such as #878), largely building off the ideas in #928.
The basic problem is that since plugin config is stored in a centralized, non-locked config file on the node, if multiple node agents install or remove CNI plugins in the same chain, they are both mutating a shared node resource with no locking, which unavoidably leads to unfixable TOCTOU errors.
This resolves that by moving the plugin configs out of the main shared node config, and into subdirectories, Unix init-system style.
One thing that is TBD is whether we rev the network config object spec for this or not.Proposal: drop-in directories #928 describes a mechanism where the subdir-based loading is dynamic and not explicit in the top-level config.This approach revisions the network config object spec to remove the oldplugins
array, more clearly indicating that subdirectories will be used instead, and forcing a hard version bump so it is not ambiguous as to what runtimes support this.This adds a new config flag
loadPluginsFromFolder
- if present, for a given named networkbar
, plugin configuration objects will be loaded from<path-to-bar-network-config-file>/bar/xxx.conf
If this flag is not present, inlined plugin configs will be handled as they were previously. If this flag is
true
and there are inlined plugin config objects in the primary network config, thelibcni
config loading will combine everything into one plugin list.Current things to note:
This will rev the config spec from 1.0.0Given that we still keep support for pre-1.0.0 spec formats inlibcni
, and this proposal will add yet another post-1.0.0 spec format, and we should support both 1.0.0 and post-1.0.0 formats inlibcni
for transitional purposes, that leaves us withlibcni
's API surface supporting 3 different versions of the spec, 2 of which would be non-current.deprecated
.