splunk / terraform-provider-splunk

Terraform Provider for Splunk
Mozilla Public License 2.0
104 stars 78 forks source link

Fix url encoding for searchers #29

Closed zscholl closed 3 years ago

zscholl commented 4 years ago

Addresses issue #26 by skipping URL encoding for search titles when building the splunk URL.

What is happening when there are special characters in a search's name, like [ or ] is that BuildSplunkUrl calls PathEscape on the search title. This encodes any special characters, so [ becomes %5B. The problem is that Url.Path gets encoded again before a final request is sent so the final URL requested becomes %255B which decodes to %5B instead of [.

Here is a great example: https://stackoverflow.com/a/63626359

I attempted to avoid this by skipping the PathEscape, but that caused errors in the tcpInputs/outputs tests. I also attempted to use the RawPath and Path pieces of Url to include the encoded and non-encoded parts, respectfully. This caused the same errors as skipping encoding.

I opted to fix this by specifically skipping the encoding when we build URLs for specific searches. This feels a bit hacky so I'm open to better suggestions, but it works.

I added some tests, both for the BuildSplunkUrl method and the acceptance tests.

anushjay commented 3 years ago

Hi @zscholl Thanks for attempting to fix this. I remember running into similar issues while writing the client. Now as I think more about it, I feel it's better to keep the build URL function simple and remove the encoding because like you pointed out Url.Path performs encoding.

func (c *Client) BuildSplunkURL(queryValues url.Values, urlPathParts ...string) url.URL {
    buildPath := ""
    for _, pathPart := range urlPathParts {
        pathPart = strings.ReplaceAll(pathPart, " ", "+") // url parameters cannot have spaces
        **buildPath = path.Join(buildPath, pathPart)**
    }

And take care of encoding in the individual resource CRUD functions, for instance in inputs_monitor

func (client *Client) ReadMonitorInput(name, owner, app string) (*http.Response, error) {
    **endpoint := client.BuildSplunkURL(nil, "servicesNS", owner, app, "data", "inputs", "monitor", url.PathEscape(name))**
    resp, err := client.Get(endpoint)
    if err != nil {
        return nil, err
    }

    return resp, nil
} 

while doing so also change the name in the acl requests for the specific resources like resources_splunk_inputs_monitor **err := (*provider.Client).UpdateAcl(aclObject.Owner, aclObject.App, url.PathEscape(name), aclObject, "data", "inputs", "monitor")**

What do you think about the fix? I can work on this if you and try to get a version released if you don't have cycles to work on the fix. And apologies for not getting to this earlier.

zscholl commented 3 years ago

Hey @anushjay, no worries!

Thanks for the insight I had not thought of that but I think it makes a lot more sense. I implemented the changes you suggested. Let me know how it looks!

anushjay commented 3 years ago

@zscholl Just left one comment. LGTM to otherwise. I can merge it after you're done with those changes. Nice work!