feat(tdf): sdk interface changes #123

sujankota · 2024-02-04T20:37:43Z

Reader interface for reading the TDF payload

dmihalcik-virtru

Some minor nits, but I think we should get this in so we can worry about performance improvements from a shared baseline

dmihalcik-virtru · 2024-02-05T19:32:24Z

sdk/tdf.go

-	// create tdf reader
-	tdfReader, err := archive.NewTDFReader(reader)
+// Manifest return the manifest as json string.
+func (reader *Reader) Manifest() (string, error) {


Why do we want to return it as a string?

I'm not sure if we want to export the Manifest object. If we want, we can return the object instead of JSON.

Yeah only export the absolute minimum we can get away with for 1.0. Any removals after 1.0 will be a major SemVer change and to be avoided if at all possible

dmihalcik-virtru · 2024-02-05T19:37:02Z

sdk/tdf.go

+	var payloadReadOffset int64
+	for index, seg := range reader.manifest.EncryptionInformation.IntegrityInformation.Segments {
+		if firstSegment > int64(index) {
+			payloadReadOffset += seg.EncryptedSize


This is quadratic, so maybe you should cache the offsets and do a binary search. Without a benchmark and some profiles I'm reluctant to ask you to spend any substantial time on it though

At first, I created a segment offset cache and reverted the changes; I can put that back and do a binary search on the index.

Let's get this in first. I suspect most of the performance penalty is due to the lack of a decrypt cache

dmihalcik-virtru · 2024-02-05T19:39:36Z

sdk/tdf.go

+	var start = math.Floor(float64(offset) / float64(defaultSegmentSize))
+	var end = math.Ceil(float64(offset+int64(len(buf))) / float64(defaultSegmentSize))
+
+	// slog.Debug("Invoked ReadAt", slog.Int("bufsize", len(buf)), slog.Int("offset", int(offset)))
+
+	firstSegment := int64(start)
+	lastSegment := int64(end)


Suggested change

var start = math.Floor(float64(offset) / float64(defaultSegmentSize))

var end = math.Ceil(float64(offset+int64(len(buf))) / float64(defaultSegmentSize))

// slog.Debug("Invoked ReadAt", slog.Int("bufsize", len(buf)), slog.Int("offset", int(offset)))

firstSegment := int64(start)

lastSegment := int64(end)

firstSegment, lastSegment := offset / defaultSegmentSize, (offset+int64(len(buf))) / defaultSegmentSize

I think we need floor and ciel on the segments.

oic

Suggested change

var start = math.Floor(float64(offset) / float64(defaultSegmentSize))

var end = math.Ceil(float64(offset+int64(len(buf))) / float64(defaultSegmentSize))

// slog.Debug("Invoked ReadAt", slog.Int("bufsize", len(buf)), slog.Int("offset", int(offset)))

firstSegment := int64(start)

lastSegment := int64(end)

firstSegment, lastSegment := offset / defaultSegmentSize, (offset+int64(len(buf))+defaultSegmentSize-1) / defaultSegmentSize

dmihalcik-virtru · 2024-02-05T19:41:37Z

sdk/tdf.go

+			continue
+		}
+
+		readBuf, err := reader.tdfReader.ReadPayload(payloadReadOffset, seg.EncryptedSize)


My suspicion is that for all but the tiniest segment size, where the quadratic above may dominate, a cache of the decrypted (or even non-decrypted) segment here might be the best use, in the event that the stride in Copy doesn't line up with the segments.

So for the 'best case' benchmark, compare a GetPayload with a CopyBuffer where the buffer is defaultSegmentSize bytes long. It looks like the default copy buffer size is 32 KiB

Yes, the default copy buffer size is 32 KiB that is the problem I'm seeing. If you want to decrypt a 20 GB file. Consider how many reads you must do versus GePayload do 2mb(segment size) writes.

This means your code will be calling Read 64 times for each segment. This will add up. To avoid this, in your benchmark do something like:

buf := make([]byte, r.DefaultSegmentSize()) n, err := io.CopyBuffer(w, r, buf)

Unfortunately this means you'll have to export DefaultSegmentSize(). Maybe call it PreferredBufferReadSize or something

dmihalcik-virtru · 2024-02-05T19:42:05Z

sdk/tdf.go


-	return totalBytes, nil
+	var err error = nil
+	bufLen := int64(len(buf))


Suggested change

bufLen := int64(len(buf))

dmihalcik-virtru · 2024-02-05T19:42:49Z

sdk/tdf.go

+		err = io.EOF
+	}
+
+	startIndex := offset - (firstSegment * defaultSegmentSize)


defaultSegmentSize kinda implies that each segment might have a different size, right?

AFAICT we always have all but the last segment be the defaultSegmentSize, right? Is there any reason to believe this might not be the case? If nothing else, it is a compatibility concern

dmihalcik-virtru · 2024-02-05T19:49:35Z

sdk/tdf.go

+	for index, seg := range reader.manifest.EncryptionInformation.IntegrityInformation.Segments {
+		if firstSegment > int64(index) {


Save some typing

Suggested change

for index, seg := range reader.manifest.EncryptionInformation.IntegrityInformation.Segments {

if firstSegment > int64(index) {

for i32, seg := range reader.manifest.EncryptionInformation.IntegrityInformation.Segments {

index := int64(i32)

if firstSegment > index {

dmihalcik-virtru · 2024-02-05T19:51:36Z

internal/archive/tdf3_reader.go

 }

 // ReadPayload Return the payload of given length from index.
 func (tdfReader TDFReader) ReadPayload(index, length int64) ([]byte, error) {


I would suggest having this be ReadAt as well for consistency

dmihalcik-virtru · 2024-02-05T20:00:09Z

sdk/tdf.go

 		}

-		n, err := writer.Write(writeBuf)
+		n, err := decryptedBuf.Write(writeBuf)


You don't need decryptedBuf here; you could write directly to buf with a copy. However, I'm fine with this as is as it will be easier to update if we add a decrypt cache. But I'd still only keep a decrypt cache

dmihalcik-virtru · 2024-02-06T15:45:53Z

I'm fine with this as is. Most of what I mention is a performance optimization. The only thing I am worried about as a latent bug is assuming defaultSegmentSize is the size of all segments except the last segment, instead of checking the actual size during the scan, but we can file that as a follow up too as a compatibility error

sdk/tdf.go

dmihalcik-virtru · 2024-02-06T19:31:46Z

Any performance regressions or compatibility concerns with ReadAt (and Read) can be addressed later

🤖 I have created a release *beep* *boop* --- ## [0.1.0](sdk-v0.1.0...sdk/v0.1.0) (2024-04-22) ### Features * add structured schema policy config ([#51](#51)) ([8a6b876](8a6b876)) * **auth:** add authorization via casbin ([#417](#417)) ([292f2bd](292f2bd)) * in-process service to service communication ([#311](#311)) ([ec5eb76](ec5eb76)) * **kas:** support HSM and standard crypto ([#497](#497)) ([f0cbe03](f0cbe03)) * key access server assignments ([#111](#111)) ([a48d686](a48d686)), closes [#117](#117) * key access server registry impl ([#66](#66)) ([cf6b3c6](cf6b3c6)) * **namespaces CRUD:** protos, generated SDK, db interactivity for namespaces table ([#54](#54)) ([b3f32b1](b3f32b1)) * **PLAT-3112:** Initial consumption of ec_key_pair functions by nanotdf ([#586](#586)) ([5e2cba0](5e2cba0)) * **policy:** add FQN pivot table ([#208](#208)) ([abb734c](abb734c)) * **policy:** add soft-delete/deactivation to namespaces, attribute definitions, attribute values [#96](#96) [#108](#108) ([#191](#191)) ([02e92a6](02e92a6)) * **resourcemapping:** resource mapping implementation ([#83](#83)) ([c144db1](c144db1)) * **sdk:** BACK-1966 get auth wired up to SDK using `Options` ([#271](#271)) ([f1bacab](f1bacab)) * **sdk:** BACK-1966 implement fetching a DPoP token ([#45](#45)) ([dbd3cf9](dbd3cf9)) * **sdk:** BACK-1966 make the unwrapper retrieve public keys as well ([#260](#260)) ([7d051a1](7d051a1)) * **sdk:** BACK-1966 pull rewrap into auth config ([#252](#252)) ([84017aa](84017aa)) * **sdk:** Include auth token in grpc ([#367](#367)) ([75cb5cd](75cb5cd)) * **sdk:** normalize token exchange ([#546](#546)) ([9059dff](9059dff)) * **sdk:** Pass dpop key through to `rewrap` ([#435](#435)) ([2d283de](2d283de)) * **sdk:** read `expires_in` from token response and use it to refresh access tokens ([#445](#445)) ([8ecbe79](8ecbe79)) * **sdk:** sdk stub ([#10](#10)) ([8dfca6a](8dfca6a)) * **sdk:** take a function so that callers can use this the way that they want ([#340](#340)) ([72059cb](72059cb)) * **subject-mappings:** refactor to meet db schema ([#59](#59)) ([59a073b](59a073b)) * **tdf:** implement tdf3 encrypt and decrypt ([#73](#73)) ([9d0e0a0](9d0e0a0)) * **tdf:** sdk interface changes ([#123](#123)) ([2aa2422](2aa2422)) * **tdf:** sdk interface cleanup ([#201](#201)) ([6f7d815](6f7d815)) * **tdf:** TDFOption varargs interface ([#235](#235)) ([b3fb720](b3fb720)) ### Bug Fixes * **archive:** remove 10gb zip file test ([#373](#373)) ([6548f55](6548f55)) * attribute missing rpc method for listing attribute values ([#69](#69)) ([1b3a831](1b3a831)) * **attribute value:** fixes attribute value crud ([#86](#86)) ([568df9c](568df9c)) * **issue 90:** remove duplicate attribute_id from attribute value create/update, and consumes schema setup changes in namespaces that were introduced for integration testing ([#100](#100)) ([e0f6d07](e0f6d07)) * **issue-124:** SDK kas registry import name mismatch ([#125](#125)) ([112638b](112638b)), closes [#124](#124) * **proto/acre:** fix resource encoding service typo ([#30](#30)) ([fe709d2](fe709d2)) * remove padding when b64 encoding ([#437](#437)) ([d40e94a](d40e94a)) * SDK Quickstart ([#628](#628)) ([f27ab98](f27ab98)) * **sdk:** change unwrapper creation ([#346](#346)) ([9206435](9206435)) * **sdk:** double bearer token in auth config ([#350](#350)) ([1bf4699](1bf4699)) * **sdk:** fixes Manifests JSONs with OIDC ([#140](#140)) ([a4b6937](a4b6937)) * **sdk:** handle err ([#548](#548)) ([ebabb6c](ebabb6c)) * **sdk:** make KasInfo fields public ([#320](#320)) ([9a70498](9a70498)) * **sdk:** shutdown conn ([#352](#352)) ([3def038](3def038)) * **sdk:** temporarily move unwrapper creation into options func. ([#309](#309)) ([b34c2fe](b34c2fe)) * **sdk:** use the dialoptions even with no client credentials ([#400](#400)) ([a7f1908](a7f1908)) * **security:** add a new encryption keypair different from dpop keypair ([#461](#461)) ([7deb51e](7deb51e)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>

🤖 I have created a release *beep* *boop* --- ## [0.1.0](opentdf/platform@sdk-v0.1.0...sdk/v0.1.0) (2024-04-22) ### Features * add structured schema policy config ([#51](opentdf/platform#51)) ([8a6b876](opentdf/platform@8a6b876)) * **auth:** add authorization via casbin ([#417](opentdf/platform#417)) ([292f2bd](opentdf/platform@292f2bd)) * in-process service to service communication ([#311](opentdf/platform#311)) ([ec5eb76](opentdf/platform@ec5eb76)) * **kas:** support HSM and standard crypto ([#497](opentdf/platform#497)) ([f0cbe03](opentdf/platform@f0cbe03)) * key access server assignments ([#111](opentdf/platform#111)) ([a48d686](opentdf/platform@a48d686)), closes [#117](opentdf/platform#117) * key access server registry impl ([#66](opentdf/platform#66)) ([cf6b3c6](opentdf/platform@cf6b3c6)) * **namespaces CRUD:** protos, generated SDK, db interactivity for namespaces table ([#54](opentdf/platform#54)) ([b3f32b1](opentdf/platform@b3f32b1)) * **PLAT-3112:** Initial consumption of ec_key_pair functions by nanotdf ([#586](opentdf/platform#586)) ([5e2cba0](opentdf/platform@5e2cba0)) * **policy:** add FQN pivot table ([#208](opentdf/platform#208)) ([abb734c](opentdf/platform@abb734c)) * **policy:** add soft-delete/deactivation to namespaces, attribute definitions, attribute values [#96](opentdf/platform#96) [#108](opentdf/platform#108) ([#191](opentdf/platform#191)) ([02e92a6](opentdf/platform@02e92a6)) * **resourcemapping:** resource mapping implementation ([#83](opentdf/platform#83)) ([c144db1](opentdf/platform@c144db1)) * **sdk:** BACK-1966 get auth wired up to SDK using `Options` ([#271](opentdf/platform#271)) ([f1bacab](opentdf/platform@f1bacab)) * **sdk:** BACK-1966 implement fetching a DPoP token ([#45](opentdf/platform#45)) ([dbd3cf9](opentdf/platform@dbd3cf9)) * **sdk:** BACK-1966 make the unwrapper retrieve public keys as well ([#260](opentdf/platform#260)) ([7d051a1](opentdf/platform@7d051a1)) * **sdk:** BACK-1966 pull rewrap into auth config ([#252](opentdf/platform#252)) ([84017aa](opentdf/platform@84017aa)) * **sdk:** Include auth token in grpc ([#367](opentdf/platform#367)) ([75cb5cd](opentdf/platform@75cb5cd)) * **sdk:** normalize token exchange ([#546](opentdf/platform#546)) ([9059dff](opentdf/platform@9059dff)) * **sdk:** Pass dpop key through to `rewrap` ([#435](opentdf/platform#435)) ([2d283de](opentdf/platform@2d283de)) * **sdk:** read `expires_in` from token response and use it to refresh access tokens ([#445](opentdf/platform#445)) ([8ecbe79](opentdf/platform@8ecbe79)) * **sdk:** sdk stub ([#10](opentdf/platform#10)) ([8dfca6a](opentdf/platform@8dfca6a)) * **sdk:** take a function so that callers can use this the way that they want ([#340](opentdf/platform#340)) ([72059cb](opentdf/platform@72059cb)) * **subject-mappings:** refactor to meet db schema ([#59](opentdf/platform#59)) ([59a073b](opentdf/platform@59a073b)) * **tdf:** implement tdf3 encrypt and decrypt ([#73](opentdf/platform#73)) ([9d0e0a0](opentdf/platform@9d0e0a0)) * **tdf:** sdk interface changes ([#123](opentdf/platform#123)) ([2aa2422](opentdf/platform@2aa2422)) * **tdf:** sdk interface cleanup ([#201](opentdf/platform#201)) ([6f7d815](opentdf/platform@6f7d815)) * **tdf:** TDFOption varargs interface ([#235](opentdf/platform#235)) ([b3fb720](opentdf/platform@b3fb720)) ### Bug Fixes * **archive:** remove 10gb zip file test ([#373](opentdf/platform#373)) ([6548f55](opentdf/platform@6548f55)) * attribute missing rpc method for listing attribute values ([#69](opentdf/platform#69)) ([1b3a831](opentdf/platform@1b3a831)) * **attribute value:** fixes attribute value crud ([#86](opentdf/platform#86)) ([568df9c](opentdf/platform@568df9c)) * **issue 90:** remove duplicate attribute_id from attribute value create/update, and consumes schema setup changes in namespaces that were introduced for integration testing ([#100](opentdf/platform#100)) ([e0f6d07](opentdf/platform@e0f6d07)) * **issue-124:** SDK kas registry import name mismatch ([#125](opentdf/platform#125)) ([112638b](opentdf/platform@112638b)), closes [#124](opentdf/platform#124) * **proto/acre:** fix resource encoding service typo ([#30](opentdf/platform#30)) ([fe709d2](opentdf/platform@fe709d2)) * remove padding when b64 encoding ([#437](opentdf/platform#437)) ([d40e94a](opentdf/platform@d40e94a)) * SDK Quickstart ([#628](opentdf/platform#628)) ([f27ab98](opentdf/platform@f27ab98)) * **sdk:** change unwrapper creation ([#346](opentdf/platform#346)) ([9206435](opentdf/platform@9206435)) * **sdk:** double bearer token in auth config ([#350](opentdf/platform#350)) ([1bf4699](opentdf/platform@1bf4699)) * **sdk:** fixes Manifests JSONs with OIDC ([#140](opentdf/platform#140)) ([a4b6937](opentdf/platform@a4b6937)) * **sdk:** handle err ([#548](opentdf/platform#548)) ([ebabb6c](opentdf/platform@ebabb6c)) * **sdk:** make KasInfo fields public ([#320](opentdf/platform#320)) ([9a70498](opentdf/platform@9a70498)) * **sdk:** shutdown conn ([#352](opentdf/platform#352)) ([3def038](opentdf/platform@3def038)) * **sdk:** temporarily move unwrapper creation into options func. ([#309](opentdf/platform#309)) ([b34c2fe](opentdf/platform@b34c2fe)) * **sdk:** use the dialoptions even with no client credentials ([#400](opentdf/platform#400)) ([a7f1908](opentdf/platform@a7f1908)) * **security:** add a new encryption keypair different from dpop keypair ([#461](opentdf/platform#461)) ([7deb51e](opentdf/platform@7deb51e)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>

feat(tdf): sdk interface changes

bbfd287

sujankota requested a review from a team as a code owner February 4, 2024 20:37

sujankota requested review from biscoe916, mkleene and strantalis February 4, 2024 20:37

patmantru previously approved these changes Feb 5, 2024

View reviewed changes

minor cleanup

1a72b4f

sujankota dismissed patmantru’s stale review via 1a72b4f February 5, 2024 16:02

dmihalcik-virtru previously approved these changes Feb 5, 2024

View reviewed changes

Merge branch 'main' into feature/sdk-tdf-interface

984354f

strantalis reviewed Feb 6, 2024

View reviewed changes

sdk/tdf.go Outdated Show resolved Hide resolved

Benchmark test and WriteTo interface

b81f220

sujankota dismissed dmihalcik-virtru’s stale review via b81f220 February 6, 2024 19:26

revert go.work.sum

001f739

dmihalcik-virtru approved these changes Feb 6, 2024

View reviewed changes

sujankota merged commit 2aa2422 into main Feb 6, 2024

sujankota deleted the feature/sdk-tdf-interface branch February 6, 2024 21:32

opentdf-automation bot mentioned this pull request Apr 18, 2024

chore(main): release sdk 0.1.0 #612

Merged

		for index, seg := range reader.manifest.EncryptionInformation.IntegrityInformation.Segments {
		if firstSegment > int64(index) {

-	for index, seg := range reader.manifest.EncryptionInformation.IntegrityInformation.Segments {
-		if firstSegment > int64(index) {
+	for i32, seg := range reader.manifest.EncryptionInformation.IntegrityInformation.Segments {
+	  index := int64(i32)
+		if firstSegment > index {

feat(tdf): sdk interface changes #123

feat(tdf): sdk interface changes #123

Uh oh!

Conversation

sujankota commented Feb 4, 2024

Uh oh!

dmihalcik-virtru left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmihalcik-virtru Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmihalcik-virtru commented Feb 6, 2024

Uh oh!

Uh oh!

dmihalcik-virtru commented Feb 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dmihalcik-virtru Feb 6, 2024 •

edited

Loading