Azure / azure-sdk-for-go

This repository is for active development of the Azure SDK for Go. For consumers of the SDK we recommend visiting our public developer docs at:
https://docs.microsoft.com/azure/developer/go/
MIT License
1.64k stars 840 forks source link

Time should be correctly serialized to ISO 8601 #21584

Open giuliohome opened 1 year ago

giuliohome commented 1 year ago

Bug Report

I would expect that, if the type-safe static checks pass, there should be no dynamic errors raised for something as format-specific as this. In other world I would expect that the time should be correctly serialized, e.g. with 7 decimal places, if that is required by the API specs. In case of this code - using package "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/sas" -

blob_client, err := azblob.NewClientWithSharedKeyCredential(fmt.Sprintf("https://%s.blob.core.windows.net/", storageAccountName), scred, nil)
sas_url, err := blob_client.ServiceClient().GetSASURL(
    sas.AccountResourceTypes{ Container: true },
    sas.AccountPermissions{
        Create: true, Delete: true, List: true, Add: true,
    },
    time.Now().Add(24 * time.Hour),
    nil,    
)

I don't have to round the time, which is what I would expect also in case of my repro below

client := storageClientFactory.NewAccountsClient()
sasToken, err := client.ListAccountSAS(ctx, resourceGroupName, storageAccountName, armstorage.AccountSasParameters{
    KeyToSign:              to.Ptr("key1"),
    SharedAccessExpiryTime: to.Ptr(time.Now().Add(24 * time.Hour)),
    Permissions:            to.Ptr(armstorage.PermissionsR),
    Protocols:              to.Ptr(armstorage.HTTPProtocolHTTPSHTTP),
    ResourceTypes:          to.Ptr(armstorage.SignedResourceTypesS),
    Services:               to.Ptr(armstorage.ServicesB),
    SharedAccessStartTime:  to.Ptr(time.Now()),
}, nil)

In the above case of ListAccountSAS, I need to add something like .UTC() or.Round(time.Second) to make it work, while that is not needed for GetSASURL.

giuliohome commented 1 year ago

I'm definitely going to use

    sasQueryParams, err := sas.BlobSignatureValues{
        Protocol:      sas.ProtocolHTTPS,
        StartTime:     time.Now(),
        ExpiryTime:    time.Now().Add(15 * time.Minute),
        Permissions:   to.Ptr(sas.ContainerPermissions{Read: true, List: true}).String(),
        ContainerName: containerName,
    }.SignWithSharedKey(scred)

that appears to work as I would expect regarding the time parameters (and also because I can restrict the SAS token to a specific container!). Only the UTC() conversion is still necessary for handling different timezones, but at least the rounding is implicitly managed.

I'll leave this open for those importing the armstorage package.

siminsavani-msft commented 1 year ago

Glad to hear that the SignWithSharedKey is working fine! Please let me know if you have any other issues with azblob!

siminsavani-msft commented 1 year ago

Moving this over to Mangement to figure out the issue with ListAccountSAS and the time serialization mentioned in the post earlier.

giuliohome commented 1 year ago

The issue with armstorage is that the models_serde uses populateTimeRFC3339 for the time, which is not ISO8601.

Minor note The suggestion for azblob sas is to perform UTC conversion, eliminating the need for users to do it themselves.

raych1 commented 1 year ago

@jhendrixMSFT do you have any insight on why it uses different date time format in this case?

jhendrixMSFT commented 1 year ago

@siminsavani-msft I think there's still an ask here for sas to convert the time to UTC.

jhendrixMSFT commented 1 year ago

I'll also note that converting the times to UTC before calling ListAccountSAS resolves the issue. At any rate, we'll look at getting this fixed in the SDKs.

raych1 commented 11 months ago

@jhendrixMSFT, as we synced offline I assigned this issue to you as we still want a fix though it's not a blocker.