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

Allow plugins to change Health endpoint status #2010

Closed
purmac opened this issue Jan 16, 2020 · 3 comments · Fixed by #2041
Closed

Allow plugins to change Health endpoint status #2010

purmac opened this issue Jan 16, 2020 · 3 comments · Fixed by #2041
Labels

Comments

@purmac
Copy link

purmac commented Jan 16, 2020

The current OPA health check endpoint only checks query evaluation and bundle activation status. There is no way for other plugins to provide health check information without recompile from source.

An enhancement can be made to OPA that allows plugins to provide health information. This is useful when a plugin needs to initialize some seed data and doesn't want an application to query it before it is ready.

I have two implementation idea

  1. provide a checker object that plugin author can register a health function to it. The health handler will iterate through the function. The API could be similar to this project https://github.com/heptiolabs/healthcheck

  2. add a new method interface to the current Plugin interface. The plugin user can implement this method to return the current plugin status.

@tsandall
Copy link
Member

@patrick-east do you think we could solve #2015 with something like this? In general, allowing plugins to opt-in to health checking (and updating the server to require all plugins be 'up') seems like a nice improvement.

@patrick-east
Copy link
Contributor

Yep, thats exactly what I was thinking 😄

@patrick-east
Copy link
Contributor

patrick-east commented Jan 22, 2020

After poking around in the code some more for this I'm leaning towards adding an API on the plugins.Manager to allow plugins to push status updates to it. Something like:

func (m *Manager) UpdateStatus(name string, status Status)

Where the Status stuff is defined like:

// State defines the state that a Plugin instance is currently
// in with pre-defined states.
type State int

const (
	// StateNotReady indicates that the Plugin is not in an error state, but isn't
	// ready for normal operation yet. This should only happen at
	// initialization time.
	StateNotReady State = iota

	// StateOK signifies that the Plugin is operating normally.
	StateOK

	// StateErr indicates that the Plugin is in an error state and should not
	// be considered as functional.
	StateErr
)

// Status has a Plugin's current status plus an optional Message
type Status struct {
	State   State  `json:"state"`
	Message string `json:"message,omitempty"`
}

Plugins have a reference to the manager at initialization time through the plugins.Factory#New API. At that point in time they can opt in to updating status immediately or later on by holding onto the manager pointer.

My main reasoning for this approach is that it avoids having to poll the plugins for health/liveness/readiness probes so that the manager always has the most up to date status for plugins.

Other clients (like the server) can then query the manager for plugin status's with another helper added to retrieve them, similar to the way bundle status gets propagated with listeners.

I'm thinking right now we maybe skip it, but I could see potentially plumbing these plugin status updates into the status API. If we do we can extend the status plugin API to support getting the updates and just have the manage forward on the info when it receives updates.

The changes to the server are going to be:

  • Fixing the bundle query param to include plugin status for discovery and bundle plugins
  • Adding a new plugins option that will include status from all plugins (in the future we could maybe add a filter for what plugins, but I'm not sure I see a great use-case for that right now).

patrick-east added a commit to patrick-east/opa that referenced this issue Jan 28, 2020
The health check now supports a `?plugin` option which will make the
response depend on whether or not all configured plugins are in an OK
state.

The `bundle` parameter will now use the bundle *and* discovery plugin
statuses to determine if the bundles are ready. This corrects an issue
where discovery bundles, and bundles defined by the discovery dynamic
config, were not included with `/health?bundle=true` checks.

The URL parameter parsing has also changed to allow for omitting the
value for the `bundle` option. It will default to `true` so that
`/health?bundle=true` can be shortened to `/health?bundle`.

Fixes: open-policy-agent#2010
Fixes: open-policy-agent#2015
Signed-off-by: Patrick East <east.patrick@gmail.com>
patrick-east added a commit to patrick-east/opa that referenced this issue Feb 7, 2020
The health check now supports a `?plugin` option which will make the
response depend on whether or not all configured plugins are in an OK
state.

The `bundle` parameter will now use the bundle *and* discovery plugin
statuses to determine if the bundles are ready. This corrects an issue
where discovery bundles, and bundles defined by the discovery dynamic
config, were not included with `/health?bundle=true` checks.

The URL parameter parsing has also changed to allow for omitting the
value for the `bundle` option. It will default to `true` so that
`/health?bundle=true` can be shortened to `/health?bundle`.

Fixes: open-policy-agent#2010
Fixes: open-policy-agent#2015
Signed-off-by: Patrick East <east.patrick@gmail.com>
tsandall pushed a commit that referenced this issue Feb 7, 2020
The health check now supports a `?plugin` option which will make the
response depend on whether or not all configured plugins are in an OK
state.

The `bundle` parameter will now use the bundle *and* discovery plugin
statuses to determine if the bundles are ready. This corrects an issue
where discovery bundles, and bundles defined by the discovery dynamic
config, were not included with `/health?bundle=true` checks.

The URL parameter parsing has also changed to allow for omitting the
value for the `bundle` option. It will default to `true` so that
`/health?bundle=true` can be shortened to `/health?bundle`.

Fixes: #2010
Fixes: #2015
Signed-off-by: Patrick East <east.patrick@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants