Skip to content

Conversation

labuladong
Copy link
Contributor

@labuladong labuladong commented Jun 26, 2024

Even #1228 fix the double escape error, there still some issue about custom dynamic config.

When the custom config value has /, the golang net/http package will unescape it by default. see golang/go#7356

For example, if the config value is a json like {"key": "https://example.com/"}, the expected request should be:

/admin/v2/brokers/configuration/someConfig/%7B%22key%22%3A%20%22https%3A%2F%2Fexample.com%2F%22%7D

But the actual request will be this:

/admin/v2/brokers/configuration/someConfig/{"key": "https://example.com/"}

And it will causes 404 not found because / in the URL.

If you escape the value twice, it will be:

/admin/v2/brokers/configuration/someConfig/%257B%2522key%2522%253A%2520%2522https%253A%252F%252Fexample.com%252F%2522%257D

The solution is url.RawPath field in this golang/go@874a605 , we need to modify this part to add the RawPath field:

func (c *Client) newRequest(method, path string) (*request, error) {
base, err := url.Parse(c.ServiceURL)
if err != nil {
return nil, err
}
u, err := url.Parse(path)
if err != nil {
return nil, err
}
req := &request{
method: method,
url: &url.URL{
Scheme: base.Scheme,
User: base.User,
Host: base.Host,
Path: endpoint(base.Path, u.Path),
},
params: make(url.Values),
}
return req, nil
}

But the newRequest is used by many other methods, I don't want to touch it. So an easy solution is to provide a MakeRequestWithURL method, users can use it to customize.

@labuladong labuladong force-pushed the donglai/fix/dynamic-config2 branch from 5c81643 to 6eaac76 Compare June 26, 2024 05:20
@labuladong labuladong force-pushed the donglai/fix/dynamic-config2 branch from 6eaac76 to bfe57d5 Compare June 26, 2024 05:31
@labuladong labuladong force-pushed the donglai/fix/dynamic-config2 branch from d3cf71c to 597be31 Compare June 26, 2024 05:36
@labuladong labuladong requested a review from tuteng June 26, 2024 06:23
@tuteng tuteng merged commit 552b541 into apache:master Jun 26, 2024
@tuteng
Copy link
Member

tuteng commented Jun 26, 2024

Sorry, I thought it was approved after the approve run test

image

@tuteng
Copy link
Member

tuteng commented Jun 26, 2024

This pr LGTM

@RobertIndie RobertIndie added this to the v0.13.0 milestone Jun 27, 2024
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.

3 participants