ionos-cloud / external-dns-ionos-webhook

External-DNS Webhook to manage IONOS DNS Records
https://cloud.ionos.de
Apache License 2.0
17 stars 2 forks source link

Wrong check on Content Type header #10

Closed Raffo closed 1 year ago

Raffo commented 1 year ago

Hi, I'm @Raffo, the author of the ExternalDNS plugin provider. I want to thank you again for giving me access to this plugin and to IONOS to test it out.

I have tried to adapt my end to end testing framework to include this plugin and I hit a problem which I think is a bug in the code.

The docs of the plugin provider say:

Additionally, the server needs to respond to GET requests on / to negotiate versions by content type as described in this document. The server needs to respond to those requests by reading the Accept header and responding with a corresponding Vary header including the value Content-Type and a Content-Type header specifying the supported media type format and version.

But your code is checking this: https://github.com/ionos-cloud/external-dns-ionos-plugin/blob/main/pkg/plugin/plugin.go#L199

func (p *Plugin) ApplyChanges(w http.ResponseWriter, r *http.Request) {
    if p.hasContentHeader(w, r) {

The content type should be set by the "server" which in this case is the IONOS provider. ExternalDNS is not setting any Content Type for all requests. I'm following those instructions for the logic: http://opensource.zalando.com/restful-api-guidelines/#114

I did instead add a commit that adds the accept header to all requests even though I originally had it only for the negotiation phase. I think it makes sense and this plugin helped me spot that problem.

Let me know what you think of this and if it makes sense.

Thanks!

/cc @akrieg-ionos @mspoeri

akrieg-ionos commented 1 year ago

Thanks, for testing and giving your feedback! With this comment the api content negotiation has been specified, and release 0.3.0 is implementing this.