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.

Sign up for GitHub

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

HPCC-30468 Use service hostname for external dafilesrv if available #17888

Merged
merged 1 commit into from
Oct 12, 2023
+61 −24
Merged

HPCC-30468 Use service hostname for external dafilesrv if available #17888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions dali/base/dautils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3612,8 +3612,10 @@ void addStripeDirectory(StringBuffer &out, const char *directory, const char *pl
}
}

static CConfigUpdateHook directIOUpdateHook;
static CriticalSection dafileSrvNodeCS;
static Owned<INode> dafileSrvNode;

void remapGroupsToDafilesrv(IPropertyTree *file, INamedGroupStore *resolver)
{
FileDescriptorFlags fileFlags = static_cast<FileDescriptorFlags>(file->getPropInt("Attr/@flags"));
Expand All @@ -3625,15 +3627,14 @@ void remapGroupsToDafilesrv(IPropertyTree *file, INamedGroupStore *resolver)
Owned<IStoragePlane> plane = getDataStoragePlane(planeName, true);
if ((0 == plane->queryHosts().size()) && isAbsolutePath(plane->queryPrefix())) // if hosts group, or url, don't touch
{
auto updateFunc = [&](const IPropertyTree *oldComponentConfiguration, const IPropertyTree *oldGlobalConfiguration)
{
CriticalBlock b(dafileSrvNodeCS);
if (nullptr == dafileSrvNode)
{
auto externalService = k8s::getDafileServiceFromConfig("directio");
VStringBuffer dafilesrvEpStr("%s:%u", externalService.first.c_str(), externalService.second);
dafileSrvNode.setown(createINode(dafilesrvEpStr));
}
}
auto externalService = k8s::getDafileServiceFromConfig("directio");
VStringBuffer dafilesrvEpStr("%s:%u", externalService.first.c_str(), externalService.second);
dafileSrvNode.setown(createINode(dafilesrvEpStr));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accessing dafileSrvNode later on in the pushback needs to (and the createIGroup) needs to be done in a critical section in case it changes inside the call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I'll fix.

};
directIOUpdateHook.installOnce(updateFunc, true);

Owned<IGroup> group;
if (cluster.hasProp("Group"))
Expand All @@ -3646,9 +3647,15 @@ void remapGroupsToDafilesrv(IPropertyTree *file, INamedGroupStore *resolver)
group.setown(resolver->lookup(planeName, defaultDir, groupType));
}

Linked<INode> dafileSrvNodeCopy;
{
// in case config hook above changes dafileSrvNode
CriticalBlock b(dafileSrvNodeCS);
dafileSrvNodeCopy.set(dafileSrvNode);
}
std::vector<INode *> nodes;
for (unsigned n=0; n<group->ordinality(); n++)
nodes.push_back(dafileSrvNode);
nodes.push_back(dafileSrvNodeCopy);
Owned<IGroup> newGroup = createIGroup((rank_t)group->ordinality(), &nodes[0]);
StringBuffer groupText;
newGroup->getText(groupText);
Expand Down
26 changes: 17 additions & 9 deletions esp/clients/ws_dfsclient/ws_dfsclient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,9 @@ class CServiceDistributedFile : public CServiceDistributedFileBase<IDistributedF
if (dafileSrvRemoteFilePlane)
{
file->setPropTree("Attr/_remoteStoragePlane", createPTreeFromIPT(dafileSrvRemoteFilePlane));
if (remoteStorage->hasProp("@secret"))

const char *serviceUrl = remoteStorage->queryProp("@service");
if (serviceUrl && startsWith(serviceUrl, "https"))
{
// if remote storage service is secure, dafilesrv connections must be also.
// this flag is used by consumers of this IFleDescriptor to tell whether they need to make
Expand Down Expand Up @@ -673,7 +675,10 @@ IDFSFile *lookupDFSFile(const char *logicalName, AccessMode accessMode, unsigned
if (!remoteStorage)
throw makeStringExceptionV(0, "Remote storage '%s' not found", remoteName.str());
serviceUrl.set(remoteStorage->queryProp("@service"));

// NB: for legacy support only, if the service url is secure, a secret name will be auto-generated
serviceSecret.set(remoteStorage->queryProp("@secret"));

logicalName = remoteLogicalFileName;
useDafilesrv = remoteStorage->getPropBool("@useDafilesrv");
}
Expand All @@ -683,20 +688,18 @@ IDFSFile *lookupDFSFile(const char *logicalName, AccessMode accessMode, unsigned
// auto-discover local environment dfs service.
#ifdef _CONTAINERIZED
// NB: only expected to be here if experimental option #option('dfsesp-localfiles', true); is in use.
// This finds and uses local dfs service for local read lookukup.
// This finds and uses local dfs service for local read lookups.
Owned<IPropertyTreeIterator> eclWatchServices = getGlobalConfigSP()->getElements("services[@type='dfs']");
if (!eclWatchServices->first())
throw makeStringException(-1, "Dfs service not defined in esp services");
const IPropertyTree &eclWatch = eclWatchServices->query();
StringBuffer eclWatchName;
eclWatch.getProp("@name", eclWatchName);
auto result = k8s::getExternalService(eclWatchName);
if (result.first.empty())
throw makeStringExceptionV(-1, "dfs '%s': service not found", eclWatchName.str());
if (0 == result.second)
throw makeStringExceptionV(-1, "dfs '%s': service port not defined", eclWatchName.str());
const char *protocol = eclWatch.getPropBool("@tls") ? "https" : "http";
serviceUrl.appendf("%s://%s:%u", protocol, result.first.c_str(), result.second);
unsigned port = (unsigned)eclWatch.getPropInt("@port", NotFound);
if (NotFound == port)
throw makeStringExceptionV(-1, "dfs '%s': service port not defined", eclWatchName.str());
serviceUrl.appendf("%s://%s:%u", protocol, eclWatchName.str(), port);
#else
{
CriticalBlock b(dfsServiceUrlCrit);
Expand All @@ -714,7 +717,12 @@ IDFSFile *lookupDFSFile(const char *logicalName, AccessMode accessMode, unsigned
#endif
}
bool useSSL = startsWith(serviceUrl, "https");
if (!useSSL)
if (useSSL)
{
if (0 == serviceSecret.length())
generateDynamicUrlSecretName(serviceSecret, serviceUrl, nullptr);
}
else
serviceSecret.clear();

DBGLOG("Looking up file '%s' on '%s'", logicalName, serviceUrl.str());
Expand Down
11 changes: 11 additions & 0 deletions helm/hpcc/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1140,6 +1140,16 @@ Generate service entries for TLS
{{- end }}
{{- end }}

{{/*
Conditionally generate annotated k8s hostname
*/}}
{{- define "hpcc.outputHostname" -}}
{{- $annotations := .annotations | default dict -}}
{{- if hasKey $annotations "external-dns.alpha.kubernetes.io/hostname" -}}
hostname: {{ index $annotations "external-dns.alpha.kubernetes.io/hostname" }}
{{- end -}}
{{- end -}}

{{/*
Generate list of available services
*/}}
Expand Down Expand Up @@ -1209,6 +1219,7 @@ Generate list of available services
port: {{ .service.servicePort | default 7600 }}
public: {{ (ne ( include "hpcc.isVisibilityPublic" (dict "root" $ "visibility" .service.visibility)) "") | ternary "true" "false" }}
{{- include "hpcc.addTLSServiceEntries" (dict "root" $ "service" $dafilesrv.service "component" $dafilesrv "visibility" $dafilesrv.service.visibility) }}
{{- include "hpcc.outputHostname" .service | nindent 2 }}
{{ end -}}
{{ end -}}
{{- end -}}
Expand Down
22 changes: 16 additions & 6 deletions system/jlib/jcontainerized.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,12 +379,22 @@ std::pair<std::string, unsigned> getDafileServiceFromConfig(const char *applicat
throw makeStringExceptionV(JLIBERR_K8sServiceError, "dafilesrv service '%s' has no public service defined", application);
StringBuffer dafilesrvName;
dafilesrv.getProp("@name", dafilesrvName);
auto externalService = getExternalService(dafilesrvName);
if (externalService.first.empty())
throw makeStringExceptionV(JLIBERR_K8sServiceError, "dafilesrv service '%s' - external service '%s' not found", application, dafilesrvName.str());
if (0 == externalService.second)
throw makeStringExceptionV(JLIBERR_K8sServiceError, "dafilesrv service '%s' - external service '%s' port not defined", application, dafilesrvName.str());
return externalService;
unsigned port = (unsigned)dafilesrv.getPropInt("@port");

StringBuffer hostname;
dafilesrv.getProp("@hostname", hostname);
if (hostname.length())
return { hostname.str(), port };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat. I never knew that....

else
{
auto externalService = getExternalService(dafilesrvName);
if (externalService.first.empty())
throw makeStringExceptionV(JLIBERR_K8sServiceError, "dafilesrv service '%s' - external service '%s' not found", application, dafilesrvName.str());
if (0 == externalService.second)
throw makeStringExceptionV(JLIBERR_K8sServiceError, "dafilesrv service '%s' - external service '%s' port not defined", application, dafilesrvName.str());
assertex(port == externalService.second);
return externalService;
}
}


Expand Down
3 changes: 2 additions & 1 deletion system/jlib/jsecrets.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,8 @@ static void splitUrlSections(const char *url, const char * &authority, size_t &a
else
{
authorityLen = path-url;
fullpath.append(path);
if (!streq(path, "/")) // treat empty trailing path as equal to no path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, agreed.

fullpath.append(path);
}
}

Expand Down
Loading