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.62k stars 826 forks source link

TTL and value for CNAME record does not get updated while creating a DNS zone record set #1161

Closed kirankumarbv closed 6 years ago

kirankumarbv commented 6 years ago

Hi,

I referenced one of the DNS examples to create a CNAME record using SDK. It used to work perfectly in the pre 14 versions. But after the recent SDK change, I am hitting an error where the record created in a hosted zone does not have TTL or value information even though I send it through my CreateOrUpdate parameter. Below is the snippet I use to create the record.

`cnameRecordParams := &dns.RecordSet{ ID: &name, RecordSetProperties: &dns.RecordSetProperties{ TTL: &ttl, CnameRecord: &dns.CnameRecord{ Cname: &value, }, }, }

    _, err = DNS.DnsRrsClient.CreateOrUpdate(ctx, resGrp, zoneId, name, rrsType, *cnameRecordParams, "", "")

`

ttl and value are of string data type. I went inside the CreateOrUpdate method and all the arguments are being passed properly.

Please let me know if I am missing something or if this is a bug?

jhendrixMSFT commented 6 years ago

In models.go could you try deleting the JSON marshaler for RecordSetProperties and see if the problem goes away? https://github.com/Azure/azure-sdk-for-go/blob/master/services/dns/mgmt/2017-09-01/dns/models.go#L328

jhendrixMSFT commented 6 years ago

Assigning to Vlad as I think this is an issue with generating spurious marshalers/unmarshalers.

kirankumarbv commented 6 years ago

Hi @jhendrixMSFT It worked as expected when I removed JSON marshaler for RecordSetProperties.

vladbarosan commented 6 years ago

@kirankumarbv I think i know whats the issue can you try keeping it as it was originally ( without the change Joel mentioned )and add this to (https://github.com/Azure/azure-sdk-for-go/blob/master/services/dns/mgmt/2017-09-01/dns/models.go#L328):

// MarshalJSON is the custom marshaler for RecordSet.
func (rs RecordSet) MarshalJSON() ([]byte, error) {
    objectMap := make(map[string]interface{})
    if rs.ID != nil {
        objectMap["id"] = rs.ID
    }
    if rs.Name != nil {
        objectMap["name"] = rs.Name
    }
    if rs.Type != nil {
        objectMap["type"] = rs.Type
    }
    if rs.Etag != nil {
        objectMap["etag"] = rs.Etag
    }
    if rs.RecordSetProperties != nil {
        objectMap["properties"] = rs.RecordSetProperties
    }
    return json.Marshal(objectMap)
}
kirankumarbv commented 6 years ago

I apologize for the late reply. Yes it works with the changes you suggested.

vladbarosan commented 6 years ago

@kirankumarbv we will be releasing a fix soon.

kirankumarbv commented 6 years ago

@vladbarosan Thanks

tombuildsstuff commented 6 years ago

@vladbarosan any idea when this fix will be available? Looking at the HTTP responses it appears this is due to the properties block being omited.

Here's the Go SDK code we're using:

parameters := dns.RecordSet{
    Name: &name,
    RecordSetProperties: &dns.RecordSetProperties{
        Metadata: expandTags(tags),
        TTL:      &ttl,
        ARecords: &records,
    },
}

eTag := ""
ifNoneMatch := "" // set to empty to allow updates to records after creation
resp, err := dnsClient.CreateOrUpdate(ctx, resGroup, zoneName, name, dns.A, parameters, eTag, ifNoneMatch)
if err != nil {
    return err
}

and the Request sent using v12.5.0-beta of the Azure SDK / v9.7.0 of go-autorest:

{
    "name": "internal",
    "properties": {
        "metadata": {
            "environment": "staging"
        },
        "TTL": 300,
        "ARecords": [{
            "ipv4Address": "1.2.4.5"
        }, {
            "ipv4Address": "1.2.3.4"
        }]
    }
}

and the Request sent via SDK v14.5.0 / go-autorest v10.2:

{
    "ARecords": [{
        "ipv4Address": "1.2.4.5"
    }, {
        "ipv4Address": "1.2.3.4"
    }],
    "TTL": 300,
    "metadata": {
        "environment": "staging"
    }
}

This behaviour seems to affect both the 2016-04-01 and the 2017-09-01 API endpoints, FWIW.

vladbarosan commented 6 years ago

Hi @tombuildsstuff in 14.5 we introduced a new API version for DNS which has the fix. ( https://github.com/Azure/azure-sdk-for-go/tree/master/services/preview/dns/mgmt/2018-03-01-preview/dns)

Does it work for you to use the new API version ?

tombuildsstuff commented 6 years ago

@vladbarosan cool, thanks for confirming that, it does - is there a timeline to roll this fix out to the stable API versions too? I'd rather we didn't roll out a Preview API unless we need something contained within the Preview version if possible.

Thanks!

jhendrixMSFT commented 6 years ago

@tombuildsstuff Unfortunately there were a few breaking changes in the DNS swaggers so this will have to wait for the v15 release which is currently planned for next week. Does that work for you?

tombuildsstuff commented 6 years ago

@jhendrixMSFT we're happy to wait for it, but given this SDK is now GA (and this functionality is currently broken) I think we could probably include a breaking change in a new version (e.g. v14.6.1) using the existing Swagger file?

jhendrixMSFT commented 6 years ago

@tombuildsstuff So this is something we've discussed internally, i.e. is introducing a breaking change that fixes a broken package really a breaking change. We've been leaning towards not counting it as a breaking change, the only problem is there is no way for us to automatically differentiate the two cases (we're working on tooling to automatically flow non-breaking changes from latest into master). Perhaps though since this case is rare it doesn't really matter and we can flow such changes by hand as required. If you folks are ok with introducing breaking changes in a patch revision to fix a broken package then we can probably go with that (I will probably run this by our partners in kubernates as well so we're all on the same page).

tombuildsstuff commented 6 years ago

@jhendrixMSFT so I'm fine with that in a minor version, but I can understand why others could not be :)

From our side, at this point we've got 4 blockers for upgrading to SDKv14: https://github.com/terraform-providers/terraform-provider-azurerm/pull/1006. As such I don't believe we'll get to upgrading until next week either way, if it helps?

jhendrixMSFT commented 6 years ago

Fixed in https://github.com/Azure/azure-sdk-for-go/releases/tag/v15.0.0