Registry and Plugin (Backend)#1511
Conversation
aa7d49f to
6aba20a
Compare
c01a4d1 to
91e3e10
Compare
|
Updated this PR so it doesn't touch existing plugins, that's probably a separate concern to decide what to add/remove from LRR. |
90930c8 to
cd86cf0
Compare
|
An update on duplicate plugins. There are many ways duplicates can happen with the entire plugin system, and the existing dev logic doesn't really handle it besides choosing a survivor and moving on. Now that a subset of plugins have crud, a duplicate does happen, then we might also run into myriad of other unforseen issues. At the moment, I don't think this PR will guarantee graceful duplicate handling, or handling of issues caused by duplicate plugins. LRR will perform plugin discovery on startup and report duplicates at warning level, and this PR will ensure that LRR plugin APIs do not introduce duplicates into the system. But past startup, LRR assumes a clean plugin state, and may not handle cases where a duplicate existed or is introduced by the user via other means. (unless loc budget/complexity isn't a concern) |
|
Removed channels in favor of semver, all registry plugin versions will be enforced at "x.x.x" style, and LRR will resolve installations by max semver version. Users can opt to install specific versions. Added stack-based rollback on install failure. If LRR fails to upgrade a plugin, for example, it's going to attempt to restore the previous plugin (which is an existing stance iirc). This is a one-time rollback; if the rollback itself fails, then we're not going to "rollback the rollback", instead return 500 error. Probably the (remaining) big item is where to place sideloaded plugins, I'll think about that after the local/git registry system is stable. Commit history is mainly for tracking what's happened. Once the pr matures (after I do another pass), I'll do a rebase and squash. |
|
The controller layer and OpenAPI contract are fixed, with exception of maybe some cosmetic changes but I won't be touching controller files anymore (unless something's really wrong). So this branch is free to api test. Next up will be improving model/utils-side of things. A lot of the code is validation and exception handling work, not exactly business logic, but I'll have to see what exactly needs changing instead of just moving things around. |
There was a problem hiding this comment.
I don't see a way to list available plugins by registry, is it part of another pr or I'm just blind?
I have a feeling empty strings will cause issues in the api.
Code looks fine maybe a bit overly verbose at least compared to the rest.
I will do a second pass when I get to test it on Windows.
No this isn't added |
|
One note: any request/response validation that can (and should) be done by OpenAPI won't be re-done by LRR. So this PR assumes that OpenAPI validation is enabled for any testing. |
|
Removed the metadata plugin features as I think we already have enough to go over as is |
|
basic cdn support is added |
|
Model layer (and above) are now reviewable (assuming the tests pass). Next up will be improvements to the Utils-layer. |
Guerra24
left a comment
There was a problem hiding this comment.
Looks fine to me, tested it on Windows and it works as expected. Model layer looks cleaner and is easier to follow.
|
The Utils layer (Plugin/PluginState/Registry) should be stable now. Going to start dogfooding on this with a gitlab/codeberg account + self hosted gitea. Unit and integration tests are still AI written, maybe we'll go through another round of mutation testing before I open it up for review. |
0f66f44 to
551e1f7
Compare
|
squashed, backup at https://github.com/psilabs-dev/LANraragi/pull/new/dev-registry/backend-backup |
|
Checked that registry on self-hosted gitea can be read by LRR (after some cert/dns shenanigans). |
|
Removed inter-worker plugin synchronization logic and added an Also moved plugin loadability out of process, so it doesn't contaminate the process INC cache via require. Should work on both windows via Karen's |
psilabs-dev
left a comment
There was a problem hiding this comment.
dobfood round 2
082e035 to
2c41b88
Compare
9ceea30 to
6252a88
Compare
6252a88 to
dc79c1f
Compare
Difegue
left a comment
There was a problem hiding this comment.
Just a few misc things, I think we can undraft this otherwise.
I think the needs_restart bits should be coordinated with #1673 since it feels like there's some overlap there. (At the very least, I suppose we should provide an API to restart the server)
| [OpenAPI lanraragi-api](https://raw.githubusercontent.com/Difegue/LANraragi/refs/heads/dev/tools/openapi.yaml) | ||
| {% endopenapi-operation %} | ||
|
|
||
| {% openapi-operation spec="lanraragi-api" path="/plugins/installed/{plugin_namespace}" method="delete" %} |
There was a problem hiding this comment.
I think the endpoint would be more sensible if we dropped the /installed
| {% openapi-operation spec="lanraragi-api" path="/plugins/installed/{plugin_namespace}" method="delete" %} | |
| {% openapi-operation spec="lanraragi-api" path="/plugins/{plugin_namespace}" method="delete" %} |
There was a problem hiding this comment.
DELETE "/plugins/{plugin_namespace}" and GET "/plugins/{type}" are indistinguishable param-wise (wrt openapi), we can have either one or the other as the methods live under the same URL
Alternatively, we could lift the managed plugins to the "/plugins/installed" URL, e.g. for installing plugins
| * [Tankoubon API](api-documentation/tankoubon-api.md) | ||
| * [Stamps API](api-documentation/stamp-api.md) | ||
| * [Plugin API](api-documentation/plugin-api.md) | ||
| * [Registry API](api-documentation/registry-api.md) |
There was a problem hiding this comment.
I think we need a doc page for Registries, can't really only have the API here.
I suppose docs can wait until the frontend makes it in, but I think we should write up the concepts and the types of supported registries already.
There was a problem hiding this comment.
(also an archi doc update for the new redis structures)
There was a problem hiding this comment.
Added some, though not sure how the docs should be presented with the UI missing 🤔 also idk how technical the docs should be...
|
|
||
| } elsif ( ref( $pluginfo->{parameters} ) eq 'HASH' ) { | ||
|
|
||
| # TODO: remove this line (and the ARRAY check above) |
There was a problem hiding this comment.
I get that this line is being removed because $namerds now also stores the registry information for the plugin, but we do still want to deprecate array parameters at some point... Maybe with the move to registries actually.
There was a problem hiding this comment.
I'd say that's something we can do.
Also this block was removed bc it clears all plugin data on config save (including prov), but that might be unclear, so I just reverted to commenting the whole thing out.
| my ( $redis, $namespace ) = @_; | ||
|
|
||
| my $namerds = "LRR_PLUGIN_" . uc($namespace); | ||
| $redis->hdel( |
There was a problem hiding this comment.
If we deprecate array params, we could change this to just delete the namerds key outright, right?
Might be worth adding a TODO here if I'm not mistaken.
There was a problem hiding this comment.
If we just delete the namerds key we would also nuke previous configurations/settings (like enabled for metadata). Which I'm personally fine with (users just need to reconfigure stuff and you're kinda asking for it by uninstalling), but this could be a UX decision.
59343c0 to
51ed1b6
Compare
this probably depends on merge order (but weakly). This PR only signals whether LRR needs to restart, but hitagi actually provides (or should provide) the restart APIs which the client can act on. Following servers like jenkins, LRR should not force a restart after installations, and instead advise the user to manually trigger one. It's only if this statement were reversed that we'll have two merge order cases to consider. |
and add a comment
|
Moved default registry to ougi remaining CI failure is addressed by #1677 |
Under testing, lots of code may be subject to change. There are quite a number of avenues where code might break. I don't recommend reviewing this code atm.
This backend-only PR
removes all (non-local) plugins from LRR, and implements the registry system and plugin management.Configuration-related features, such as metadata plugin priority, enablement, and dropdown hiding. Add metadata plugin priority support.Obligatory security warning, as downloading untrusted arbitrary files and allowing code execution from the internet requires extra care which was not a consideration when plugins were the responsibility of LRR. The boundary for this PR is that LRR will not audit or review files/registries/licenses for the user, the user must trust registries and audit them personally/take responsibility for any bugs/vulnerabilities within plugins.
Some examples of ways to be compromised, which LRR cannot protect against: registry URL typos/typo squatting, DNS poison/certificate updates, hijacked registries/supply chain attacks
(tbh, with this I'm also just wondering whether plugin installations should require a restart altogether)
(PS: maybe we should support uninstalling sideloaded plugins?)