plugins: Additive updates to services when discovery enabled #2311
Conversation
plugins/discovery/discovery.go
Outdated
| } | ||
|
|
||
| // check for updates to the discovery service | ||
| if !reflect.DeepEqual(client, manager.Client(service)) { |
There was a problem hiding this comment.
We might want to just have a like Compare() API or something on the clients. We probably just want to compare their config right? We wouldn't really need to compare the other fields on them afaik.
There was a problem hiding this comment.
Yes we compare their config only. Added an Equal API on the client.
| if err == nil { | ||
| t.Fatal("Expected error but got nil") | ||
| } |
There was a problem hiding this comment.
nit: Do we need any validation for what error it ran into?
There was a problem hiding this comment.
Updated the tests to check the error message.
Earlier with discovery enabled updates to the 'services' configuration was not allowed to protect against accidental changes to the discovery service itself. Since adding new services could be useful, this change allows modifications to the 'services' configuration. The only exception is that the service used to download the discovery bundle cannot be modified. Fixes open-policy-agent#2058 Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
ashutosh-narkar
force-pushed
the
additive-discovery
branch
from
226d981 to
66a4a00
Compare
| @@ -148,6 +148,11 @@ func (c Client) WithBytes(body []byte) Client { | |||
| return c | |||
| } | |||
|
|
|||
| // Equal returns true if this client is equal to the other. | |||
| func (c Client) Equal(other Client) bool { | |||
There was a problem hiding this comment.
This seems like an odd API to have on an HTTP client--and the comment is misleading since it only checks if the configs are equal. I'd remove this and just expose a way to access the config to perform equality checks when needed.
There was a problem hiding this comment.
The Equal API can be removed and added on the rest client config struct.
There was a problem hiding this comment.
That should be fine. I guess technically we only care if the discovered configuration is not a subset of the bootstrap configuration because that's where it gets problematic. Strict equality should be fine for now.
| } | ||
|
|
||
| // check for updates to the discovery configuration | ||
| if config.Discovery != nil { |
There was a problem hiding this comment.
Isn't this going to break anyone who is sending OPA a discovery bundle that contains the discovery (and dependent service) configuration unchanged? I think we need to perform a semantic diff on the discovery and dependent service configuration instead of just checking for existence AFAICT.
There was a problem hiding this comment.
What is the scenario in which a user sends an unchanged discovery configuration in the service bundle ? Given that we already say in the docs that the discovery config cannot be updated, is this something we could be more explicit about in the docs OR Are you proposing we check the discovery config the service bundle and compare with the boot config ?
For the dependent service, we are checking the config of the rest client.
There was a problem hiding this comment.
Are you proposing we check the discovery config the service bundle and compare with the boot config ?
Yes, exactly. The current change is backwards incompatible so we need to fix that anyway. From a design perspective, I'd rather not require users special case discovered configuration generation to remove overlap with bootstrap configuration for no reason. As a design principle, it's important to keep APIs idempotent.
| // check for updates to the discovery service | ||
| if !client.Equal(manager.Client(service)) { | ||
| return nil, fmt.Errorf("updates to the discovery service are not allowed") | ||
| } |
There was a problem hiding this comment.
Shouldn't the check on the service config occur before we reconfigure the manager? As is, this means we'll end up with updates being applied inconsistently.
There was a problem hiding this comment.
Yes I was thinking about that which meant we need to either make parseServicesConfig (plugins/plugins.go) public or copy over the logic or move the function to a common package.
Any preference here ?
There was a problem hiding this comment.
A good design principle is to optimize for change--i.e., assume we're going to have to modify the code at some point. Given that, avoid copying logic because it means more maintenance and more bugs. If you're worried about increasing the API surface area outside of OPA consider exporting the function inside of an internal package.
Earlier with discovery enabled updates to the 'services' configuration
was not allowed to protect against accidental changes to the discovery service
itself. Since adding new services could be useful, this change allows modifications
to the 'services' configuration. The only exception is that the service used to download
the discovery bundle cannot be modified.
Fixes #2058
Signed-off-by: Ashutosh Narkar anarkar4387@gmail.com