autopilot: feature configuration #626
Conversation
This commit adds a field to the autopilot server rpc that transports default configurations from autopilot to litd.
We pass on default configuration to litrpc in order to display them in the list features call.
We add a command line flag to litcli in order to be able to submit a feature configuration when registering a session.
We obfuscate pubkeys entered in configurations.
| // ObfuscateConfig alters the config string by replacing pubkeys with random | ||
| // values and updates the privacy pair map. | ||
| func ObfuscateConfig(privacyPairs map[string]string, configB []byte) ([]byte, |
There was a problem hiding this comment.
very cool! should we also match on channel IDs/outpoints?
There was a problem hiding this comment.
yes, will add that 👍!
There was a problem hiding this comment.
Looking good! A few notes from my side:
- currently looks like we are persisting the obfuscated config values. We should instead persist the un-obfuscated configs.
- the PR needs to be opened from upstream branch instead as discussed offline
make protosmust be run.- we should obfuscate a few more things like outpoints/channel ids
- (more a comment for the autopilot side) currently, I can set a config struct that is not valid for the feature. This means users can make mistakes (they think they are setting config for 1 feature but then it gets used for another). Instead, on the autopilot side, we should verify that the feature config sent is valid for the given feature. This can be done by: unmarshaling the sent data into the expected struct for that version, then re-marshal. If the re-marshaled struct is not equal to what the user sent, throw an error.
| // We allow empty configs, to signal the usage of the default | ||
| // configuration when the session is registered. |
There was a problem hiding this comment.
can we add this to the flag description too? also should mention the importance of the order used when specifying the config flags
There was a problem hiding this comment.
btw - I'd say this is a breaking change since users now have to set the new flag. Not a bad thing since I think most users dont register for autopilot sessions through the CLI but we should at least mention this in the release notes.
perhaps worth adding a tracking issue for things like this so that we can just post a comment there if we want to remind ourselves about anything that should be added to the readme or release notes for the next release.
There was a problem hiding this comment.
added all the suggestions, hope we have a better description now :). is it a breaking change? we can leave out all config flags and it will use the defaults
There was a problem hiding this comment.
ah - you are right. Sorry about that 🙈
| configs := ctx.StringSlice("configuration") | ||
| features := ctx.StringSlice("feature") | ||
|
|
||
| if len(configs) > 0 && len(features) != len(configs) { |
There was a problem hiding this comment.
can we just add a comment here explaining that as soon as one config flag is set, we need all of them set as else we dont know which config goes with which feature
| configB, err = firewall.ObfuscateConfig( | ||
| privacyMapPairs, configB, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
this means that featureConfig given to NewSession and thus persisted, will be the obfuscated one right? Dont think we want to do that.
There was a problem hiding this comment.
very good point 👍 ! the new version persists the unobfuscated data
|
Replaced by #629 (PR from upstream branch) |
2 participants
In this PR we add support for configuration of features within autopilot. Feature configuration can signal preferences to autopilot which are not enforceable by the
litdfirewall.ListFeaturescalllitcli