Add local testing mode to the buildmaster config #289
Conversation
This makes it (relatively) easy to test a builder setup locally. The buildmaster and the web interface should be bound only to local interfaces for security reasons (you can use ssh port forwarding if wanting to run on a server).
|
I know zero about doing this because I always ask @omjavaid to do it for me 🤣 It would be wonderful to have this working. The long cycle times working with buildbots can be difficult to deal with so more local testing is always welcome. |
| ####### Workers | ||
|
|
||
| c['workers'] = config.workers.get_all() | ||
|
|
||
| c['protocols'] = {'pb': {'port': 9990}} | ||
| c['protocols'] = {'pb': {'port': "tcp:9990:interface=127.0.0.1" if test_mode else 9990}} |
There was a problem hiding this comment.
I not 100% sure on this but I believe putting interface=127.0.0.1 will make test buildmaster inaccessible from the outside world which is not always the case. For example, if we are running this test master in a cloud vps and want our out of network workers to have access to it.
There was a problem hiding this comment.
That's correct, my thinking is to default to something that's as secure as possible. I use ssh port forwarding in this case, but I think socat could also work if you just want to quickly expose the port to the outside world with no auth needed. Or of course editing the command line yourself.
I can document ssh port forwarding and/or socat for this and the web interface in the followon patch to https://llvm.org/docs/HowToAddABuilder.html that describes testing mode.
Perhaps I'm being over-cautious? But on the other hand:
- This mode uses a default password for connections, meaning it's easy to connect to the buildmaster and thus exploitation is much more plausible vs a production buildmaster where attackers shouldn't be able to connect
- People often run a mode like this on developer machines or within secure networks, so the potential impact is high vs a production buildmaster where (hopefully!) time and thought have gone into appropriate permissions / sandboxing / firewalling.
- For most cases where you're interactively testing a buildmaster and set of builders, I'd imagine using ssh port forwarding does the job fine.
Anyway, that's my thinking - other suggestions welcome!
| ) | ||
|
|
||
| if not test_mode: |
There was a problem hiding this comment.
If we dont get auth from github, does it mean that all worker are accessible to us or are there other changes required for allowing test buildmaster owner to be the owner of all the connected workers.
There was a problem hiding this comment.
Leaving out both auth and authz fields in the config dict seems to result in being able to access the web interface with no auth needed, and anyone accessing the interface being able to force builds and clear queues. Leaving out 'auth' but leaving in 'authz' means you can't do those commands.
Are there any other actions I might have missed I should test?
The buildbotURL also needs to be correct or else you get "invalid origin" errors when clicking those buttons in the web UI. This is another case ssh port forwarding helps if you happened to be testing on a server - as you can have it so localhost:8011 forwards to 8011 on the loopback interface on the remote machine.
…ength Upstream buildbot releases error if a step name has over 50 characters long <buildbot/buildbot#3414>. UnifiedTreeBuilder can produce quite long step names that violate this, meaning it's difficult to spin up a local test environment (as in <llvm#289>) without local hacks to workaround this. In this patch, we simply truncate step names at creation time. This means llvm-zorg's buildbot config can be used with an unmodified upstream buildbot package.
|
Firstly: I've submitted a fix for the issue of some configs having overly long step names (causing an error upon startup) to #293, and chosen to just apply this fix universally rather than trying to gate it on "testing mode" or not. Secondly: Do let me know if there is any feedback people feel is unaddressed? I'd love to move forwards with this and an associated docs update. |
Buildbot limits the maximum length to 18 characters.
Once <llvm/llvm-zorg#289> and <llvm/llvm-zorg#293> land, it's quite reasonable to ask people to test their builder configurations locally. This patch adds documentation on how to do so. I think review at this stage is useful, but of course if there's more review feedback on <llvm/llvm-zorg#289> it's possible some details may change. This won't be committed until those llvm-zorg PRs land of course.
Once <llvm/llvm-zorg#289> and <llvm/llvm-zorg#293> land, it's quite reasonable to ask people to test their builder configurations locally. This patch adds documentation on how to do so. I think review at this stage is useful, but of course if there's more review feedback on <llvm/llvm-zorg#289> it's possible some details may change. This won't be committed until those llvm-zorg PRs land of course.
|
Matching llvm documentation PR is now up here llvm/llvm-project#115024 which includes instructions on how ssh port forwarding can be used (which I think provides further clarity beyond what I suggested before in response to @omjavaid's questions - thanks for taking a look!). |
There was a problem hiding this comment.
This looks good to me. I have tested it locally and it works out of the box which is great.
Please consider merging it along with your changes to https://llvm.org/docs/HowToAddABuilder.html
Thanks Alex!
…ength Upstream buildbot releases error if a step name has over 50 characters long <buildbot/buildbot#3414>. UnifiedTreeBuilder can produce quite long step names that violate this, meaning it's difficult to spin up a local test environment (as in <llvm#289>) without local hacks to workaround this. In this patch, we simply truncate step names at creation time. This means llvm-zorg's buildbot config can be used with an unmodified upstream buildbot package.
BUILDBOT_TEST is sufficiently descriptive (after all, 'buildbot' is the package name and the name of the binary invoked to create a buildmaster) and it's possible upstream may later remove the buildmaster terminology <buildbot/buildbot#5382> or alternatively we might move away from it downstream. As such, it seems prudent to use a name that would be unaffected. BUILDMASTER_TEST was added in #289 and as the docs PR hasn't landed yet, shouldn't have any users relying on it yet.
… length (#293) Upstream buildbot releases error if a step name has over 50 characters long <buildbot/buildbot#3414>. UnifiedTreeBuilder can produce quite long step names that violate this, meaning it's difficult to spin up a local test environment (as in <#289>) without local hacks to workaround this. In this patch, we simply truncate step names at creation time. This means llvm-zorg's buildbot config can be used with an unmodified upstream buildbot package.
…#115024) With <llvm/llvm-zorg#289> and <llvm/llvm-zorg#293> landed, it's now reasonable to ask people to test their builder configurations locally. This patch adds documentation on how to do so.
…llvm#115024) With <llvm/llvm-zorg#289> and <llvm/llvm-zorg#293> landed, it's now reasonable to ask people to test their builder configurations locally. This patch adds documentation on how to do so.
This makes it (relatively) easy to test a builder setup locally. The buildmaster and the web interface should be bound only to local interfaces for security reasons (you can use ssh port forwarding if wanting to run on a server).
This patch ended up fairly small, but it needed quite a lot of fiddling about to get something that works and requires such minimal changes. I assume everyone has their own flows or downstream patches for testing with a local buildmaster, but I want to make it substantially easier for anyone to do this without repeating that effort. With this change, you just need to set the BUILDMASTER_TEST environment variable when starting the buildmaster, and it will bind to local interfaces and default to a "test" password for workers. The web interface is configured with no auth needed in this case.
I believe that any notification generation is disabled by default as
getReporters()inbuildbot/osuosl/master/config/status.pyreturns an empty array unless a config option is_production is explicitly set. It would be helpful if people more familiar with the codebase could also confirm there's no chance this will try to send unwelcome emails.Open issue:
site-packages/buildbot/process/buildstep.pyin my venv (running with an sqlite database, I don't believe the width is enforced anyway).I intend to pair this with a section on local testing of a build config in https://llvm.org/docs/HowToAddABuilder.html, but the basic gist of using this is: