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

Get/set legal hold APIs #925

Merged
merged 5 commits into from
Jun 7, 2021
Merged

Conversation

prakashsvmx
Copy link
Member

@prakashsvmx prakashsvmx commented Apr 26, 2021

Legal hold APIs support

getObjectLegalHold(bucketName, objectName, getOpts={}, cb)
setObjectLegalHold(bucketName, objectName, setOpts={}, cb)

Note:
To run all functional tests locally the below must be exported and certificates as required:
export MINIO_KMS_KES_ENDPOINT=https://play.min.io:7373;export MINIO_KMS_KES_KEY_FILE=root.key;export MINIO_KMS_KES_CERT_FILE=root.cert;export MINIO_KMS_KES_KEY_NAME=my-minio-key;

@prakashsvmx prakashsvmx self-assigned this Apr 26, 2021
@prakashsvmx prakashsvmx force-pushed the legal-hold-apis branch 2 times, most recently from d8e58b7 to b478c33 Compare April 28, 2021 09:45
@prakashsvmx prakashsvmx marked this pull request as ready for review May 3, 2021 12:36
@prakashsvmx prakashsvmx force-pushed the legal-hold-apis branch 2 times, most recently from e02295c to 49800a0 Compare May 27, 2021 07:39
docs/API.md Outdated Show resolved Hide resolved
src/main/minio.js Outdated Show resolved Hide resolved
kannappanr
kannappanr previously approved these changes May 28, 2021
Copy link
Contributor

@kannappanr kannappanr left a comment

Choose a reason for hiding this comment

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

LGTM

kannappanr
kannappanr previously approved these changes Jun 2, 2021
Copy link
Contributor

@kannappanr kannappanr left a comment

Choose a reason for hiding this comment

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

LGTM

.on('data', data => {
legalHoldConfig = data
})
.on('error', cb)
Copy link
Contributor

@ebozduman ebozduman Jun 2, 2021

Choose a reason for hiding this comment

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

When there is no legalhold on an object, or when the bucket doesn't exist, it looks like the backend sends a response error and the stack trace and we simply show the error message and the stack trace.

If there is no legalhold on the object, that is a legit state and hence we may gracefully handle it and behave like mc. However, we can say this is not SDK responsibility and if something needs to be fixed, it needs to be done at the backend.

If the bucket doesn't exist, the error message is kind of misleading, but that is definitely a backend issue. I;d expect to see an error like "Bucket doesn't exist" or something.
UnhandledPromiseRejectionWarning: S3Error: Bucket is missing ObjectLockConfiguration

Here is the mc and api's behavior if the object does not have a legal hold on and we query its legalhold status.

$ mc legalhold info --versions s3/erslock/legal/file 
[  Not set ]  Jka2bUOMoe4uVUNUMc8dyZXkxzcC1tGL  file
[    ON    ]  L2zN6WG8AMgZjz3DpIH0zOmhziGjDgBX  file
[  Not set ]  DFYkQ9qTQJaIcbqZZLftRANf3c6peNqS  file
[    ON    ]  b8N7Ccujn5Mph5lyO9OylWTgNYniLaa9  file
$
$ mc legalhold info --vid L2zN6WG8AMgZjz3DpIH0zOmhziGjDgBX s3/erslock/legal/file
[    ON    ]  L2zN6WG8AMgZjz3DpIH0zOmhziGjDgBX  file
$
$ mc legalhold info --vid DFYkQ9qTQJaIcbqZZLftRANf3c6peNqS s3/erslock/legal/file
[  Not set ]  DFYkQ9qTQJaIcbqZZLftRANf3c6peNqS  file
$
$ node run-get-object-legal-hold.js # gets legalhold status of object `file` version `DFYkQ9qTQJaIcbqZZLftRANf3c6peNqS`
Unable to get legal hold config for the object S3Error: The specified object does not have a ObjectLock configuration
    at Object.parseError (/home/ersan/work/src/github.com/minio/minio-js/dist/main/xml-parsers.js:89:11)
    at /home/ersan/work/src/github.com/minio/minio-js/dist/main/transformers.js:164:22
    at DestroyableTransform._flush (/home/ersan/work/src/github.com/minio/minio-js/dist/main/transformers.js:88:10)
    at DestroyableTransform.prefinish (/home/ersan/work/src/github.com/minio/minio-js/node_modules/readable-stream/lib/_stream_transform.js:138:10)
    at DestroyableTransform.emit (events.js:315:20)
    at prefinish (/home/ersan/work/src/github.com/minio/minio-js/node_modules/readable-stream/lib/_stream_writable.js:619:14)
    at finishMaybe (/home/ersan/work/src/github.com/minio/minio-js/node_modules/readable-stream/lib/_stream_writable.js:627:5)
    at endWritable (/home/ersan/work/src/github.com/minio/minio-js/node_modules/readable-stream/lib/_stream_writable.js:638:3)
    at DestroyableTransform.Writable.end (/home/ersan/work/src/github.com/minio/minio-js/node_modules/readable-stream/lib/_stream_writable.js:594:41)
    at IncomingMessage.onend (_stream_readable.js:678:10) {
  code: 'NoSuchObjectLockConfiguration',
  requestid: 'X92ZKEDWQJ4DGX1J',
  hostid: 'tanAjE6k3Vlqj+Dnwid64KtRScyPlqPmB5UY3Ae0NPJ+rJjdzSH4UH8jfPH1KM5333QQAcdqiHI=',
  amzRequestid: null,
  amzId2: null,
  amzBucketRegion: null
}

Copy link
Member Author

Choose a reason for hiding this comment

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

In the examples, we tend to print the entire javascript error object which prints the callstack.

I will update the examples as well to clarify this.

instead, the caller function could just use:

s3Client.getObjectLegalHold('bucketName', 'objectName', { versionId:'my-obj-version-uuid' }, function(err, res) {
  if (err) {
    return console.log('Unable to get legal hold config for the object', err.message) // Print only the message.
  }
  console.log(res)
})

examples/set-object-legal-hold.js Outdated Show resolved Hide resolved
docs/API.md Show resolved Hide resolved
docs/API.md Outdated Show resolved Hide resolved
@prakashsvmx
Copy link
Member Author

@ebozduman thank you for the review. I will update it.

kannappanr
kannappanr previously approved these changes Jun 3, 2021
Copy link
Contributor

@kannappanr kannappanr left a comment

Choose a reason for hiding this comment

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

LGTM

examples/get-object-legal-hold.js Outdated Show resolved Hide resolved
docs/API.md Outdated Show resolved Hide resolved
ebozduman
ebozduman previously approved these changes Jun 4, 2021
Copy link
Contributor

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

LGTM when the last requested change is in.

Copy link
Contributor

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kannappanr kannappanr left a comment

Choose a reason for hiding this comment

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

LGTM

@kannappanr kannappanr merged commit 8969396 into minio:master Jun 7, 2021
@prakashsvmx prakashsvmx deleted the legal-hold-apis branch February 10, 2022 16:53
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

Successfully merging this pull request may close these issues.

4 participants