adguardhome: Added ujail dependency#29277
Conversation
|
You need to bump the |
Thx. Done. :) |
|
This all needs to be inside 1 commit. First letter after prefix in the commit subject must be lowercase. You also need to use your actual email and not GH noreply. |
|
The author ( Also not sure where this MIME thing is coming from, but it's noisy. |
In order to create a proper jail, we net the procd-ujail package. Otherwise, AdGuardHome will run as unprivileged process, and will not be able to listen on ports below 1024. Signed-off-by: Alexander Krause <alexander.krause@cs.tu-dortmund.de>
Hope that's fixed now. |
Formalities fail, so no: |
The actual Signed-off-By line in the commit looks quite ok to me. while the error message from @GeorgeSapkin hyperstickler seems to expect a dot-starting domain in email address? The actual S-o-B is ok, so I will merge this. (Sad that in the current CI the formalities do block the actual build testing.) |
|
That dot is coming from the patch:
I feel like I'm getting all the CI flack 😅, but I didn't make that decision. OpenWrt GH actions are usually overloaded, IDK what can be improved there. |
Well, I proposed to you and @BKPepe in openwrt/actions-shared-workflows#55 (comment) that at least the refresh test would be concurrent to the formalities. |
|
Look, I still stand by the fact that fixing formalities is such a trivial task that it should be instant, and we shouldn't have to spin up the whole machinery just to prep the build env, start the build, and spit out a package—whether it's just for one platform (which is wrong on principle alone) or for all of them. If we only do it for one platform, users on the others will scream about why it's missing for them. It comes down to Git configuration, which is always a complete nightmare for average users. It needs to be right, and above all, this shouldn't have been merged in the first place. It's nice that the S-o-B is there, but did anyone notice when opening the commit https://github.com/openwrt/packages/commit/e6b5141c7ea68f85b47abfd4904e0262782d43e3 that it simply isn't linked to a GitHub username? From my perspective, that's a real problem. If you look at the issues, you'll see users sometimes struggle to figure out who to contact because they can't @mention them—either the user doesn't exist, they haven't linked their email to GitHub, or something is just messed up somewhere, and that's exactly the case here. :-/ And yes, the ideal solution would be having more runners so we'd get results immediately, but that's not happening, as you can see in the recent pull requests. I did a rebase and waited over 2-3 hours just for a runner to pick it up and give a result. To me, that's far from ideal and genuinely terrible. |
| TITLE:=Network-wide ads and trackers blocking DNS server | ||
| URL:=https://github.com/AdguardTeam/AdGuardHome | ||
| DEPENDS:=$(GO_ARCH_DEPENDS) +ca-bundle | ||
| DEPENDS:=$(GO_ARCH_DEPENDS) +ca-bundle +procd-ujail |
There was a problem hiding this comment.
Are we sure this is the right approach? A hard dependency on procd-ujail doesn't seem correct to me.
Why should adguardhome force this dependency? If a user doesn't want to (or can't) useprocd-ujail(e.g., due to storage constraints on custom builds), this strictly limits them. The standard OpenWrt approach is to handle jailing optionally within the init script, rather than forcing it at the package level.
See how this optional jailing is handled in other packages:
https://github.com/openwrt/packages/blob/master/net/snort3/files/snort.init#L46
I believe this hard dependency should be reverted, and the jail configuration should be moved to the init script as an optional feature.
There was a problem hiding this comment.
Well, when I checked yesterday, the init script has no evaluation of ujail functionality, but has hardcoded in the usage of jail. So, dependency on ujail is ok from that angle.
On the other hand, it may be somewhat unnecessary, as ujail is default package when not SMALL_FLASH. So, without the dependency, the package may fail in some routers.
The standard OpenWrt approach is to handle jailing optionally within the init script, rather than forcing it at the package level.
Regarding "how other packages do it", there are some packages that nicely evaluate the availability of ujail, like bkpepe showed above. That would be the more perfect approach. But quite many packages just assume in init script that the functionality is there, just adguardhome here. Of the 18 jail users in this repo, 5 do evaluate and have optionality, 13 have hardcoded in the usage.
Due to the default status of ujail, both checking for the functionality and also a hard dependency can been seen somewhat unnecessary for the majority. Small flash routers are the ones, where difficulties may arise, others should be just fine.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds the procd-ujail runtime dependency to the OpenWrt adguardhome package so it can be started in a proper jail and still bind to privileged ports (<1024).
Changes:
- Bump
PKG_RELEASEfrom 1 to 2 - Add
+procd-ujailtoDEPENDS
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| TITLE:=Network-wide ads and trackers blocking DNS server | ||
| URL:=https://github.com/AdguardTeam/AdGuardHome | ||
| DEPENDS:=$(GO_ARCH_DEPENDS) +ca-bundle | ||
| DEPENDS:=$(GO_ARCH_DEPENDS) +ca-bundle +procd-ujail |
There was a problem hiding this comment.
Yeah, I think we need to enable Copilot reviews everywhere as it happens in main repo (or will happen shortly).
There was a problem hiding this comment.
Copilot missed that the init script here unconditionally forces the use of jail. Nice answer, otherwise.
Quoting copilot: "Adding +procd-ujail as a hard dependency is only correct if the package (init script/procd service definition) unconditionally uses ujail"
So, my interpretation of the answer is that as the init script unconditionally uses jail, the dependency is right.
(Is the unconditional usage a good practice, is then another question.)
This commit was merged into the master branch by accident and should be undone. Adding ujail as a hardcoded dependency is incorrect, as ujail is meant to be an optional dependency. A better approach is to implement ujail support within the init script, which was discussed in the pull request (#29277), consistent with how other packages in the repository handle this. Therefore, reverting for now. This reverts commit e6b5141. Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
This commit was merged into the master branch by accident and should be undone. Adding ujail as a hardcoded dependency is incorrect, as ujail is meant to be an optional dependency. A better approach is to implement ujail support within the init script, which was discussed in the pull request (openwrt#29277), consistent with how other packages in the repository handle this. Therefore, reverting for now. This reverts commit e6b5141. Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
In order to create a proper jail, we net the procd-ujail package. Otherwise, AdGuardHome will run as unprivileged process, and will not be able to listen on ports below 1024.
📦 Package Details
Maintainer: @GeorgeSapkin @hnyman
Description:
See commit message.
🧪 Run Testing Details
✅ Formalities
If your PR contains a patch:
git am