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

Unmarshalling null ProxyURL #394

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

OlivierCazade
Copy link

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.

Signed-off-by: Olivier Cazade <ocazade@redhat.com>
@roidelapluie
Copy link
Member

Is this related to json unmarshalling?

@OlivierCazade
Copy link
Author

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 :

Proxy: http.ProxyURL(cfg.ProxyURL.URL),

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:
558a787

  • 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.

@roidelapluie
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants