spark icon indicating copy to clipboard operation
spark copied to clipboard

Replace BinaryFormatter with MessagePack serialization

Open arsdragonfly opened this issue 1 year ago • 16 comments

Fixes #1131 . As explained in my comment #1131 the whole thing is very much pointless due to the nature of Spark being RCE so we must be able to pass arbitrary data/computation around (except for one place where we don't really need the flexibility of deserializing arbitrary types and I've replaced that with a strongly typed deserializer). Nevertheless, using MessagePack's typeless serialization will hopefully saves us the trouble of the ecosystem downstream.

arsdragonfly avatar Apr 05 '24 16:04 arsdragonfly

@elvaliuliuliu @AFFogarty

Can someone please help with this merge? Or assign to reviewers who have write access?

This introduces dependencies on :

  • "MessagePack"
  • "MessagePack.Annotations"
  • "Microsoft.NET.StringTools" (MSBuild utilities)

@arsdragonfly Have you already found someone to help you merge this, by any chance?

dbeavon avatar Apr 17 '24 22:04 dbeavon

@luisquintanilla @GoEddie Do you have rights to approve of PR's? Can you help with this one? It was a primary concern that needed to be addressed, according to AFFogarty

dbeavon avatar Apr 18 '24 00:04 dbeavon

@luisquintanilla @GoEddie Do you have rights to approve of PR's? Can you help with this one? It was a primary concern that needed to be addressed, according to AFFogarty

Sorry I don’t

GoEddie avatar Apr 18 '24 17:04 GoEddie

@arsdragonfly => Great job!

@luisquintanilla

You are one of the pioneering advocates. Could you help identify who has the right to trigger the merge?

I will write you en email.

GeorgeS2019 avatar Apr 19 '24 07:04 GeorgeS2019

@dbeavon

Can this merge also work for .NET 8.0? https://github.com/dotnet/spark/issues/1170

GeorgeS2019 avatar Apr 19 '24 07:04 GeorgeS2019

@arsdragonfly @dbeavon

With this merge, what version of Spark will this support?

Spark 3.3 or beyond to spark 3.5.1? What version has been tested to work with this merge? https://github.com/dotnet/spark/pull/1163

GeorgeS2019 avatar Apr 19 '24 07:04 GeorgeS2019

@dbeavon

Could you put all the necessary merges and upgrade e.g. to .NET8 to your fork?

I could direct people to your fork.

We state clearly which latest version of Spark this will support.

We work on this fork until the dotnet team responds.

Reference: https://github.com/dotnet/spark/discussions/1174

GeorgeS2019 avatar Apr 20 '24 18:04 GeorgeS2019

@GeorgeS2019

Could you put all the necessary merges and upgrade e.g. to .NET8 to your fork?

Yes, but I think we need to know what cloud platforms we want to support. I think there are some minor tweaks needed for HDInsight.

I'm guessing we should limit ourselves to HDInsight for now (and local OSS on windows).

Then we need to decide if we use a new version number or stick with 2.1.x. Then we need to decide if we publish a nuget for our Microsoft.Spark changes (to make it easier when new developers start working on a .Net driver)

The amount of work to be done is non-trivial, maybe about 40 hours to get up and running with .net 8 on HDI. Most of the work is non-technical and NOT an area where I add value (ie. management and decision making and communicating). I don't want to host this in my own fork forever. Have you tried to contact Microsoft? Do you have a .net foundation membership?

Frankly, I'd prefer if you owned a fork, and I'll send you PR's to merge. Your help would be invaluable, whether technical or non-technical. I can send you PR's right away for .net 8 and messagepack and HDI compatibility.

dbeavon avatar Apr 20 '24 20:04 dbeavon

Start small..we recruit more to join. Local Windows 11 OS is key.

Ideally we show the spark dotnet book examples work in e.g. Net

So new and existing users can read the book and yet doing exercises code based on Net 8.0. And a convenient microsft spark 2.1.* version, as U have suggested

GeorgeS2019 avatar Apr 20 '24 20:04 GeorgeS2019

Updated

GeorgeS2019 avatar Apr 20 '24 20:04 GeorgeS2019

@arsdragonfly @dbeavon

With this merge, what version of Spark will this support?

The versions of Spark are probably the same as they were in Microsoft.Spark 2.1.1.

I am only testing Spark 3.1.2. (Version supported for HDI 5) That has been sitting around for a long time.

I am pretty sure we could get to other versions of spark if needed. It seems like the least of our concerns at this point.

What is the concern insofar as Spark versioning? Please remember that .net and spark are pretty independent of each other. You can support a wide range of versions without too much difficulty.

dbeavon avatar Apr 20 '24 20:04 dbeavon

Just start very small..others will join with renewed interest

GeorgeS2019 avatar Apr 20 '24 20:04 GeorgeS2019

still discussing internally on how to structure project governance, stay tuned

arsdragonfly avatar Apr 30 '24 02:04 arsdragonfly

Note to @AFFogarty and other MSFT reviewers: MessagePack's "typeless" functionality is banned for MSFT internal use under the same regulation that bans BinaryFormatter for MSFT internal use. If you accept this PR, you will still have to jump through the same "ask upper management for an exception to use this" hoops that you would've had to go through with BF.

As OP notes, you could make an argument that since no logical trust boundary is being crossed, it's all a moot point anyway and the exception should be granted. I imagine your security team would sign off on that statement. But the main point I'm trying to make is that this PR does not simplify any internal signoff + release process. This PR should be viewed wholly through the lens of broader ecosystem impact, nothing more. I am only posting this comment because I am the compliance owner for serialization related technologies and I wanted to clarify how this PR would / would not impact any signoff.

GrabYourPitchforks avatar May 14 '24 20:05 GrabYourPitchforks

Note to @AFFogarty and other MSFT reviewers: MessagePack's "typeless" functionality is banned for MSFT internal use under the same regulation that bans BinaryFormatter for MSFT internal use. If you accept this PR, you will still have to jump through the same "ask upper management for an exception to use this" hoops that you would've had to go through with BF.

As OP notes, you could make an argument that since no logical trust boundary is being crossed, it's all a moot point anyway and the exception should be granted. I imagine your security team would sign off on that statement. But the main point I'm trying to make is that this PR does not simplify any internal signoff + release process. This PR should be viewed wholly through the lens of broader ecosystem impact, nothing more. I am only posting this comment because I am the compliance owner for serialization related technologies and I wanted to clarify how this PR would / would not impact any signoff.

Thanks for the reminder. This is the unfortunate workaround that we'd probably have to settle around since BF is on its way out and the strictly statically typed alternative that Orleans folks chose to use since Orleans 7 will IMO make .NET for Spark completely useless for anything but the most trivial toy programs. @luisquintanilla could you help drive the security review/signoff process for this particular PR and for our future community-driven development model in general?

arsdragonfly avatar May 29 '24 10:05 arsdragonfly

do not merge this PR yet (see above mentioned problems)

arsdragonfly avatar Sep 11 '24 00:09 arsdragonfly