Remove an incomplete workaround for database connection_url formatting #22224
Conversation
Instead fix the underlying issue of `dbutil.QueryHelper` making multiple passes over the input string, and potentially reprocessing placeholders inserted as a result of previous substitutions. Follow-up to hashicorp#13414
| @@ -69,7 +69,7 @@ func (c *mySQLConnectionProducer) Init(ctx context.Context, conf map[string]inte | |||
| password := c.Password | |||
|
|
|||
| // QueryHelper doesn't do any SQL escaping, but if it starts to do so | |||
| // then maybe we won't be able to use it to do URL substitution any more. | |||
| // then maybe we won't be able to use it to do URL substitution anymore. | |||
There was a problem hiding this comment.
Grammar fix. Also, note that this is a forked copy of the sdk's SQLConnectionProducer that did not contain the workaround.
| strings.Contains(c.Password, "{{password}}") { | ||
|
|
||
| return nil, fmt.Errorf("username and/or password cannot contain the template variables") | ||
| } |
There was a problem hiding this comment.
This is the workaround being removed.
| // Don't escape special characters for the username and password if the | ||
| // disable_escaping parameter is set to true. A user does this if the | ||
| // connection_url is actually not a URL, but database-specific other | ||
| // format, typically space or semicolon separated key=value pairs. |
There was a problem hiding this comment.
Add clarity to comment on why disable_escaping exists.
| username := c.Username | ||
| password := c.Password | ||
| if !c.DisableEscaping { | ||
| username = url.PathEscape(c.Username) | ||
| } | ||
| if (c.Type != "mysql") && !c.DisableEscaping { |
There was a problem hiding this comment.
This mysql special case can be deleted - the mysql plugin is no longer using this code at all. As can be seen in the first file touched in this PR, it now has its own forked copy.
| @@ -22,6 +20,9 @@ func TestSQLPasswordChars(t *testing.T) { | |||
| {"postgres", "pass/word"}, | |||
| {"postgres", "p@ssword"}, | |||
| {"postgres", "pass\"word\""}, | |||
| // Much to my surprise, CREATE USER "{{password}}" PASSWORD 'foo' worked. | |||
| {"{{password}}", "foo"}, | |||
| {"user", "{{username}}"}, | |||
There was a problem hiding this comment.
Re-add test cases removed in #13414.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove test that tests the workaround being removed.
| assert.Equal(t, tc.expected, QueryHelper(tc.tpl, data), | ||
| "template processing produced unexpected result") | ||
| } | ||
| } |
There was a problem hiding this comment.
This test passes before and after the change.
| assert.Equal(t, "A{{a}}{{b}}{{c}}{{d}}B{{a}}{{b}}{{c}}{{d}}C{{a}}{{b}}{{c}}{{d}}D{{a}}{{b}}{{c}}{{d}}", | ||
| QueryHelper("{{a}}{{b}}{{c}}{{d}}", data), | ||
| "template processing produced unexpected result") | ||
| } |
There was a problem hiding this comment.
This test fails when applied to the existing implementation of QueryHelper - it showcases the issues just discussed at the end of #13414.
| for k, v := range data { | ||
| tpl = strings.ReplaceAll(tpl, fmt.Sprintf("{{%s}}", k), v) | ||
| } | ||
| var queryHelperRegex = regexp.MustCompile(`{{[^{}]+}}`) |
There was a problem hiding this comment.
I don't have the original context so I won't do a full review, but if we do go with a regex I think we should be extremely prescriptive with it to more precisely match what we're looking for, e.g. here's a little Go playground test case for a key that has a } in it (which probably isn't valid for other reasons, but it's just to illustrate the point) https://go.dev/play/p/qUXqc397xkY.
So I'd rather see a regex that specifically looks for the keys we know how to replace, e.g.
fmt.Sprintf(`{{(%s)}}`, strings.Join(keys, "|"))There was a problem hiding this comment.
The major point of this change is to do only one pass over the input string, not one per replaceable parameter, so the regex needs to stay as is.
However, I recognise I should make it clearer in the documentation that replacement names may not contain { or } themselves.
EDIT: Oops, responding from phone, and half the suggested code was off screen... I'll have a think about it. I'm not sure this is a case we really need to care about though.
There was a problem hiding this comment.
Having thought on this some more, I am of the opinion that we should not make this change, because it makes the code more complex (and less efficient, though this is not a particularly a hotspot), only to support an edge case of placeholder keys containing { or } which they realistically never will.
The keys here are all specified by the coders of plugins and never by users, and it would be mad to define a {{xxx}} substitution that contained internal { or } as part of the xxx - it would be a supremely confusing UI.
Also, if we dynamically build the regex, we then need to account for quoting regex metacharacters in key names - the . character could plausibly appear in a future key, for example.
I do, though, see that I should document the limitation of not using { } within substitution keys, and add an example of it to the tests.
Would that be an acceptable way to proceed?
|
I'll just close this out ... HashiCorp aren't interested, and I no longer use Vault, and am discouraged from doing so again in the future by the license change. |
Instead fix the underlying issue of
dbutil.QueryHelpermaking multiple passes over the input string, and potentially reprocessing placeholders inserted as a result of previous substitutions.Follow-up to #13414