common icon indicating copy to clipboard operation
common copied to clipboard

Unmarshalling null ProxyURL

Open OlivierCazade opened this issue 3 years ago • 3 comments

When unmarshalling a ProxyURL field set to null, it get unmarshalled to an empty URL.

Contrary to a null pointer, an empty string will be handled by the http package like a valid Proxy, resulting in some connection error. https://pkg.go.dev/net/http#Transport

This PR change empty ProxyURL to nul URL during the validate phase of unmarshalling.

OlivierCazade avatar Jul 20 '22 17:07 OlivierCazade

Is this related to json unmarshalling?

roidelapluie avatar Aug 03 '22 10:08 roidelapluie

Yes, when marshalling to json an httpconfig and then unmarshalling it, the proxyURL move from beeing nil to an empty string.

Here is a code snippet:

package main

import (
	"encoding/json"
	"fmt"

	promConfig "github.com/prometheus/common/config"
)

func main() {
	config := promConfig.HTTPClientConfig{}
	bs, _ := json.Marshal(config)

	config2 := promConfig.HTTPClientConfig{}
	json.Unmarshal(bs, &config2)
	fmt.Println(config2)
}

In the case of the HTTPClientConfig, this change the behaviour, the proxyURL used here : https://github.com/prometheus/common/blob/148258742722faf27ecdafc27da93b1e29efe4b0/config/http_config.go#L464 An empty string will be considered as a valid URL while a nil pointer will be ignored.

I first wanted to submit a patch to the URL unmarshall function, but it looked likes this was done this way intentionnaly: https://github.com/prometheus/common/commit/558a7872666f119bab26572eb9fb40422c8dc83f

  • the marshalling function explicitly marshall to json
  • unit tests explicitly check that a null URL is unmarshalled to an empty string

So, I assumed this was used somewhere else, and I submited this patch instead to only affect HTTPClientConfig.

OlivierCazade avatar Aug 05 '22 10:08 OlivierCazade

Can you add tests for this? Or maybe we need to implement marshaljson properly

roidelapluie avatar Nov 03 '22 14:11 roidelapluie