Fix proto resource 153 marshalling for autoupdate_* resources #50688
Conversation
hugoShaka
added
the
no-changelog
public-teleport-github-review-bot
bot
removed request for
espadolini,
tcsc,
vapopov and
rosstimothy
api/types/resource_153.go
Outdated
| // | ||
| // Note that CheckAndSetDefaults is a noop for the returned resource and | ||
| // SetSubKind is not implemented and panics on use. | ||
| func Resource153ToLegacyV2(r Resource153) Resource { |
There was a problem hiding this comment.
Are we supposed to favor Resource153ToLegacyV2 over Resource153ToLegacy, or should it be decided case-by-case?
Should we update Resource153ToLegacy to mention Resource153ToLegacyV2?
Could we choose a method name clearer than V2?
Alternatively, should we use the options pattern along with Resource153ToLegacy and keep it as a single method?
There was a problem hiding this comment.
Should we update Resource153ToLegacy to mention Resource153ToLegacyV2?
I don't know in which context other resource153ToLegacy.MarshalJSON is used. This might break schemas and compatibility.
At least for resource manifests, yes we should always use protojson, else all the timestamps/durations and other google magical proto types are wrong.
Could we choose a method name clearer than V2?
I'm open to any suggestion. Maybe Resource153ToLegacyWithOptions if we chose the options pattern?
Alternatively, should we use the options pattern along with Resource153ToLegacy and keep it as a single method?
Sounds good to me, but this is a breaking change, this code is in api, and we're going to backport this at least up to v15. So this would break the semver-like versioning we have.
There was a problem hiding this comment.
IMO adding variadic options is fine even for older api/ versions, but I'd also be OK with a non-breaking variant for release branches if you think it's worth it.
I do think that keeping it to a single method is worth doing for master, as we'll carry this forward for a long time.
There was a problem hiding this comment.
IMO we should either have a separate method for "adapt this types.Resource153 that's also a proto.Message that should get marshaled with protojson into a types.Resource" or - the worst option of all but maybe the pragmatic option - we add an explicit list of concrete types that should be protojsoned in there. 😢
There was a problem hiding this comment.
(outside of other pretty fringe options like setting a protobuf extension on the message and checking that at runtime or using that extension for a custom codegen that defines MarshalJSON/UnmarshalJSON as needed)
There was a problem hiding this comment.
I would rather avoid custom codegen, the more we can rely on vanilla protoc generators the less ongoing work needs to be done (and the less weird the project gets for developers).
A custom extension or a list of types could be a solution, although I guess it depends on how much code repetition it actually saves. The list is handy in some ways, but also somewhat obscure in others (particularly if it changes the behavior of what looks to be a "simple" function).
There was a problem hiding this comment.
I changed the controversial V2 into types.ProtoResource153ToLegacy which explains more clearly the difference with the original one and doesn't have to check if the underlying resource implements proto.Message every time.
public-teleport-github-review-bot
bot
removed the request for review
from espadolini
There was a problem hiding this comment.
@hugoShaka you requested a review but there are still plenty of open/unaddressed comments. I don't see significant changes in the PR either (apart from a couple of accepted suggestions). Maybe you forgot to push some commits?
Co-authored-by: Alan Parra <[email protected]>
Co-authored-by: Alan Parra <[email protected]>
- Change from Resource153AdapterV2 to ProtoResource153Adapter - fix test failures and unmarshal proto resources properly - add a failing round-trip proto 153 test case - bonus: fix the table tesst reosurce create that did not support running a single row
hugoShaka
force-pushed
the
hugo/proto-resource153-marshalling
branch
from
80136b4 to
db3eb75
Compare
| // tctlGetAllValidations allows tests to register post-test validations to validate | ||
| // that their resource is present in "ttcl get all" output. | ||
| // This allows running test rows instead of the whole test table. | ||
| var tctlGetAllValidations []func(t *testing.T, out string) | ||
|
|
There was a problem hiding this comment.
Sorry for the noisy changes in the table test but I got fed up by the test failing if I only ran a single row. Now the individual rows can be ran.
Co-authored-by: Alan Parra <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This PR adds
types.Resource153ToLegacyV2()which acts exactly liketypes.Resource153ToLegacy()but detects if the underlying resource153 implementation is a proto struct and usesprotojsoninstead of regular json marshaling.This fixes improper marshaling for resources
autoupdate_config,autoupdate_version, andautoupdate_agent_rollout.This is a band-aid solution, the proper way to address this would be to have the proto structs implement
json.Marshaler.This PR does not change
types.Resource153ToLegacy()because:Goal (internal): https://github.com/gravitational/cloud/issues/10289