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

After calling disks.grantAccess successfully, accessSas is undefined #17499

Closed
2 of 6 tasks
MRayermannMSFT opened this issue Sep 7, 2021 · 21 comments
Closed
2 of 6 tasks
Assignees
Labels
Compute customer-reported Issues that are reported by GitHub users external to the Azure organization. Mgmt This issue is related to a management-plane library. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that

Comments

@MRayermannMSFT
Copy link

  • Package Name: arm-compute
  • Package Version: 16.4.0
  • Operating system:
  • nodejs
    • version: 14
  • browser
    • name/version:
  • typescript
    • version: 4.3.5
  • Is the bug related to documentation in

Describe the bug
In the return value for disks.grantAccess, accessSas is undefined.

To Reproduce

  1. Call disks.grantAccess such that it succeeds
  2. Inspect value of accessSas on the return value

Expected behavior
The access SAS is there.

Screenshots
N/A

Additional context
You can get to the SAS by doing:

(grantAccessResult as any)._response.parsedBody.properties.output.accessSAS

This is not a regression, I've just been lazy about opening this issue. ¯\(ツ)

@ghost ghost added needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Sep 7, 2021
@ramya-rao-a ramya-rao-a added Compute Mgmt This issue is related to a management-plane library. labels Sep 8, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Sep 8, 2021
@qiaozha
Copy link
Member

qiaozha commented Dec 28, 2021

@MRayermannMSFT Hi, sorry for the late response, I tried on my side and the accessSas is not undefined, Would you mind share some of your code with us so that we can look into the root cause of this issue?

@qiaozha qiaozha added the needs-author-feedback Workflow: More information is needed from author to address the issue. label Dec 28, 2021
@ghost ghost added the no-recent-activity There has been no recent activity on this issue. label Jan 4, 2022
@ghost
Copy link

ghost commented Jan 4, 2022

Hi, we're sending this friendly reminder because we haven't heard back from you in a while. We need more information about this issue to help address it. Please be sure to give us your input within the next 7 days. If we don't hear back from you within 14 days of this comment the issue will be automatically closed. Thank you!

@MRayermannMSFT
Copy link
Author

@qiaozha can you confirm what version of arm-compute you are using? I'm still on 16.5, but it seems like 17 was released recently. The breaking changes for that are significant enough that it will take a while for us to onboard to that. So let me know with what version you do not reproduce this issue.

@ghost ghost added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-author-feedback Workflow: More information is needed from author to address the issue. no-recent-activity There has been no recent activity on this issue. labels Jan 4, 2022
@qiaozha
Copy link
Member

qiaozha commented Jan 5, 2022

I tried both 16.5.0 and the latest one and it works on my side. Here's my code, hope it's helpful.

import { ComputeManagementClient } from '@azure/arm-compute';
import { ClientSecretCredential, DefaultAzureCredential } from '@azure/identity';
async function main() {
    const subscriptionId = "";
    const AZURE_CLIENT_ID="";
    const AZURE_CLIENT_SECRET=""; 
    const AZURE_TENANT_ID="";
    const creds = new ClientSecretCredential(AZURE_TENANT_ID, AZURE_CLIENT_ID, AZURE_CLIENT_SECRET);
    const client = new ComputeManagementClient(creds, subscriptionId);
    const resourceGroupName = "qiaozhatest";
    const diskName = "qiaozhavmtest";
    // const result2 = [];
    // for await (const item of client.operations.list()) {
    //     result2.push(item);
    // }
    // console.log(result2);
    const result1 = await client.disks.createOrUpdate(resourceGroupName, diskName, {
        location: "eastus",
        creationData: {
          createOption: "Empty",
        },
        diskSizeGB: 200,
    });
    console.log(result1);
    const result = await client.disks.grantAccess(resourceGroupName, diskName, {
        access: "Read",
        durationInSeconds: 3600
    });
    console.log(result);
}

main().catch(console.error);

@qiaozha
Copy link
Member

qiaozha commented Jan 5, 2022

Here's the result on my side
image

@qiaozha qiaozha added the needs-author-feedback Workflow: More information is needed from author to address the issue. label Jan 5, 2022
@ghost ghost removed the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Jan 5, 2022
@MRayermannMSFT
Copy link
Author

Here's the result on my side

So you do reproduce the issue then. :) Check the typing for what grant access returns. The accessSAS property is supposed to be a top level string property. Right?

@ghost ghost added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-author-feedback Workflow: More information is needed from author to address the issue. labels Jan 5, 2022
@qiaozha
Copy link
Member

qiaozha commented Jan 5, 2022

I see your point now

@Sandido
Copy link
Member

Sandido commented Jan 19, 2022

Hi @MRayermannMSFT ,

  1. Why do you say 'accessSAS' is supposed to be a top level property?
  2. What type is accessSAS currently?
  3. Is the expected behavior supposed to be DiskGrantAccessResponse.accessSAS ?

@MRayermannMSFT
Copy link
Author

@Sandido

Why do you say 'accessSAS' is supposed to be a top level property?

Because that is what the typing indicates?
image
image

What type is accessSAS currently?

Do you mean in the typing? A string. See my above screenshot.

Is the expected behavior supposed to be DiskGrantAccessResponse.accessSAS ?

That is what the typing indicates, yes.

@qiaozha
Copy link
Member

qiaozha commented Jan 20, 2022

@Sandido The SDK is generated from the swagger file. And as you can see the definition https://github.com/Azure/azure-rest-api-specs/blob/main/specification/compute/resource-manager/Microsoft.Compute/stable/2021-08-01/disk.json#L3044-L3057 for AccessUri here, and the response model that refers to AccessUri https://github.com/Azure/azure-rest-api-specs/blob/main/specification/compute/resource-manager/Microsoft.Compute/stable/2021-08-01/disk.json#L377 directly, there's nothing like properties.output.accessSAS defined in between. but we can see it from the response. which is inconsistent.

Hope that can make it clear.

@Sandido
Copy link
Member

Sandido commented Jan 27, 2022

I'll let @qiaozha continue with this and involve someone from the JS SDK team for better input.
I don't know why the JS code indicates accessSAS should be a top level property. The swagger file for disk.json defines the accessSAS property response as being within a Properties bag.

@Sandido
Copy link
Member

Sandido commented Jan 27, 2022

Didn't see last comment.

Perhaps this is an Autorest issue that occurs in the SDK generation for JS.
I'm on the .Net SDK side of things, and I commented on this item because it appeared in my team's S360 KPI report.

@qiaozha
Copy link
Member

qiaozha commented Jan 28, 2022

@Sandido I don't think there's anything wrong in the SDK generator for JS for this issue. According the issue and above comment, there's a clear inconsistency between the swagger response definition and the live traffic response and SDK response type is always the same with the swagger definition, if you think the live traffic response we get is the correct one, then you should fix the swgger, if you think the swagger definition is the correct one, then you should fix your service backend.

@qiaozha
Copy link
Member

qiaozha commented Feb 16, 2022

@MRayermannMSFT I think this issue have been resolved. Could you try again and let us know if it's not ? Thanks

@qiaozha qiaozha added the needs-author-feedback Workflow: More information is needed from author to address the issue. label Feb 16, 2022
@ghost ghost removed the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Feb 16, 2022
@ghost ghost added the no-recent-activity There has been no recent activity on this issue. label Feb 23, 2022
@ghost
Copy link

ghost commented Feb 23, 2022

Hi, we're sending this friendly reminder because we haven't heard back from you in a while. We need more information about this issue to help address it. Please be sure to give us your input within the next 7 days. If we don't hear back from you within 14 days of this comment the issue will be automatically closed. Thank you!

@MRayermannMSFT
Copy link
Author

@qiaozha can you be more specific on how/where it has been resolved? I tried again this morning and I am getting the same result. I am on v16.5.0 and I do not see a higher 16.x.y on NPM.

@ghost ghost added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-author-feedback Workflow: More information is needed from author to address the issue. no-recent-activity There has been no recent activity on this issue. labels Feb 23, 2022
@qiaozha
Copy link
Member

qiaozha commented Feb 24, 2022

@MRayermannMSFT The version I am trying is 17.1.0
image

I believe this is align with the swagger definition.

Actually I would suggest you to migration to version 17.x.x and above because we will going to have an announcement to deprecate version lower than 17.0.0 of @azure/arm-compute early next month (March 2022). About the breaking change between the previous version and the next generation i.e. 17.x.x and above of @azure/arm-compute. You may refer to this migration guideline for help.

@qiaozha qiaozha added the needs-author-feedback Workflow: More information is needed from author to address the issue. label Feb 24, 2022
@ghost ghost removed the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Feb 24, 2022
@MRayermannMSFT
Copy link
Author

@qiaozha this issue is currently blocking our upgrading to v17: #20380

@ghost ghost added needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team and removed needs-author-feedback Workflow: More information is needed from author to address the issue. labels Feb 25, 2022
@vikramd-ms
Copy link

@MRayermannMSFT, @qiaozha

Can the original issue with grantAccess be considered resolved?

@MRayermannMSFT
Copy link
Author

I haven't had a chance to migrate to 17 yet. I understand y'all wanting to close this issue though if you think the issue is resolve there. I can always open another issue post migration.

@Sandido
Copy link
Member

Sandido commented Jun 15, 2022

@MRayermannMSFT , I will go ahead and close this issue for now. If this issue is not resolved when you migrate and test on 17, feel free to reopen this or make a new issue.

@Sandido Sandido closed this as completed Jun 15, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Compute customer-reported Issues that are reported by GitHub users external to the Azure organization. Mgmt This issue is related to a management-plane library. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that
Projects
None yet
Development

No branches or pull requests

5 participants