build: allow to define additional configs

[a-gave]

This introduces the possibility of passing additional configs to the request.

Only the few not related to kernel/packages, as they are not recompiled by the imagebuilder, would make sense.
For example, to request a specific name/number for a firmware image:

CONFIG_VERSION_DIST=MyRouterOS
CONFIG_VERSION_NUMBER=1.6

Or to request a format other than the default format for the output firmware image, for example:

# CONFIG_TARGET_ROOTFS_SQUASHFS is not set
CONFIG_TARGET_ROOTFS_JFFS2=y

[aparcar]

Please fix the failing test.

[codecov]

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (6f634f8) 80.94% compared to head (60b45b9) 86.70%.

❗ Current head 60b45b9 differs from pull request most recent head 88f632b. Consider uploading reports for the commit 88f632b to get more accurate results

Files	Patch %	Lines
asu/build.py	18.18%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #728      +/-   ##
==========================================
+ Coverage   80.94%   86.70%   +5.75%     
==========================================
  Files          15       15              
  Lines         976      985       +9     
==========================================
+ Hits          790      854      +64     
+ Misses        186      131      -55     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Neustradamus]

What is the status of this PR?

[aparcar]

I'm not a huge fan to allow arbitrary changes but since we moved into containers, it might be fine. Is anyone interested in using this?

[a-gave]

Hi, rebased on current Git HEAD, let me know if there is any interest on this, and/or if there are things that should be improved. I would still be interested.

[aparcar]

Should we add a configuration which sets specific CONFIG options which are allowed? I'm a bit unsure this doesn't cause any issues down the road. For example you could simply bypass the root filesystem size limitation of rootfs_size_mb and set it to 100GB, filling up the servers storage...

[a-gave]

Should we add a configuration which sets specific CONFIG options which are allowed? I'm a bit unsure this doesn't cause any issues down the road. For example you could simply bypass the root filesystem size limitation of rootfs_size_mb and set it to 100GB, filling up the servers storage...

Thanks, this make a lot of sense, I've pushed some modifications to have an allow-list of configs, stored in settings.

[a-gave]

pushed fixes on formatting (https://github.com/openwrt/asu/actions/runs/12634720904)

[aparcar]

@efahl what do you think about this?

[efahl]

I'd say before proceeding that a clear case needs to be made for the specific config vars of interest, and that each would need a good description as to effects and possible issues. (If, for example, CONFIG_TARGET_ROOTFS_TARGZ is disabled, will that create huge uncompressed images that fill the server?) And, if there is a good case to be made for specific ones, they should be incorporated into the imagebuilder first, exposing them just as ROOTFS_PARTSIZE and EXTRA_IMAGE_NAME are...

But, I'm curious what ASU clients would use this?

And for some of the values, I think it would cause the client some problems. For example, if you change the rootfs to jffs2 via a CONFIG var, then all of auc, owut and LuCI app will not know what to do because they'll still be looking in profiles.json for a squashfs one. Also with rootfs type, there's the issue that the imagebuilders must have been created with the proper rootfs variants in the first place (see how owut handles this with its --fstype option validation).

[efahl]

Typo "Applying"

[efahl]

Why not do this filtering when you are creating the .config_local file?

[efahl]

Shouldn't the second clause also be anchored? As is, the second clause matches anywhere in the line.

[efahl]

Should be an f-string to evaluate the expression.

[efahl]

You could leave out that extra list call, just say sorted(set( as that has the same effect and eliminates an extra list-ification.

[efahl]

After sleeping on this, I think this is inappropriate for inclusion in the server. The philosophy of ASU server should be to provide a robust, secure solution for assembling images with as little attack surface as possible. This would add a little used hole in the API for no benefit to the existing ASU clients.

If a person wants to change imagebuilder config, they are already sophisticated enough to do so without involving the ASU server.

[a-gave]

I'd say before proceeding that a clear case needs to be made for the specific config vars of interest, and that each would need a good description as to effects and possible issues. (If, for example, CONFIG_TARGET_ROOTFS_TARGZ is disabled, will that create huge uncompressed images that fill the server?) And, if there is a good case to be made for specific ones, they should be incorporated into the imagebuilder first, exposing them just as ROOTFS_PARTSIZE and EXTRA_IMAGE_NAME are...

Thanks also for the reviews, I agree with that, and with considerations on asu. I will continue for a while to use a fork that does that. But I'm actually interested in the first two options, the others were added as possibilities that were present, but didn't generate interest, so I think they are not used, and at the limit as you say should be added to the imagebuilder. I had missed then that the use of JFF2 is also discouraged. [0]

[0] https://openwrt.org/docs/techref/filesystems#can_we_switch_the_filesystem_to_be_entirely_jffs2

[efahl]

If you do end up submitting something for imagebuilder to directly support your usage, tag me on the PR. I'd be glad to review and if it's accepted, then get it into ASU.