sourcehut: update all component; lots of fixes#245394
sourcehut: update all component; lots of fixes#245394tomberek merged 29 commits intoNixOS:masterfrom
Conversation
|
Thank you for taking the trouble of going through this much needed update. I'm in the process of setting up my first Sourcehut forge, therefore I'm sorry if I won't be able to provide much knowledgeable feedback, but I'll try my best. It seems that your PR suffers from the same problem as upstream where the On this note, what about updating the manual with a working example as well, since the one that is written there is no longer valid? Maybe include something like the one in #133984. But maybe documentation can be updated as part of a separate PR, depending on how much work this one requires. |
Thanks for trying it out and catching that! In my config, I have set it explicitly, since I don't use/need all services. I'll fix it and will set set up a test server with all services enabled for the future.
Good point, I will do that too. Should not be too much of a hassle, I think. |
Apparently, setting it explicitly was the old way of doing it, now replaced by the Another pain point is the need for all those OAuth IDs and secrets to be set manually, but I guess it would be non-trivial to come up with a safe way of generating them. Maybe this will have to be addressed in a future PR, but at least the documentation should be explicit about that. |
Well, then I'd suggest to explicitly deprecate setting the
Yeah, secret management would definitely push this PR to far. That should be brainstormed separately, I'd rather have this PR go somewhere sooner than later. In any case, big thanks for helping out with this! |
5ab70b2 to
401f82a
Compare
|
I can confirm that, at this moment, your PR seems to be working. I managed to bring up both I noticed you have chosen to deprecate Apart from that, during the setup process I found two sources of frustration that, maybe, we could address in this PR. The first one is that secret management is terribly inconsistent between secrets that are to be included in the INI files and those that, instead, are accessed by path. Those that are to be written to the INI files are the easiest to manage, as the activation script decrypts them as root and copies their contents verbatim to the INIs, meaning that the secret management utility that one uses (in my case, agenix) doesn't have to treat them in any special way. The major issue with secrets that are accessed by path, though, is that nowhere it is written that Sourcehut services run in heavily-restricted chroots, and therefore don't have access to the filesystem. If it weren't for your NixOS configs I found on your repo (thanks for that), I would have never guessed that I had to set the The other issue is that different services run under different users/groups, meaning that one has to either replicate the secrets and assign each one to the user of the service, or create an ad-hoc group with the various services' users, and assign it the ownership of the shared secrets. Is there any way in which we could handle secrets better? Personally, I went for an additional Lastly, something still related to the GPG key but that can be solved through documentation: Sourcehut expects keys in armored format, not binary, but this is not told anywhere. In the coming days, if I have time, I will try to spin up a |
Yeah, was my first thought too, but there isn't something like
Thanks for pointing that out, I totally forgot about that issue due to having it "solved" in my config. I'll fix that 👍
Hm .. having a separate group for all (shared) secrets does sounds like a good idea, I think. Maybe even something like
I will update the documentation w.r.t. that when I get around to that issue. The doc is definitely lacking lots of things.
Thanks! If something non-obvious comes up, I happily can help you out. Also, would be good to take notes of such things, so that I can improve it or add it to the documentation, as appropriate. |
I totally see your point, both solutions are acceptable, I think. I was just wondering which way would be better, but it's totally up to you.
The group hack was the least convoluted solution to the problem without having to modify the module, but maybe it's not the cleanest as an upstream technique. I would not assume that all services share the same subset of secrets, therefore we would violate the least-privilege/need-to-know principle by doing that. Since we're building chroot jails for all services, we might as well copy and chown the necessary secrets inside each jail upon service activation, letting the derivation of each Sourcehut service choose which secrets it wants. I think this can be delegated to some logic near the generators for their Systemd services and INI configurations. Since the (encrypted) secrets are kept under the Nix store and tied to the system derivation, any change of secrets would entail a change of inputs, triggering a rebuild of the jails and reload of the services, so we shouldn't have any problems with stale secrets, either. Well then, I'm off to setup this |
|
Alright, here I am with a status update. First, let's begin with the easy stuff. The issue with the shared secrets had already been found in #177423, and #177423 (comment) suggests to address it by relying on the Moving on. Setting up in order to produce the necessary derivation. Finally, for the difficult part, I tried to run the service. The web UI works correctly, and I submitted the following manifest to test everything out: This run produced an error 500 response, the job is stuck in the This error is utterly incomprehensible to me, so I hope it is something that can be easily solved on the GraphQL side, but I have no idea what GraphQL is apart from the obvious meaning of its name. Maybe is just a misconfiguration on my part. Anyway, this concludes the status report. I can see that there are some small issues that can be tackled straightforwardly, so maybe it is wiser to chip at Sourcehut little by little, opening some spun-off PRs and then rebasing this larger update onto them? But I don't know how the rest of the updates come into play in all of this. In any case, I am more than willing to help by spinning off a |
|
I was about to send you a private email, good timing! Just let me learn how to send patches via email and I'll post it to the list. |
|
Time for another status report. The modification to As for the long stack trace generated while trying to submit build manifests, I might have found a related patch in patches/redis-socket/core/0001-Fix-Unix-socket-support-in-RedisQueueCollector.patch that is worth a look, I think. I will try to do that either tomorrow or soon after. Then, I decided to try out The cause behind this failure is that This defect could have been caught by a simple test waiting for a socket to appear on the API port, so I suggest we add something along those lines to verify that these backend components are actually up and running. I guess very few people self-host their pastebins on NixOS machines... |
|
Getting all the services to work together and configured was a hurdle last time I was looking at this. There were some cases where there was so much service isolation that they couldn't communicate. Are there tests we can add to to make this less fragile? |
|
I would advise some narrow-scoped integration tests that carry out some basic inter-component transaction, in addition to simple tests to verify that all components are up. For example, pushing a new repo to By the way, @tomberek, since you are the original maintainer of this module, do you have any working configuration for a multi-service Sourcehut installation to show us and that can guide us through some of the basic pitfalls? Many of the hurdles that I am encountering are being solved by reading other people's configs. |
|
I don't know if I am stating the obvious, but it seems that the Also, one day we should refactor this module to respect RFC42, because the amount of options is hideous and the repetition is disturbing. |
|
Found it. At line 22 of When running in the default local configuration, the Redis store is on the same machine as the worker, so the connection is established via socket, resulting in a URI with a The reason why nobody reported this bug before is probably because everyone using What can we do now? I think that, for the time being, we should either patch the Go source code with something that mangles the schema into the right form (difficult), or we should make it so that the local Redis instance, at least for |
|
With a simple modification to my system configuration, I was able to make Sadly, it seems that In addition, the Again, this is an easy issue to solve, as the reason for commenting that line is no longer applicable, any future re-occurrence of the problem can be easily side-stepped, and the absolute paths can all be replaced by store paths and build-time parameters. What troubles me the most is that, once again, this bug has been sitting there for more than a year, with 3 intervening NixOS releases shipping broken code and no failure detected by the testing suite. That said, I begin to wonder if it is meaningful to hold back on the merge of this PR, @christoph-heiss. Your updates are not breaking any of the core services that people seem to actually use ( Or, maybe we should ask for help on Discourse, and have other Sourcehut administrators provide suggestions. @fgaz, sorry to trouble you again, but I noticed that you lamented the lack of meaningful Minio integration in #164440. Does this mean you are using any other service besides |
iirc I stumbled upon that issue while attempting to test a pages.sr.ht patch on which I gave up, so I don't have anything else sorry |
|
It's fine it's fine, better that way. Thanks! |
|
Success reports follow. First of all, the fix of the The patch to the As for
I finally succeeded in running the NixOS image. Great success! Still a problem remains for All (temporary) fixes that I applied can be found at my Nixpkgs fork. I have yet to decide which service to test next, but probably I will write a checklist with all we've tested and with all we are yet to test, so that people may come over and help out without going through my walls of text. |
|
Oh, and I just noticed #202608. Sad story indeed, I can resonate with much of that frustration. @apfelkuchen6 I would love to hear your opinion! If you have time and are not nauseated by this module, of course... |
|
|
I know this is a large amount of work. I apologize for not testing and reviewing yet. |
Signed-off-by: Christoph Heiss <christoph@c8h4.io>
Signed-off-by: Christoph Heiss <christoph@c8h4.io>
Signed-off-by: Christoph Heiss <christoph@c8h4.io>
This breaks the (already fragile) gitsrht-dispatch -> gitsrht-keys command chain. Signed-off-by: Christoph Heiss <christoph@c8h4.io>
Signed-off-by: Christoph Heiss <christoph@c8h4.io>
…le` flags Signed-off-by: Christoph Heiss <christoph@c8h4.io>
Signed-off-by: Christoph Heiss <christoph@c8h4.io>
Signed-off-by: Christoph Heiss <christoph@c8h4.io>
An empty log directory, in case it stays unused, does not hurt anyone. Signed-off-by: Christoph Heiss <christoph@c8h4.io>
These are needed, as the used sourcehut version is not compatible with the newer major-releases for both packages. Signed-off-by: Christoph Heiss <christoph@c8h4.io>
Signed-off-by: Christoph Heiss <christoph@c8h4.io>
Signed-off-by: Christoph Heiss <christoph@c8h4.io>
Signed-off-by: Christoph Heiss <christoph@c8h4.io>
f853c04 to
88a3d2a
Compare
|
I have rebased this branch on the current master and addressed most of @h7x4's feedback. I also had to add overrides for two more packages, which got major releases in the meantime. And a fix for PostgreSQL >= 15 was also needed. This PR has now gone way beyond its scope. I'm not willing to do any more changes, as otherwise this will stay open forever. Either it gets merged as-is or never. I cannot justify wasting more time & energy, just to constantly keep up with the master branch. |
There was a problem hiding this comment.
I just a couple of small observations around other places that still had set -x
This PR has now gone way beyond its scope. I'm not willing to do any more changes, as otherwise this will stay open forever. Either it gets merged as-is or never.
Just saw this comment. Please ignore since I totally can understand where your coming from
| source = pkgs.writeShellScript "srht-dispatch" '' | ||
| source = pkgs.writeShellScript "srht-dispatch-wrapper" '' | ||
| set -e | ||
| set -x |
There was a problem hiding this comment.
did you still need mean to set -x here?
| ]; | ||
| environment.systemPackages = optional cfg.meta.enable | ||
| (pkgs.writeShellScriptBin "metasrht-manageuser" '' | ||
| set -eux |
There was a problem hiding this comment.
another one set -x that could potentially be dropped
| extraService | ||
| ])) extraServices) | ||
|
|
||
| # Work around 'pq: permission denied for schema public' with postgres v15, until a |
There was a problem hiding this comment.
thanks for the note here
|
Result of 2 packages blacklisted:
21 packages built:
|
tomberek
left a comment
There was a problem hiding this comment.
Build and functions as expected. NixOS tests pass.
(also did testing on x86_64-linux)
|
@christoph-heiss : thanks so much for the work. The constant update and rebase is not fun. (this is mostly my fault, i've not been using or testing along the way, making it much slower to do review). |
|
I'll try to make it quick, but I won't assure that I will be able to land all the fixes before the branch-off. Great thing that at least we have a refreshed package set and fixed service module, now! Thank you, see you soon! |
Description of changes
This updates all sourcehut components to their latest release; as of 23-07-2023.
Additionally, some things changed and had to be adapted. Further, some bug fixes (notably, should fix #199778, #201424 and #201425 IIRC).
I'm currently only running the metasrht and gitsrht services, thus the other are basically untested (apart from running
nixosTests.sourcehut). I will do that once I have the spare time, but I thought to open a (draft) PR anyway, such that other people can chime in and maybe even help with testing. It's a rather big thing that definitely needs more than one pair of eyes.Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)