Provide structured errors for TPP connector

[davidsbond]

BUSINESS PROBLEM Sparked from https://github.com/cert-manager/cert-manager/issues/5148

When integrating with the TPP API, the codebase primarily returns API errors as messages with the response body as a JSON string. This makes it hard to handle different error scenarios. Leading to checking strings and regex, which isn't a fantastic integration experience.

For example: https://github.com/Venafi/vcert/blob/f1b4862abb7b800ac9558ad9b9c201dca6261b43/pkg/venafi/tpp/connector.go#L1232

PROPOSED SOLUTION Modify the TPP connector to expose a structured error like so:

type Error struct {
  Type string `json:"error"`
  Description string `json:"error_description"`
}

func (e Error) Error() string {
  return e.Description
}

This would allow users of vcert to use errors.As and handle different error types appropriately.

CURRENT ALTERNATIVES cert-manager currently just has to return the JSON string within the error as is. We could do something with string checks and regex but it's not very clean.

[EduardoVV]

Hi David

I'm Eduardo and i have been assigned this issue

Thanks for your proposal

I have discussed with the team and we would like to address the issue with a little bit of changes to your suggestion

PROPOSED SOLUTION Modify the TPP connector to expose a structured error like so:

type Error struct {
  Type string `json:"error"`
  Description string `json:"error_description"`
}

func (e Error) Error() string {
  return e.Description
}

^ This is a very specific error that only happens on certain requests, and not all server errors are of that format

What we propose is a more generic error:

https://github.com/Venafi/vcert/commit/91d14c8f8b119d9f1858c38fb653698126ba1be6#diff-4af69c437195303c0480347fd270ae7106bd76c963759d2275d1d014e3f099dcR1417

-       return nil, fmt.Errorf("Invalid status: %s Server response: %s", status, string(body))
+	return nil, verror.VCertConnectorError{Status: status, Body: body}

https://github.com/Venafi/vcert/commit/91d14c8f8b119d9f1858c38fb653698126ba1be6#diff-594f20814933b7ca5cf967382cbe00b1d26eef03dd079ba5db6edd5187ebfd6eR85-R89

type VCertConnectorError struct {
	VCertError
	Status string
	Body   []byte
}

You could parse the specific JSON response you mentioned like so

func Parse (body []byte) (err error) {
	var verror VCertServerError
	err = json.Unmarshal(body, &verror)
	if err != nil {
		return
	}

	switch verror.Type {
	case "invalid_token":
		return InvalidTokenError{verror}
	default:
		return verror
}

And create a custom error to hide the token

type VCertServerError struct {
	VCertConnectorError
	Type        string `json:"error"`
	Description string `json:"error_description"`
}
type InvalidTokenError struct{ VCertServerError }

func (e InvalidTokenError) Error() string {
	// Don't display token
	return fmt.Sprintf("Invalid token provided.")
}

Please let us know if you have comments

[davidsbond]

Hey @EduardoVV, thanks for looking at this. Anything that makes the errors in vcert more useful will be beneficial I think. I've shared this issue with the cert-manager team to see if they have any 2 cents as they use this package directly.

The scope might be large but it may also be good to provide known error types for the Body field? Or a set of exported functions to determine the kind of error being dealt with without the package user having to declare the types and do the unmarshaling themselves?

[jahrlin]

The idiomatic way is to return wrapped errors when you want to offer other programs using your package the option to make more informed decisions, and then let users of your package choose to use errors.Is. The error text is always the same so programmers will have the same information regardless if you wrap or not.

That way your users can choose to use the idiomatic errors.Is with your exported types, which is preferred over parsing or using a Type-string.

This way a user of your package can simply do if errors.Is(err, mypkg.ErrTemporary) instead of parsing, as described in your example.

You can export a behavioural sentinel error like ErrTemporary and return fmt.Errorf("connect to server: %w", mypkg.ErrTemporary) to let programs decide when to retry, for example.

You can also export more specific sentinel errors, like ErrInvalidAccessToken if you want to let programs make informed decisions on that.

Sentinel errors should be described in the package specification.

In most cases, something like, return fmt.Errorf("connection refused: %s", message) is enough.

In all cases, errors should be handled, not only checked - this prevent leaking implementation details or other sensitive information.

To summarize, my opinion on the path forward here is to:

• export a few sentinel errors that you want to make part of your public API
• document these sentinel errors in the package
• instruct users of your package to use the idiomatic errors.Is to make informed decisions in their programs