Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plugins: Additive updates to services when discovery enabled #2311

Merged
merged 1 commit into from
Apr 17, 2020
Merged

plugins: Additive updates to services when discovery enabled #2311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions docs/content/management.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -944,6 +944,5 @@ configuration labels or environment variables.

### Limitations

The discovery feature cannot be used to dynamically modify `services`, `labels`
and `discovery`. This means that these configuration settings should be included
in the bootup configuration file provided to OPA.
The discovery feature cannot be used to dynamically modify `labels` and `discovery`. This means that these configuration settings should be included
in the bootup configuration file provided to OPA. However, it can be used to modify `services` with the exception of the service used to download the discovery bundle.
34 changes: 20 additions & 14 deletions plugins/discovery/discovery.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,20 +174,11 @@ func (c *Discovery) processUpdate(ctx context.Context, u download.Update) {

func (c *Discovery) reconfigure(ctx context.Context, u download.Update) error {

config, ps, err := processBundle(ctx, c.manager, c.factories, u.Bundle, c.config.query, c.metrics)
ps, err := processBundle(ctx, c.manager, c.factories, u.Bundle, c.config.query, c.config.service, c.metrics)
if err != nil {
return err
}

if err := c.manager.Reconfigure(config); err != nil {
return err
}

// TODO(tsandall): we don't currently support changes to discovery
// configuration. These changes are risky because errors would be
// unrecoverable (without keeping track of changes and rolling back...)

// TODO(tsandall): add protection against discovery -service- changing.
for _, p := range ps.Start {
if err := p.Start(ctx); err != nil {
return err
Expand Down Expand Up @@ -220,15 +211,30 @@ func (c *Discovery) logrusFields() logrus.Fields {
}
}

func processBundle(ctx context.Context, manager *plugins.Manager, factories map[string]plugins.Factory, b *bundleApi.Bundle, query string, m metrics.Metrics) (*config.Config, *pluginSet, error) {
func processBundle(ctx context.Context, manager *plugins.Manager, factories map[string]plugins.Factory, b *bundleApi.Bundle, query, service string, m metrics.Metrics) (*pluginSet, error) {

config, err := evaluateBundle(ctx, manager.ID, manager.Info, b, query)
if err != nil {
return nil, nil, err
return nil, err
}

// check for updates to the discovery configuration
if config.Discovery != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return nil, fmt.Errorf("updates to the discovery configuration are not allowed")
}

client := manager.Client(service)

if err := manager.Reconfigure(config); err != nil {
return nil, err
}

// 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")
}
Comment on lines +232 to 235
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


ps, err := getPluginSet(factories, manager, config, m)
return config, ps, err
return getPluginSet(factories, manager, config, m)
}

func evaluateBundle(ctx context.Context, id string, info *ast.Term, b *bundleApi.Bundle, query string) (*config.Config, error) {
Expand Down
189 changes: 186 additions & 3 deletions plugins/discovery/discovery_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ func TestProcessBundle(t *testing.T) {
}
`)

_, ps, err := processBundle(ctx, manager, nil, initialBundle, "data.config", nil)
ps, err := processBundle(ctx, manager, nil, initialBundle, "data.config", "default", nil)
if err != nil {
t.Fatal(err)
}
Expand All @@ -139,7 +139,7 @@ func TestProcessBundle(t *testing.T) {
}
`)

_, ps, err = processBundle(ctx, manager, nil, updatedBundle, "data.config", nil)
ps, err = processBundle(ctx, manager, nil, updatedBundle, "data.config", "default", nil)
if err != nil {
t.Fatal(err)
}
Expand All @@ -156,7 +156,7 @@ func TestProcessBundle(t *testing.T) {
}
`)

_, _, err = processBundle(ctx, manager, nil, updatedBundle, "data.config", nil)
_, err = processBundle(ctx, manager, nil, updatedBundle, "data.config", "default", nil)
if err == nil {
t.Fatal("Expected error but got success")
}
Expand Down Expand Up @@ -274,6 +274,189 @@ func TestReconfigure(t *testing.T) {

}

func TestReconfigureWithUpdates(t *testing.T) {

ctx := context.Background()

manager, err := plugins.New([]byte(`{
"labels": {"x": "y"},
"services": {
"localhost": {
"url": "http://localhost:9999"
}
},
"discovery": {"name": "config"},
}`), "test-id", inmem.New())
if err != nil {
t.Fatal(err)
}

disco, err := New(manager)
if err != nil {
t.Fatal(err)
}

initialBundle := makeDataBundle(1, `
{
"config": {
"bundle": {"name": "test1"},
"status": {},
"decision_logs": {}
}
}
`)

err = disco.reconfigure(ctx, download.Update{Bundle: initialBundle})
if err != nil {
t.Fatalf("Unexpected error %v", err)
}

// update the discovery configuration
updatedBundle := makeDataBundle(2, `
{
"config": {
"discovery": {
"name": "config",
"decision": "/foo/bar"
}
}
}
`)

err = disco.reconfigure(ctx, download.Update{Bundle: updatedBundle})
if err == nil {
t.Fatal("Expected error but got nil")
}

expectedErrMsg := "updates to the discovery configuration are not allowed"
if err.Error() != expectedErrMsg {
t.Fatalf("Expected error message: %v but got: %v", expectedErrMsg, err.Error())
}

// update the discovery service
updatedBundle = makeDataBundle(3, `
{
"config": {
"services": {
"localhost": {
"url": "http://localhost:9999",
"credentials": {"bearer": {"token": "blah"}}
}
}
}
}
`)

err = disco.reconfigure(ctx, download.Update{Bundle: updatedBundle})
if err == nil {
t.Fatal("Expected error but got nil")
}
Comment on lines +351 to +353
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Do we need any validation for what error it ran into?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the tests to check the error message.


expectedErrMsg = "updates to the discovery service are not allowed"
if err.Error() != expectedErrMsg {
t.Fatalf("Expected error message: %v but got: %v", expectedErrMsg, err.Error())
}

// add a new service and a new bundle
updatedBundle = makeDataBundle(4, `
{
"config": {
"services": {
"acmecorp": {
"url": "http://localhost:8181"
}
},
"bundles": {
"authz": {
"service": "acmecorp"
}
}
}

}
`)

err = disco.reconfigure(ctx, download.Update{Bundle: updatedBundle})
if err != nil {
t.Fatalf("Unexpected error %v", err)
}

if len(disco.manager.Services()) != 2 {
t.Fatalf("Expected two services but got %v\n", len(disco.manager.Services()))
}

bPlugin := bundle.Lookup(disco.manager)
config := bPlugin.Config()
expected := "acmecorp"
if config.Bundles["authz"].Service != expected {
t.Fatalf("Expected service %v for bundle authz but got %v", expected, config.Bundles["authz"].Service)
}

// update existing bundle's config and add a new bundle
updatedBundle = makeDataBundle(5, `
{
"config": {
"bundles": {
"authz": {
"service": "localhost",
"resource": "foo/bar"
},
"main": {
"resource": "baz/bar"
}
}
}
}
`)

err = disco.reconfigure(ctx, download.Update{Bundle: updatedBundle})
if err != nil {
t.Fatalf("Unexpected error %v", err)
}

bPlugin = bundle.Lookup(disco.manager)
config = bPlugin.Config()
expectedSvc := "localhost"
if config.Bundles["authz"].Service != expectedSvc {
t.Fatalf("Expected service %v for bundle authz but got %v", expectedSvc, config.Bundles["authz"].Service)
}

expectedRes := "foo/bar"
if config.Bundles["authz"].Resource != expectedRes {
t.Fatalf("Expected resource %v for bundle authz but got %v", expectedRes, config.Bundles["authz"].Resource)
}

expectedSvcs := map[string]bool{"localhost": true, "acmecorp": true}
if _, ok := expectedSvcs[config.Bundles["authz"].Service]; !ok {
t.Fatalf("Expected service for bundle main to be one of [%v, %v] but got %v", "localhost", "acmecorp", config.Bundles["authz"].Service)
}

// update existing (non-discovery)service's config
updatedBundle = makeDataBundle(6, `
{
"config": {
"services": {
"acmecorp": {
"url": "http://localhost:8181",
"credentials": {"bearer": {"token": "blah"}}
}
},
"bundles": {
"authz": {
"service": "localhost",
"resource": "foo/bar"
}
}
}
}
`)

err = disco.reconfigure(ctx, download.Update{Bundle: updatedBundle})
if err != nil {
t.Fatalf("Unexpected error %v", err)
}
}

type testServer struct {
t *testing.T
mtx sync.Mutex
Expand Down
5 changes: 5 additions & 0 deletions plugins/rest/rest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Equal API can be removed and added on the rest client config struct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return reflect.DeepEqual(c.config, other.config)
}

// Do executes a request using the client.
func (c Client) Do(ctx context.Context, method, path string) (*http.Response, error) {

Expand Down