OPA: Fail fast on discovery or bundle download errors

[mjungsbluth]

As part of #3119, this PR changes the behaviour on starting up OPA instances inside the OPA based Skipper filters.

Currently even in the face of download errors of bundles, the default timeout of 30s is applied.

By using listeners both for bundle download and discovery bundle download, we can detect a failure early despite the respective plugins still waiting for the bundles to become available.

Still a draft because tests are missing, there are some cleanups in the code that need to be done.

Pending: Unregister discovery and bundle plugin listeners.

[Pushpalanka]

Addressing the race condition in the upstream as a part of this reported issue.

[mjungsbluth]

This still feels like we are only handling a subset of errors, is this really reliable? My understanding was that the Code only gives information about what went wrong and that length(status.Errors) essentially gives yoj if anything goes wrong…

[Pushpalanka]

That indeed is more intuitive, but had below concerns.

When looking at the OPA implementation below, status.Errors it is set only in 1 occasion out of the 3. But the other 2 occasions set status.Errors to nil even though it is setting an error. Based on the existing implementation status.Code is most reliable as I see.

I also understand if the bundle_error constant is ever modified then we are in trouble. But then again, just relying on code to be non-empty is also not good if the code got ever set in an non-errornous scenario.

Status Error code segment, /plugins/bundle/status.go#L64-L97

const (
	errCode = "bundle_error"
)

func (s *Status) SetError(err error) {
	var (
		astErrors ast.Errors
		httpError download.HTTPError
	)
	switch {
	case err == nil:
		s.Code = ""
		s.HTTPCode = ""
		s.Message = ""
		s.Errors = nil

	case errors.As(err, &astErrors):
		s.Code = errCode
		s.HTTPCode = ""
		s.Message = types.MsgCompileModuleError
		s.Errors = make([]error, len(astErrors))
		for i := range astErrors {
			s.Errors[i] = astErrors[i]
		}

	case errors.As(err, &httpError):
		s.Code = errCode
		s.HTTPCode = json.Number(strconv.Itoa(httpError.StatusCode))
		s.Message = err.Error()
		s.Errors = nil

	default:
		s.Code = errCode
		s.HTTPCode = ""
		s.Message = err.Error()
		s.Errors = nil
	}
}

[szuecs]

Sounds like we want to reach out to styra to make a proper error handling possible.
I also wonder about the typing here. Status.HTTPCode being a string and httpError not being an error, what the hack is this code?

[Pushpalanka]

Will raise this in the OPA community and get back.

PS: open-policy-agent/opa#6983 raised with minimum changes that will help us here. Will take it forward based on review.

[Pushpalanka]

Went ahead with the bundle_error constant as a change there will be communicated in OPA release notes.

[Pushpalanka]

Moving to draft as using OPA 0.67.0, which is yet to be taken by Skipper dependency upgrades.

[MustafaSaber]

Moving to draft as using OPA 0.67.0, which is yet to be taken by Skipper dependency upgrades.

OPA 0.67.1 is merged now.

[Pushpalanka]

Requesting a review as the PR goes green without the 2 ToDo's to be addressed to unregister the listeners.
Unregistering is removed as it prematurely unregistering the listeners without handling the errors. Appreciate inputs.