{nixos, nixos/tests}/traefik: re-refactor traefik, update tests - #490985
{nixos, nixos/tests}/traefik: re-refactor traefik, update tests#490985jackrosenberg wants to merge 11 commits into
Conversation
jackrosenberg
changed the title
nixpkgs-ci
Bot
added
10.rebuild-linux: 1-10
|
@TheRealGramdalf @SigmaSquadron @adamcstephens continuing the conversation in #490814, My preference goes toward removing the I have tested extensively, especially in regards to Fernando's changes here. They work as expected, but are also needlessly complex. |
|
Furthermore, we need to revamp the testing of this module. I propose integration testing with pangolin or some other complex module that depends on traefik. Open to suggestions/feedback here. |
6a0597c to
e214125
Compare
e214125 to
e9f5ac0
Compare
nixpkgs-ci
Bot
added
8.has: package (new)
e9f5ac0 to
d3be4dd
Compare
nixpkgs-ci
Bot
added
the
2.status: merge conflict
8048ce6 to
d779e78
Compare
|
@ofborg test traefik |
|
I understand that concern, but this isn't blindly generated LLM garbage (slop). I put a lot of thought into it myself and I manually checked every line of code. Most of it diffs cleanly against the original PR code or even is the original code just repositioned in the config tree to make it work with the |
|
I've marked this as a draft following @jackrosenberg's wishes to rollback all AI-assisted code, a decision that I agree with. LLMs do not understand the complexities of the module system and NixOS module design, and I have already seen a couple of AI-generated mistakes in the new code. |
|
@TheRealGramdalf would you please move your commits to a seperate PR, based on this one? |
nixpkgs-ci
Bot
added
the
2.status: merge conflict
|
Following up on the draft: I've rebuilt my attrTag refactor as a commit series on top of 162b5d3 (your last commit), so it no longer touches or references any of the work that's in question. Branch: This comment is long because it carries the reasoning behind each decision. The non-trivial commits carry that same reasoning in their messages, so you can also just go through the series commit by commit. Same disclosure as before: LLM-assisted, but the design and the reasoning are mine. What I first posted was a proof of concept; since then I've worked it through properly, and every line of these commits has been through my own review. I understand the concern about LLMs. For this series I can only say that the tests show what the module actually generates, and that Traefik serves it. I'd just like it reviewed like any other patch, and if something is wrong I'd rather know. A few commits build on TheRealGramdalf's work from this PR and carry his Co-authored-by; since you've asked for those commits to move to their own PR, I checked those pieces especially carefully against the module evaluation and the VM test before keeping them. For context on why I'm in this module at all: this grew out of #487367 (NetBird). The NetBird stack runs fine behind nginx, but their newer built-in reverse proxy is a separate feature that needs Traefik: the NetBird proxy manages its own certificates, so whatever sits in front of it must not terminate TLS, and Traefik is the only proxy NetBird supports for that. That work is still WIP and not yet tested against this series. I'll follow up when it is. I'd like to help move this module forward wherever I can. What's in the seriesThe main goal was to make the mutually exclusive options unrepresentable by construction, instead of having assertions catch them afterwards. The implementation itself stays as close to Traefik's own configuration as possible: if you have used Traefik before NixOS, your existing config should translate almost key for key, and the examples from Traefik's docs carry over directly. The three docs-mirror nodes in the test check exactly that. Nine small standalone fixes come first (see "What I hit" below), then:
Why the type-level splitAt With the attrTag split, options hold only user configuration and the module's own derived values are merged at generation time — there's nothing to tell apart afterwards, and what you write is what Traefik gets: How the options flow, before and after (diagrams LLM-assisted)at my attrTag refactor: If we want to expose the derived final install config we still could do so by putting it in a What I hit at
|
| directory = mkOption { | ||
| default = cfg.routing.dir; | ||
| defaultText = literalExpression "cfg.routing.dir"; | ||
| description = "Load dynamic configuration from a file."; |
There was a problem hiding this comment.
Is this description flip flopped with the file description above?
|
@ulic75 I'm not sure if this PR is in a good state for review. You could review the changes up until 162b5d3: 3836cc44c259f27c5e3fc15e3e9aa9585a555334^...162b5d3 If you're very invested you might also like to check my previous comment. But I understand if you don’t have time for that 😆 Edit: jackrosenberg just dropped the commits. |
c208a98 to
51eed21
Compare
|
I've force pushed back to 51eed21. I truly don't mean to erase other's work, but will not have LLM assisted work in my PR. Feel free to open a separate PR based on this one. I will be fixing the conflicts in this and polishing this up in the next few days :). Hopefully we can finally get this rework merged, and open up some room for future work! |
|
I understand that you don't want LLM assisted work on your PR. To speak for me: I didn't open a "competing" PR to your work as it feels like an attack against you and if I would "steal" your work. This is the last thing I want. I'd really like to build and move this forward together :) As to the fixes you want to tackle: You might want to check out the first 9 commits from my changes: RafaelKr/nixpkgs@162b5d3...c5f9c10 Still I would appreciate some feedback on my attrTag implementation, as I really like the option interface it provides. |
…x configurations and failing testcases." Co-Authored-By: Fernando Rodrigues <alpha@sigmasquadron.net> Co-Authored-By: TheRealGramdalf <gramdalftech@gmail.com> Co-Authored-By: Florian Schubert <30634234+JamesofScout@users.noreply.github.com>
This function takes in the provider, owner and plugin name, and outputs a correctly-formatted derivation with the Traefik plugin, which can be `symlinkJoin`ed in the `/var/lib/traefik/plugins-local` folder. Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
…refactor traefik module Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net> Co-authored-by: Jack Rosenberg <github@jackr.eu>
Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
Signed-off-by: Fernando Rodrigues <alpha@sigmasquadron.net>
|
Rebased and fixed conflicts. I would appreciate another round of feedback if people are willing :) Looking to get this merged soon! |
If you meant to drop my work as well, I'm quite frustrated by this.
If you don't want AI in "your" PR, I'm happy to take my code elsewhere. If that's the case, I'd ask that you remain consistent and drop the code that was based on my initial work as well. As I said on matrix, that means basically everything except for the plugins option and fetcher. I'm happy to collaborate if you're open to it, but I'm not okay with my work being essentially stolen and getting kicked out of my own project. If you want to include my work, then you need to include all of it. Even if it was mildly assisted by AI to move things along quicker as you so desperately seem to want. |
|
TBH i'm pretty done guys. This PR is very large, and I'm tired of pulling and pulling to get this over the finish line. I am vehemently opposed to LLM usage in any way shape or form. I made this clear above. Seeing walls of LLM text as a response to me saying 'dont use LLM in my fork', motivated me to drop all commits that were not my own or @SigmaSquadron's. @TheRealGramdalf, I asked you to move your new commits to a separate PR because this one is huge, not because I want to kick you out and claim credit for your part of the work. |
I'm sorry for the part I played in this. I've just been very busy, though I'm afraid that's about the most I can say - I'd prefer to keep the details private.
To each their own; I can respect that. Thanks for clarifying.
Thank you for making that clear, and sorry if what I said came across the wrong way. I didn't intend to be accusatory, rereading it now I can see my frustration getting the better of me. Apologies for the kerfuffle. Going forward, here's what I propose:
Does that sound reasonable? |
|
Here's the code that I removed for the plugins: Located under the # TODO make sure this properly replaces `mkIf` statement as intended
experimental.localPlugins = mkOption {
default =
default=
lib.listToAttrs (
map (
plugin: lib.nameValuePair plugin.plugin { inherit (plugin) moduleName; }
) cfg.localPluginPackages
);
defaultText = literalExpression ''
default=
lib.listToAttrs (
map (
plugin: lib.nameValuePair plugin.plugin { inherit (plugin) moduleName; }
) config.services.traefik.localPluginPackages
);
'';
example = {
"wasm-plugin-name".settings = {
envs = [ "SECRET_ENV" ];
mounts = [ "/path/to/mount" ];
};
};
description = ''
Local plugins allow you to use plugins from a local directory, without publishing them to the Traefik plugin catalog.
::: {.note}
By default, this will create an attribute set from the option `cfg.localPluginPackages`. To add a plugin from a package, use that option instead.
:::
'';
};Located at localPluginPackages = mkOption {
default = [ ];
type = listOf package;
example = literalExpression ''
[
pkgs.geoblock
pkgs.fetchTraefikPlugin
{
plugin = "plugindemo";
owner = "traefik";
version = "0.2.2";
hash = "sha256-6MuKVvtHUtWuibjUMZknOEklzaHQUjRYHvXdP2QqE6c=";
}
];
'';
description = ''
List of plugin packages to be added to the `localPlugins` attribute in the install configuration.
These plugins can be packaged in Nixpkgs, or [fetched directly](#module-services-traefik-plugins-custom)
'';
};Add to warnings =
# TODO check for functionality as intended
# TODO does/can this show where the definition location is (i.e. what file of the user's config)?
++ optional (!(builtins.all (plugin: plugin._isTraefikPlugin or false) cfg.localPluginPackages)) ''
Some of the Traefik local plugins in 'services.traefik.localPluginPackages' may be misconfigured.
The following paths are built from derivations that do not have the '_isTraefikPlugin' attribute set to 'true':
- ${
concatMapStringsSep "\n- " (badPlugin: badPlugin.outPath) (
filter (plugin: !plugin._isTraefikPlugin or false) cfg.localPluginPackages
)
}
'';Add to tmpfiles settings # Symlink package directories (in the nix store) to the `plugins-local` folder
# This path is hard coded, and should be placed in the working directory of the process running the Traefik binary.
# TODO What happens to old symlinks? it appears they would just pile up indefinitely.
(mkIf (cfg.localPluginPackages != [ ]) {
"${cfg.dataDir}/plugins-local"."L+" = {
argument = toString (
pkgs.symlinkJoin {
name = "traefik-plugins";
paths = cfg.localPluginPackages;
}
);
};
})NixOS Manual: "module-services-traefik-plugins": [
"index.html#module-services-traefik-plugins"
],
"module-services-traefik-plugins-custom": [
"index.html#module-services-traefik-plugins-custom"
],Plugins {#module-services-traefik-plugins}When using the structured The {option} {
services.traefik = {
plugins = [ pkgs.geoblock ];
install.settings.entryPoints.websecure.http.middlewares = "my-geoblock";
routing.settings.http.middlewares.my-geoblock.plugin.geoblock.countries = [ "NL" ];
};
}Custom Plugins {#module-services-traefik-plugins-custom}Plugins that are not currently packaged in Nixpkgs can also be added to the {
services.traefik.localPluginPackages = [
(pkgs.fetchTraefikPlugin {
plugin = "traefik";
owner = "plugindemo";
version = "v0.2.2";
hash = "";
})
];
}For plugins not found in {
services.traefik.localPluginPackages = [
(
(lib.fileset.toSource {
root = ./my-plugin;
fileset = ./my-plugin;
})
# Supress Traefik module warning.
# Don't forget to ensure that ./my-plugin has an appropriate
# directory structure as expected by Traefik.
// {
_isTraefikPlugin = true;
}
)
];
}And some todo list items: Future PRs
|
| # Building a Traefik plugin hosted on `plugins.traefik.io` | ||
|
|
||
| The following recipe shows how to use `fetchTraefikPlugin` to fetch the `geoblock` Traefik plugin at version `0.3.3`: | ||
|
|
||
| ```nix | ||
| { fetchTraefikPlugin }: | ||
| fetchTraefikPlugin { | ||
| plugin = "geoblock"; | ||
| owner = "PascalMinder"; | ||
| version = "0.3.3"; | ||
| hash = "sha256-gLhihjsBJEKGzpIwWzu/zmvLJbqnqXsF8ZiJWWKwDIA="; | ||
| } | ||
| ``` | ||
|
|
||
| After building the recipe, the derivation output will set up the appropriate paths expected by Traefik: | ||
|
|
||
| ```shell | ||
| $ nix-build | ||
| (output removed for clarity) | ||
| /nix/store/m68kmlay1vdjzqjq7144r6ra885fd8l8-geoblock-0.3.3 | ||
|
|
||
| $ tree /nix/store/m68kmlay1vdjzqjq7144r6ra885fd8l8-geoblock-0.3.3 | ||
| /nix/store/m68kmlay1vdjzqjq7144r6ra885fd8l8-geoblock-0.3.3 | ||
| └── src | ||
| └── github.com | ||
| └── PascalMinder | ||
| └── geoblock | ||
| ├── docker | ||
| │ ├── dev-geoblock | ||
| │ │ └── docker-compose.yml | ||
| │ └── traefik-config | ||
| │ ├── dynamic-configuration.yml | ||
| │ └── traefik.yml | ||
| ├── lrucache | ||
| │ ├── lru.go | ||
| │ ├── lru_interface.go | ||
| │ └── lru_test.go | ||
| ├── LICENSE | ||
| ├── Makefile | ||
| ├── geoblock.go | ||
| ├── geoblock_test.go | ||
| ├── go.mod | ||
| └── readme.md | ||
| ``` | ||
|
|
||
| :::: |
There was a problem hiding this comment.
This could use the demoplugin, that should be a pretty stable example which won't look out of place in a few years. There's a fetch example in my latest PR comment with the right parameters you can steal, but the tree will need to be regenerated
| + The Traefik module has been refactored to allow new ways to deploy the install and routing configuration files, as well as nixpkgs packaged plugins. See [Migrating traefik for pre 26.11 configurations](#module-services-traefik-migrating-to-26.11). | ||
| + | ||
| >>>>>>> conflict 1 of 1 ends |
There was a problem hiding this comment.
| + The Traefik module has been refactored to allow new ways to deploy the install and routing configuration files, as well as nixpkgs packaged plugins. See [Migrating traefik for pre 26.11 configurations](#module-services-traefik-migrating-to-26.11). | |
| + | |
| >>>>>>> conflict 1 of 1 ends |
I have this in my new PR, you can drop it for this one
| "module-services-traefik-usage": [ | ||
| "index.html#module-services-traefik-usage" | ||
| ], | ||
| "module-services-traefik-usage-routing": [ | ||
| "index.html#module-services-traefik-usage-routing" | ||
| ], | ||
| "module-services-traefik-usage-install": [ | ||
| "index.html#module-services-traefik-usage-install" | ||
| ], | ||
| "module-services-traefik-migrating-to-26.11": [ | ||
| "index.html#module-services-traefik-migrating-to-26.11" | ||
| ], |
There was a problem hiding this comment.
| "module-services-traefik-usage": [ | |
| "index.html#module-services-traefik-usage" | |
| ], | |
| "module-services-traefik-usage-routing": [ | |
| "index.html#module-services-traefik-usage-routing" | |
| ], | |
| "module-services-traefik-usage-install": [ | |
| "index.html#module-services-traefik-usage-install" | |
| ], | |
| "module-services-traefik-migrating-to-26.11": [ | |
| "index.html#module-services-traefik-migrating-to-26.11" | |
| ], |
Covered, can be dropped
| "module-services-traefik": [ | ||
| "index.html#module-services-traefik" | ||
| ], | ||
| "module-services-traefik-environment": [ | ||
| "index.html#module-services-traefik-environment" | ||
| ], |
There was a problem hiding this comment.
| "module-services-traefik": [ | |
| "index.html#module-services-traefik" | |
| ], | |
| "module-services-traefik-environment": [ | |
| "index.html#module-services-traefik-environment" | |
| ], |
Covered, can be dropped
| group = "fossorial"; | ||
| dataDir = "${cfg.dataDir}/config/traefik"; | ||
| staticConfigOptions = { | ||
| localPlugins = [ pkgs.fosrl-badger ]; |
There was a problem hiding this comment.
| localPlugins = [ pkgs.fosrl-badger ]; | |
| localPluginPackages = [ pkgs.fosrl-badger ]; |
I renamed this attribute to distinguish it in my PR
| meta = { | ||
| description = "Traefik plugin that handles authentication for Pangolin resources"; | ||
| homepage = "https://plugins.traefik.io/plugins/676da7c6eaa878daeef9c7e9/fossorial-badger"; | ||
| license = lib.licenses.mit; |
There was a problem hiding this comment.
| license = lib.licenses.mit; | |
| license = [ lib.licenses.mit ]; |
There was a discussion about this on matrix a while back, a list is preferred
| maintainers = with lib.maintainers; [ | ||
| jackr | ||
| ]; |
There was a problem hiding this comment.
| maintainers = with lib.maintainers; [ | |
| jackr | |
| ]; | |
| maintainers = [ | |
| lib.maintainers.jackr | |
| ]; |
| meta = { | ||
| description = "Traefik plugin that denies requests based on country of origin"; | ||
| homepage = "https://plugins.traefik.io/plugins/62d6ce04832ba9805374d62c/geo-block"; | ||
| license = lib.licenses.asl20; |
There was a problem hiding this comment.
| license = lib.licenses.asl20; | |
| license = [ lib.licenses.asl20 ]; |
| maintainers = with lib.maintainers; [ | ||
| jackr | ||
| ]; |
There was a problem hiding this comment.
| maintainers = with lib.maintainers; [ | |
| jackr | |
| ]; | |
| maintainers = [ | |
| lib.maintainers.jackr | |
| ]; |
Minor gripe, I can't say that it actually makes a difference performance wise (I've never been able to get a clear answer) but the general sentiment is to avoid with unless necessary.
There was a problem hiding this comment.
You'd have to double check, but I think I covered everything that needs to be added here in my PR comment. I would just drop everything while rebasing and copy the plugin stuff in afterwards to avoid merge conflicts. I can re-review once this is rebased.
9 participants
WIP WIP
Un-Reverts #490920, with the necessary fixes.
Closes #471071, traefik/traefik#12777
TODO:
pluginsoption, add documentation #437073