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

Serialize using mapstructure conversion instead of JSON marshalling. #1401

Closed
wants to merge 0 commits into from
Closed

Conversation

slewsys
Copy link

@slewsys slewsys commented Dec 30, 2023

These changes remove the JSON encoding/decoding steps that are performed during the serialization of ini and dotenv files. This roundtrip loses type information during the transformation which causes values to be incorrectly converted to the JSON marshaller defaults (int becomes float64, bool becomes string, etc, etc). In place of this JSON encoding, the mapstructure library allows for a direct conversion between the Metadata struct and map[string]interface{} needed to leverage the stores.Flatten and stores.Unflatten functions.

In addition, this adds mapstructure tags to the metadata structures to allow backwards compatibility with the JSON encoding.

Resolves #879 & resolves #857

This is a re-submission of @acastle's #1009 - with mapstructure updated to v1.5.0 and unused imports removed - applied to getsops/sops/main HEAD (previously #1338).

@devstein devstein requested a review from a team December 30, 2023 21:43
age/keysource.go Outdated Show resolved Hide resolved
stores/stores.go Outdated
// to a map suitable for use with the Flatten function.
//
// Note: this will only emit the public fields of a structure, private fields are ignored entirely.
func ConvertStructToMap(input interface{}) (map[string]interface{}, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is currently not used.

You need to:

  1. modify FlattenMetadata in flatten.go to use this function;
  2. modify UnflattenMetadata in flatten.go to use mapstructure.WeakDecode;
  3. remove the calls to DecodeNonStrings, since they should be no longer needed.

Probably EncodeNonStrings should be changed as well to traverse the structure and call ValueToString for every non-string value, so that EncodeNonStrings no longer needs to know about the exact metadata structure.

Copy link
Author

@slewsys slewsys Jan 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! Restored ConvertStructToMap and WeakDecode per original patch.

Removed DecodeNonStrings, but TestDecodeNonStrings fails when:

err := DecodeNonStrings(tt.input)

is replaced with just:

_, err := UnflattenMetadata(tt.input)

So restoring DecodeNonStrings for now...

Presently, EncodeNonStrings considers only two cases: mac_only_encrypted and shamir_threshold. Are you thinking of replacing that with something like:

	for k, v := range m {
		if _, ok := v.(string); !ok {
			m[k] = ValueToString(v)
		}
	}

?

@slewsys
Copy link
Author

slewsys commented Jan 4, 2024

If following commits look okay, I'll squash and add sign-off per devstein request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants