-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Key Vault Admin] Tests #11010
[Key Vault Admin] Tests #11010
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but should this PR include the recordings as well?
sdk/keyvault/keyvault-admin/test/public/accessControlClient.spec.ts
Outdated
Show resolved
Hide resolved
sdk/keyvault/keyvault-admin/test/public/accessControlClient.spec.ts
Outdated
Show resolved
Hide resolved
sdk/keyvault/keyvault-admin/test/public/accessControlClient.spec.ts
Outdated
Show resolved
Hide resolved
SAS tokens for the test account were leaked in the last PR with the recordings. These keys have been revoked by this point. I added the code to ensure a leak like this won't happen again.
@@ -29,7 +29,7 @@ nock('https://eastus2.keyvault_name.managedhsm-int.azure-int.net:443', {"encoded | |||
'no-cache' ]); | |||
|
|||
nock('https://eastus2.keyvault_name.managedhsm-int.azure-int.net:443', {"encodedQueryParams":true}) | |||
.post('/backup', {"storageResourceUri":"https://chrissprim.blob.core.windows.net/backup","token":"?sv=2019-12-12&ss=bfqt&srt=sco&sp=rwdlacupx&se=2022-07-14T05:56:52Z&st=2020-09-03T21:56:52Z&spr=https,http&sig=Rzts1hBG%2BgqTCTZo%2BX%2FZgJWJ5Ao16ueN%2F1BB4Dg8%2FLo%3D"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The keys of these SAS tokens were revoked within the same day of the recording. They're harmless. The team is aware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR now includes a fix that prevents further SAS tokens to be leaked for the kv-admin.
}, | ||
"BLOB_STORAGE_URI": { | ||
"type": "string", | ||
"value": "7.2-preview-blob-storage-uri" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These need more work. This package won't run live tests for now. I'm leaving this as a reminder of what I have to do later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we don't have a Managed HSM test environment for the pipeline runs yet, so I think this should be ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a proper fix in the roadmap, so I won't log another issue.
sdk/keyvault/keyvault-admin/test/public/accessControlClient.spec.ts
Outdated
Show resolved
Hide resolved
* WIP * some more content * README WIP * The readme seems good for a review * wip * the rest of the docs and samples for now * fixed references to keys * fixed bad path * fixed bad path references in kv-keys * feedback Addressed: - Azure#11075 (comment) Mentioning RBAC at least once per file. - Azure#11075 (comment) Adding the imports to the readme snippets. - Azure#11075 (comment) Declaring the vaultUrl variable in the other snippet in the main README for keyvault-admin. - Azure#11075 (comment) By expanding most of the references to KeyVaultRoleDefinition and KeyVaultRoleAssignment. - Azure#11075 (comment) by removing en-us from the URLs. - Azure#11075 (comment) by renaming the sample file backupHelloWorld.(js|ts) to backupRestoreHelloWorld.(js|ts) - Azure#11075 (comment) by renaming the sample file backupSelective.(js|ts) to backupSelectiveRestore.(js|ts). - Azure#11075 (comment) by getting a role assignment in the samples. - Azure#11075 (comment) By correctly referencing the link to core-lro in the main README. - Azure#11075 (comment) By fixing a syntax issue. * removed some more en-us * more backupRestoreHelloWorld and backupSelectiveRestore instances * Fixed Azure#11075 (comment) * some fixes Re-arranged some sections to make more sense, which hopefully addresses: Azure#11075 (comment) Fixed the core-lro link. Fixes Azure#11075 (comment) Replaced `Admin` with `Administration`, to stay consistent. Lowered `Long Running Operations`, thus addressing: Azure#11075 (comment) * Update sdk/keyvault/keyvault-admin/README.md Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com> * more changes - Lowercased some LRO references. - One fixes: Azure#11075 (comment) - Changed the example intro paragraph, which hopefully addresses: - Azure#11075 (comment) - Cleaned up the environment variables with this too: Azure#11075 (comment) * Update sdk/keyvault/keyvault-admin/CHANGELOG.md Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com> * Addressing Azure#11075 (comment) * Addressing Azure#11075 (comment) and merging some lines * Addressing Azure#11075 (comment) * Addressing Azure#11075 (comment) * Addressing two feedbacks - Azure#11075 (comment) - Azure#11075 (comment) * Addressing Azure#11075 (comment) * Clarification about the CLIENT_OBJECT_ID * fixing samples link for CI. Later well make them absolute * removing links to the source since they are broken at the moment Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>
Removed the generation of unnecessary credentials. Addresses: - Azure#11010 (comment) - Azure#11010 (comment)
fbc8991
to
c57f3d6
Compare
try { | ||
return await client.fullBackupStatus(vaultUrl, jobId, options); | ||
return await client.restoreStatus(vaultUrl, jobId, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important change found through careful testing, and with the help of @vcolin7
try { | ||
return await client.fullBackupStatus(vaultUrl, jobId, setParentSpan(span, options)); | ||
return await client.restoreStatus(vaultUrl, jobId, setParentSpan(span, options)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important change found through careful testing, and with the help of @vcolin7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
nock('https://eastus2.keyvault_name.managedhsm.azure.net:443', {"encodedQueryParams":true}) | ||
.get('///providers/Microsoft.Authorization/roleAssignments/b36b00af-89c6-435f-a43d-9a3087015c27') | ||
.query(true) | ||
.reply(404, {"error":{"code":"RoleAssignmentNotFound","message":"Requested role assignment not found (Activity ID: e6aeb748-f2ae-11ea-857a-0242ac120004)"}}, [ 'content-type', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is when we try to get the assignment after we deleted it.
'x-content-type-options', | ||
'nosniff', | ||
'www-authenticate', | ||
'Bearer authorization="https://login.windows-ppe.net/azure_tenant_id", resource="https://managedhsm-int.azure-int.net"', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was unable to obscure this instance, since the requests would break saying that the resource wasn't found, something like that. .Net doesn't obscure this instance at all, so I'm not planning on going more in depth.
nock('https://eastus2.keyvault_name.managedhsm.azure.net:443', {"encodedQueryParams":true}) | ||
.get('/restore/89317af12e784adb89a1176762b907dd/pending') | ||
.query(true) | ||
.reply(200, {"endTime":1599781044,"error":null,"jobId":"89317af12e784adb89a1176762b907dd","startTime":1599781032,"status":"Succeeded","statusDetails":null}, [ 'server', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the service response for the restore operation.
nock('https://eastus2.keyvault_name.managedhsm-int.azure-int.net:443', {"encodedQueryParams":true}) | ||
.put('/keys/beginBackupthenbeginSelectiveRestore/restore', {"sasTokenParameters":{"storageResourceUri":"https://chrissprim.blob.core.windows.net/backup","token":"?sv=2019-12-12&ss=bfqt&srt=sco&sp=rwdlacupx&se=2022-07-14T05:56:52Z&st=2020-09-03T21:56:52Z&spr=https,http&sig=Rzts1hBG%2BgqTCTZo%2BX%2FZgJWJ5Ao16ueN%2F1BB4Dg8%2FLo%3D"},"folder":"https://chrissprim.blob.core.windows.net/backup"}) | ||
nock('https://eastus2.keyvault_name.managedhsm.azure.net:443', {"encodedQueryParams":true}) | ||
.put('/keys/rsa-1/restore', {"sasTokenParameters":{"storageResourceUri":"https://uri.blob.core.windows.net/backup","token":"blob_storage_sas_token"},"folder":"mhsm-keyvault_name-2020091100085704"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a existing key we're using for testing in Java too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using an existing key simplifies the tests considerably. Over time we can expand and make more in depth tests.
|
||
state.endTime = endTime; | ||
state.status = status; | ||
state.statusDetails = statusDetails; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this PR shouldn't have been about source code changes, but it ends up involving them due to bugs found as I tested.
Also note that, even though this whole file is considerably similar to the restore operation file (noting that this is the selectiveRestore operation file), I don't want to preemptively unify them. Unification here would suggest me making some form of "common" set of resources for LRO operations, which is something I'm not sure we should do at the moment. We might go that route in the future for sure.
(recording: any): any => | ||
recording.replace( | ||
/keyvault_name\.[a-z-]+\.azure[a-z-]*\.net/g, | ||
`keyvault_name.managedhsm.azure.net` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing \.[a-z-]+\.azure[a-z-]*\.net
only causes issues with the HTTP requests. I don't want to go deeper since this isn't obscured in other languages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@@ -60,7 +60,8 @@ | |||
"extract-api": "tsc -p . && api-extractor run --local", | |||
"format": "prettier --write --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"test/**/*.ts\" \"*.{js,json}\"", | |||
"integration-test:browser": "echo skipped", | |||
"integration-test:node": "echo skipped", | |||
"integration-test:node": "nyc mocha -r esm --require source-map-support/register --reporter ../../../common/tools/mocha-multi-reporter.js --timeout 180000 --full-trace dist-esm/**/*.spec.js", | |||
"integration-test:node:no-timeout": "nyc mocha -r esm --require source-map-support/register --reporter ../../../common/tools/mocha-multi-reporter.js --no-timeouts --full-trace dist-esm/**/*.spec.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like to use this!
@@ -66,7 +66,7 @@ export function nodeConfig(test = false) { | |||
// different output file | |||
baseConfig.output.file = "dist-test/index.node.js"; | |||
|
|||
baseConfig.external.push("assert", "fs", "path"); | |||
baseConfig.external.push("assert", "fs", "path", "chai"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we mark all dev deps as external in TA here cc @willmtemple. Anyway #10923 will help us standardize those configs anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's think about this later then. Thank you!
Thank you, @deyaaeldeen , for the review 🙌♥ |
Tests for the Key Vault Admin clients.
This PR will fail until these PRs are merged:
[Key Vault Admin] Convenience layer - KeyVaultAccessControlClient [Key Vault Admin] Convenience layer - KeyVaultAccessControlClient #10815Merged.[Key Vault Admin] Convenience layer - KeyVaultBackupClient [Key Vault Admin] Convenience layer - KeyVaultBackupClient #11009Merged.[Key Vault Admin] Recordings [Key Vault Admin] Recordings #11042Merged.[Key Vault Admin] No LROs on the generated code for now [Key Vault Admin] No LROs on the generated code for now #11008Merged.For: #10799