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.

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

server: Fix panic in /health?bundle=true #1704

Merged
merged 1 commit into from
Aug 30, 2019

Conversation

patrick-east
Copy link
Contributor

The issue was that with bundles loaded from the file system we would
not initialize the mutex used for checking bundle status.

This fixes the initialization and prevents the error. Health status
works as expected now.

Fixes: #1703
Signed-off-by: Patrick East east.patrick@gmail.com

The issue was that with bundles loaded from the file system we would
not initialize the mutex used for checking bundle status.

This fixes the initialization and prevents the error. Health status
works as expected now.

Fixes: open-policy-agent#1703
Signed-off-by: Patrick East <east.patrick@gmail.com>
Copy link
Member

@tsandall tsandall left a comment

Choose a reason for hiding this comment

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

LGTM


ctx := context.Background()

err := storage.Txn(ctx, f.server.store, storage.WriteParams, func(txn storage.Transaction) error {
Copy link
Member

Choose a reason for hiding this comment

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

What does the health check report before this transaction? Do we have a test that covers that? I'd expect it to be unhealthy if ?bundle is defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case it actually returns healthy because there is no known bundle. This test is somewhat synthetic to trigger the panic as it starts the server up without a bundle then fakes adding ones manifest in the store.

Bundles loaded via the data paths are processed when the runtime starts up and before the server ever starts serving so it isn't possible to call the health check. In practice the end result is that calls to the health check fail because the server isn't running yet, and opa would exit with an error initializing the runtime.

@patrick-east patrick-east merged commit 9c9bc29 into open-policy-agent:master Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic while handling /health?bundle=true
2 participants