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

Status Marshaller Should Never Error #484

Closed
sam-heilbron opened this issue Jun 21, 2022 · 2 comments
Closed

Status Marshaller Should Never Error #484

sam-heilbron opened this issue Jun 21, 2022 · 2 comments
Assignees

Comments

@sam-heilbron
Copy link
Contributor

Description

Ensure the StatusUnmarshaller can never return an error

Context

The StatusUnmarshaller

UnmarshalStatus(status v1.Status, into InputResource) error

Is used when unmarshalling a Kubernetes CR into a Solo-Kit InputResource, the abstract Go type underpinning all Go types that represent CRDs in Gloo Edge.

Problem

Solo-Kit added support for tracking the status of multiple controllers on a single CR (#447). This involved changing the Status API. To make this a backwards compatible change, new controllers first attempt to read the new Status API, and then if unsuccessful, they fallback to the old Status API. This fallback should only occur once, the first time a controller reads a resource persisted in etcd after an upgrade. All statuses are written using the new API.

This works well for upgrades because the controller is backwards compatible. However, it does not work well on downgrades, because the old controller is not forwards compatible. That is, when the old controllers encounter an resource with the new API, they will attempt to unmarshal the status, and error.

Proposed Solution

Statuses are maintained by the controller itself, and should never fail unmarshalling. To more easily support upgrades,downgrades, the marshaller should never error. Instead, if an error would occur, we create an empty status, and let the controller populate the status on the next iteration.

@sam-heilbron
Copy link
Contributor Author

Spoke with @elcasteel and @inFocus7 and the proposed solution is acceptable. @inFocus7 Will be taking this work across the line in Gloo Edge.

@bewebi
Copy link
Contributor

bewebi commented Jul 13, 2022

Fixes have been released in solo-kit v0.20, v0.22, v0.24, and v0.30, all of the minor versions of solo-kit used in supported versions of Gloo
All LTS (ie >=v1.8) versions of Gloo OSS and EE have been released with updated versions of solo-kit, with the exception of EE 1.11 which has not been released since the fix was merged

Note that the fix on v0.30 includes a breaking API change, with UnmarshalStatus() no longer returning an error, while fixes on earlier versions were done without changing the API
As a result Gloo 1.12 and 1.11, which use solo-kit v0.30, updated implementations of UnmarshalStatus() and solo-apis for Gloo 1.12 and 1.11 got dependency bumps to solo-kit

The fixes to date do not account for Gloo Fed resources
This is not not a priority to fix, as there are no known users of Gloo Fed who might want to downgrade to v1.8
solo-io/solo-projects#3874 has been opened for future reference

@bewebi bewebi closed this as completed Jul 13, 2022
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

No branches or pull requests

4 participants