HPCC-29981 Improve gateway support in ESDL script #17642
Conversation
|
https://track.hpccsystems.com/browse/HPCC-29981 |
| static constexpr const char* gwLocalSecretPrefix = "local-secret:"; | ||
| static constexpr const size_t gwLocalSecretPrefixLength = strlen(gwLocalSecretPrefix); | ||
| static constexpr const char* gwPassThroughPrefix = "pass-through:"; | ||
| static constexpr const size_t gwPassThroughPrefixLength = strlen(gwPassThroughPrefix); |
There was a problem hiding this comment.
If you haven't already, would be good to open a ticket to document this new behavior so we don't forget to do it.
There was a problem hiding this comment.
The possible existence of Method/Gateways is included in platform documentation. I will wait until the full extent of changes is known before submitting a ticket for the documentation team.
There was a problem hiding this comment.
Ok, I was thinking just for our developer ESDL scripting docs, but either way.
| const char* name = gateway.queryProp("@name"); | ||
| StringBuffer url(gateway.queryProp("@url")); | ||
| if (url.isEmpty()) | ||
| throw makeStringExceptionV(-1, "gateway %s: missing url property", name); |
There was a problem hiding this comment.
Maybe check to ensure we have a @name - unless that is already previously guaranteed.
There was a problem hiding this comment.
You commented on @name twice. I'll address here. There is a logical requirement for @name. In the legacy transform, @name becomes @ServiceName (see lines 1477 and 1479). There is a need for something that the roxie can use to select the gateway and we've assumed for years it is called @name.
That's not to say that nothing should change. What should the absence of @name mean? We've been assuming its presence for legacy transforms. Absence here, and at 1414, only affects error reporting. Should we default to something like "unknown" for error reporting, because we don't know there isn't another naming convention in use, but fail when we try to act on it?
There was a problem hiding this comment.
I was only thinking in terms of the local effect of calling makeStringExceptionV with a possible null pointer. If it seems overkill to have more checks then you should leave as-is.
| throw makeStringExceptionV(-1, "gateway %s: '%s' does not identify an 'esp' category secret", name, identification); | ||
| if (!isEmptyString(permission) && !secret->getPropBool(permission)) | ||
| throw makeStringExceptionV(-1, "gateway %s: '%s' does not grant '%s' permission", name, identification, permission); | ||
| if (!secret->getProp("url", url.clear()) || url.isEmpty()) |
There was a problem hiding this comment.
Won't the url always get cleared by the first clause, resulting in the second clause returning true even if it was originally populated before the first clause?
There was a problem hiding this comment.
The url will always get cleared to prevent getProp from appending the secret's defined URL to the configurations secret identification . The second clause is a sanity check that the secret didn't define an empty URL. I think the logic is as intended.
There was a problem hiding this comment.
Ok I guess I misunderstood. I thought there could be a valid case here where it wasn't required to resolve the secret.
There was a problem hiding this comment.
@timothyklemm generally looks fine. Some comments about naming, syntax, etc.
| url.clear(); | ||
| url.append(scheme); | ||
| url.append("://"); | ||
| if (userinfo.length()>0 ) |
There was a problem hiding this comment.
Extra space. I also think the >0 on many of these ifs is a bit superfluous. if (userinfo.length()) would suffice.
| url.appendf("%s@", userinfo.str()); | ||
| url.append(host); | ||
| if (port.length() > 0) | ||
| url.appendf(":%s", port.str()); |
There was a problem hiding this comment.
url.append(':').append(port);
might be slightly faster.
| throw makeStringExceptionV(-1, "gateway %s: '%s' is not of the form \"[ vault-id ':' ] secret-name\"", name, identification); | ||
| } | ||
| if (!secret) | ||
| throw makeStringExceptionV(-1, "gateway %s: '%s' does not identify an 'esp' category secret", name, identification); |
There was a problem hiding this comment.
Wording might be slightly confusing. Mostly in the sense that I would say the secret wasn't found rather than "doesn't identify". But the rest including mentioning the category is good.
| throw makeStringExceptionV(-1, "gateway %s: missing url property", name); | ||
| if (strncmp(url, gwLocalSecretPrefix, gwLocalSecretPrefixLength)) | ||
| throw makeStringExceptionV(-1, "gateway %s: expected '%s...'; got '%s'", name, gwLocalSecretPrefix, url.str()); | ||
| const char* identification = url.str() + gwLocalSecretPrefixLength; |
There was a problem hiding this comment.
For ECL I currently require connection secrets to start with "http-connect-". We should consider doing the same here. It means the secrets were explictly created for this purpose. I may regret it in the future, who knows, but it gives a little control and forced standardization.
| tgtcfg.setown(createPTreeFromIPT(tgtcfg)); | ||
| const char* qt = tgtcfg->queryProp("@querytype"); | ||
| bool published = (qt && (strieq(qt, "roxie") || strieq(qt, "wsecl"))); | ||
| const char* permission = (published ? "allowPublishedGatewayUsage" : "allowUnpublishedGatewayUsage"); |
There was a problem hiding this comment.
Just because permission is used in so many overloaded ways I wonder if something like the term "requiredUsage" would work instead?
| if (!secret->getProp("url", url.clear()) || url.isEmpty()) | ||
| throw makeStringExceptionV(-1, "gateway %s: '%s' missing required 'url' property; credential-only secrets not supported", name, identification); | ||
| const char* username = secret->queryProp("username"); | ||
| if (isEmptyString(username) && !secret->getPropBool("insecure")) |
There was a problem hiding this comment.
The term insecure is a bit ambiguous. It makes me think of things like do we accept self signed certs or not verify the server.. some of those types of options may be needed for dev secrets in the future.
Can we call it something else?
| gateway.setProp("@url", url); | ||
| } | ||
|
|
||
| void EsdlServiceImpl::adjustURL(StringBuffer& url, const char* username, const char* password) const |
There was a problem hiding this comment.
Can we put something in the method name about adding credentials? adjustURL is a bit ambiguous.
timothyklemm
force-pushed
the
hpcc-29981-gateway_transformations
branch
from
b572b4a to
724eecb
Compare
- Add a transactional secret cache to the script context. - Add secret support to http-post-xml. - Update secret support in mysql. Signed-off-by: Tim Klemm <[email protected]>
- Add support for gateway URLs that are references to secrets that can be resolved by the query target. - Add support for gateway URLs that are references to secrets that can be resolved by the ESP. Only url, username, and password secret properties are used to assemble a replacement URL for the gateway. Each secret must explicitly enable compatibility with this use case. - Add support for explicit pass-through values not interpreted by the ESP. - Ensure inline URLs can be updated with separately configured username and password values. These updates must be enabled by defining either Gateways/@resolveInlineURLs as true or, for backward compatibility, Gateways/@legacyTransformTarget as a non-empty value. Signed-off-by: Tim Klemm <[email protected]>
timothyklemm
force-pushed
the
hpcc-29981-gateway_transformations
branch
from
072f211 to
512cc33
Compare
Type of change:
Checklist:
Smoketest:
Testing: