feat: module allowlist #166
Conversation
|
So we are checking this https://raw.githubusercontent.com/lilypad-tech/module-allowlist/main/allowlist.txt list every 5 seconds? Maybe I misunderstood, but that seems odd to me, I assume there's something I don't understand? |
|
@richardbremner The code initializes a one-time fetch of the module allowlist when the controller starts and then sets up a periodic update via a time.Ticker that triggers once every hour |
…oryev/feat-module-allowlist
AquiGorka
requested review from
walkah,
arsen3d,
10d9e,
richardbremner,
AquiGorka,
bgins and
taoshengshi
| @@ -79,3 +79,5 @@ There are two commands that can be used to run existing tests: `./stack unit-tes | |||
| ## Notes on tooling | |||
|
|
|||
| Things should work right out-of-the-box, no extra configuration should be needed as Doppler provides the environment variables that are required with the current setup. | |||
|
|
|||
| ß | |||
There was a problem hiding this comment.
This seems off, let's remove this.
| @@ -140,7 +145,8 @@ type ResourceOffer struct { | |||
| Spec MachineSpec `json:"spec"` | |||
| // the module ID's that this resource provider can run | |||
| // an empty list means ALL modules | |||
| Modules []string `json:"modules"` | |||
| Modules []EnabledModules `json:"modules"` | |||
There was a problem hiding this comment.
Can you rename this to EnabledModules? Modules feels too generic and the intention may get lost in other places where this is used.
| @@ -141,6 +148,7 @@ func (controller *JobCreatorController) subscribeToWeb3() error { | |||
| controller.web3Events.Storage.SubscribeDealStateChange(func(ev storage.StorageDealStateChange) { | |||
| deal, err := controller.solverClient.GetDeal(ev.DealId) | |||
| if err != nil { | |||
| err = fmt.Errorf("module allowlist error: %s", err) | |||
There was a problem hiding this comment.
This looks off, if that's the error then it should bubble up from where it happened no? The way this code reads it says any error during the GetDeal process is a module allowlist err
| Offers ResourceProviderOfferOptions | ||
| Web3 web3.Web3Options | ||
| Pow ResourceProviderPowOptions | ||
| Allowlist ResourceProviderAllowlistOptions |
There was a problem hiding this comment.
Same here, let's deprecate allowList and bring forth enabledModules
| } | ||
|
|
||
| // strip out https- keep the org and module in place | ||
| // https://lilypad.tech/cowsay -> lilypad.tech/cowsay |
There was a problem hiding this comment.
It is more like https://github.com/Lilypad-Tech/lilypad-module-cowsay -> lilypad-tech/lilypad-module-cowsay (take notice how they are all lower case)
| return moduleID == allowlistModuleID | ||
| } | ||
|
|
||
| func versionMatch(version, allowlistVersion string) bool { |
There was a problem hiding this comment.
an we get unit tests for different pattern matching? Here's two references that we can rely on:
| if resourceOffer.Spec.CPU < jobOffer.Spec.CPU { | ||
| ) bool */ | ||
| func doOffersMatch(resourceOffer data.ResourceOffer, jobOffer data.JobOffer, allowlist []AllowlistItem, allowlistEnabled bool) bool { | ||
| if !allowlistEnabled { |
There was a problem hiding this comment.
Does this mean: "if the allow list feature is not enabled" then all offers match?
|
Hey @noryev it looks like these changes still include comments on TODOs, can you double check you've pushed all the changes and let us know? |
Review Type Requested (choose one):
Summary
Limit the containers that can be executed on the jobcreator. Updates consist of a module allowlist fetch and allowlist check before deal agreement.
Task/Issue reference
Closes: #63
Details
Location of Module Allowlist: https://github.com/Lilypad-Tech/module-allowlist/blob/main/allowlist.txt
Implemented into the jobcreator/controller.go due to controller managing job offers and doing the interaction with the solver on deal agreements. This includes the allowlist fetch using a simple http fetch to the lilypad-tech Github as well as the allowlist check function before a deal is agreed on.
How to test this code? (optional)
I am still having issues with my lilypad on-chain jobs in my local dev environment- please test as well!
Anything else? (optional)
I did a alias to the http package as to not step on the "github.com/lilypad-tech/lilypad/pkg/http" package