hpcc-systems · ghalliday · Nov 23, 2023 · Nov 10, 2023
diff --git a/dali/base/dadfs.cpp b/dali/base/dadfs.cpp
@@ -11156,6 +11156,9 @@ class CDaliDFSServer: public Thread, public CTransactionLogTracker, implements I
         if (queryTransactionLogging())
             transactionLog.log("%s", trc.str());
 
+        // This is Dali, and for foreign access (see below), if in use, this is likely to be false
+        bool secureService = getComponentConfigSP()->getPropBool("@tls");
+
         mb.clear();
         CDfsLogicalFileName dlfn;
         dlfn.set(lname);
@@ -11181,7 +11184,7 @@ class CDaliDFSServer: public Thread, public CTransactionLogTracker, implements I
                         if (hasMask(opts, GetFileTreeOpts::remapToService))
                         {
                             tree.setown(createPTreeFromIPT(tree)); // copy live Dali tree, because it is about to be altered by remapGroupsToDafilesrv
-                            remapGroupsToDafilesrv(tree, &queryNamedGroupStore());
+                            remapGroupsToDafilesrv(tree, true, secureService);
                             groupResolver = nullptr; // do not attempt to resolve remapped group (it will not exist and cause addUnique to create a new anon one)
 
                             const char *remotePlaneName = tree->queryProp("@group");
@@ -11231,7 +11234,7 @@ class CDaliDFSServer: public Thread, public CTransactionLogTracker, implements I
                             if (getComponentConfigSP()->getPropBool("@foreignAccess"))
                             {
                                 tree.setown(createPTreeFromIPT(tree)); // copy live Dali tree, because it is about to be altered by remapGroupsToDafilesrv
-                                remapGroupsToDafilesrv(tree, &queryNamedGroupStore());
+                                remapGroupsToDafilesrv(tree, true, secureService);
                                 groupResolver = nullptr; // do not attempt to resolve remapped group (it will not exist and cause addUnique to create a new anon one)
                             }
                         }

diff --git a/dali/base/dautils.cpp b/dali/base/dautils.cpp
@@ -3642,11 +3642,10 @@ void addStripeDirectory(StringBuffer &out, const char *directory, const char *pl
 
 static CConfigUpdateHook directIOUpdateHook;
 static CriticalSection dafileSrvNodeCS;
-static Owned<INode> dafileSrvNode;
+static Owned<INode> tlsDirectIONode, nonTlsDirectIONode;
 
-void remapGroupsToDafilesrv(IPropertyTree *file, INamedGroupStore *resolver)
+void remapGroupsToDafilesrv(IPropertyTree *file, bool foreign, bool secure)
 {
-    FileDescriptorFlags fileFlags = static_cast<FileDescriptorFlags>(file->getPropInt("Attr/@flags"));
     Owned<IPropertyTreeIterator> iter = file->getElements("Cluster");
     ForEach(*iter)
     {
@@ -3659,10 +3658,24 @@ void remapGroupsToDafilesrv(IPropertyTree *file, INamedGroupStore *resolver)
             {
                 auto updateFunc = [&](const IPropertyTree *oldComponentConfiguration, const IPropertyTree *oldGlobalConfiguration)
                 {
-                    CriticalBlock b(dafileSrvNodeCS);
-                    auto externalService = k8s::getDafileServiceFromConfig("directio");
-                    VStringBuffer dafilesrvEpStr("%s:%u", externalService.first.c_str(), externalService.second);
-                    dafileSrvNode.setown(createINode(dafilesrvEpStr));
+                    auto resolve = [&](bool secure) -> INode *
+                    {
+                        auto directioService = k8s::getDafileServiceFromConfig("directio", secure, false);
+                        if (0 == directioService.second) // port. If 0, getDafileServiceFromConfig did not find a match
+                            return nullptr;
+                        VStringBuffer dafilesrvEpStr("%s:%u", directioService.first.c_str(), directioService.second);
+                        const char *typeText = secure ? "secure" : "non-secure";
+                        Owned<INode> directIONode = createINode(dafilesrvEpStr);
+                        if (directIONode->endpoint().isNull())
+                            throw makeStringExceptionV(0, "Unable to resolve %s directio dafilesrv hostname '%s'", typeText, directioService.first.c_str());
+                        PROGLOG("%s directio = %s", typeText, dafilesrvEpStr.str());
+                        return directIONode.getClear();
+                    };
+                    {
+                        CriticalBlock b(dafileSrvNodeCS);
+                        tlsDirectIONode.setown(resolve(true));
+                        nonTlsDirectIONode.setown(resolve(false));
+                    }
                 };
                 directIOUpdateHook.installOnce(updateFunc, true);
             }
@@ -3672,20 +3685,26 @@ void remapGroupsToDafilesrv(IPropertyTree *file, INamedGroupStore *resolver)
                 group.setown(createIGroup(cluster.queryProp("Group")));
             else
             {
-                assertex(resolver);
+                // JCSMORE only expected here if via foreign access (not entirely sure if this route is ever possible anymore)
+                assertex(foreign);
                 StringBuffer defaultDir;
                 GroupType groupType;
-                group.setown(resolver->lookup(planeName, defaultDir, groupType));
+                group.setown(queryNamedGroupStore().lookup(planeName, defaultDir, groupType));
             }
 
             std::vector<INode *> nodes;
             if (isContainerized())
             {
                 Linked<INode> dafileSrvNodeCopy;
                 {
-                    // in case config hook above changes dafileSrvNode
+                    // in case config hook above changes tlsDirectIONode/nonTlsDirectIONode
                     CriticalBlock b(dafileSrvNodeCS);
-                    dafileSrvNodeCopy.set(dafileSrvNode);
+                    dafileSrvNodeCopy.set(secure ? tlsDirectIONode : nonTlsDirectIONode);
+                }
+                if (!dafileSrvNodeCopy)
+                {
+                    const char *typeText = secure ? "secure" : "non-secure";
+                    throw makeStringExceptionV(0, "%s DFS service request made, but no %s directio service available", typeText, typeText);
                 }
                 for (unsigned n=0; n<group->ordinality(); n++)
                     nodes.push_back(dafileSrvNodeCopy);

diff --git a/dali/base/dautils.hpp b/dali/base/dautils.hpp
@@ -582,7 +582,7 @@ inline unsigned calcStripeNumber(unsigned partNum, const char *lfnName, unsigned
     return ((partNum+lfnHash)%numStripes)+1;
 }
 interface INamedGroupStore;
-extern da_decl void remapGroupsToDafilesrv(IPropertyTree *file, INamedGroupStore *resolver);
+extern da_decl void remapGroupsToDafilesrv(IPropertyTree *file, bool foreign, bool secure);
 
 #ifdef NULL_DALIUSER_STACKTRACE
 extern da_decl void logNullUser(IUserDescriptor *userDesc);

diff --git a/esp/services/ws_dfsservice/ws_dfsservice.cpp b/esp/services/ws_dfsservice/ws_dfsservice.cpp
@@ -42,7 +42,15 @@ static unsigned __int64 getLockId(unsigned __int64 leaseId)
     return ++nextLockID;
 }
 
-static void populateLFNMeta(const char *logicalName, unsigned __int64 leaseId, bool remap, IPropertyTree *metaRoot, IPropertyTree *meta)
+
+enum LfnMetaOpts : byte
+{
+    LfnMOptNone   = 0x00,
+    LfnMOptRemap  = 0x01,
+    LfnMOptTls    = 0x02,
+};
+BITMASK_ENUM(LfnMetaOpts);
+static void populateLFNMeta(const char *logicalName, unsigned __int64 leaseId, LfnMetaOpts opts, IPropertyTree *metaRoot, IPropertyTree *meta)
 {
     CDfsLogicalFileName lfn;
     lfn.set(logicalName);
@@ -54,8 +62,13 @@ static void populateLFNMeta(const char *logicalName, unsigned __int64 leaseId, b
     Owned<IPropertyTree> tree = queryDistributedFileDirectory().getFileTree(logicalName, nullptr);
     if (!tree)
         return;
-    if (remap)
-        remapGroupsToDafilesrv(tree, nullptr);
+    if (hasMask(opts, LfnMOptRemap))
+    {
+        bool secure = hasMask(opts, LfnMOptTls);
+        // If !secure - called from insecure DFS service, remapGroupsToDafilesrv needs to direct to an insecure dafilesrv
+
+        remapGroupsToDafilesrv(tree, false, secure);
+    }
 
     bool isSuper = streq(tree->queryName(), queryDfsXmlBranchName(DXB_SuperFile));
 
@@ -98,7 +111,7 @@ static void populateLFNMeta(const char *logicalName, unsigned __int64 leaseId, b
             {
                 IPropertyTree &sub = *(orderedSubFiles[f]);
                 sub.getProp("@name", subname.clear());
-                populateLFNMeta(subname, leaseId, remap, metaRoot, fileMeta);
+                populateLFNMeta(subname, leaseId, opts, metaRoot, fileMeta);
             }
         }
     }
@@ -160,9 +173,16 @@ bool CWsDfsEx::onDFSFileLookup(IEspContext &context, IEspDFSFileLookupRequest &r
         unsigned __int64 leaseId = req.getLeaseId();
 
         // populate file meta data and lock id's
+        LfnMetaOpts opts = LfnMOptNone;
+        if (req.getAccessViaDafilesrv())
+            opts |= LfnMOptRemap;
+
+        // NB: if we ever have some services with tls, and some without in bare-metal, this may need revisiting.
+        if (getComponentConfigSP()->getPropBool("@tls"))
+            opts |= LfnMOptTls;
+
         Owned<IPropertyTree> responseTree = createPTree();
-        bool remap = req.getAccessViaDafilesrv();
-        populateLFNMeta(logicalName, leaseId, remap, responseTree, responseTree);
+        populateLFNMeta(logicalName, leaseId, opts, responseTree, responseTree);
 
         // serialize response
         MemoryBuffer respMb, compressedRespMb;

diff --git a/esp/services/ws_dfu/ws_dfuService.cpp b/esp/services/ws_dfu/ws_dfuService.cpp
@@ -6116,7 +6116,7 @@ void CWsDfuEx::dFUFileAccessCommon(IEspContext &context, const CDfsLogicalFileNa
     if (!hasIssuerTlsConfig(keyPairName))
         throw makeStringExceptionV(-1, "dFUFileAccessCommon: file signing certificate ('%s') not defined in configuration.", keyPairName.str());
 
-    auto externalService = k8s::getDafileServiceFromConfig("stream");
+    auto externalService = k8s::getDafileServiceFromConfig("stream", true, true);
     dafilesrvHost.set(externalService.first.c_str());
     port = externalService.second;
     secure = true;
@@ -6497,7 +6497,7 @@ bool CWsDfuEx::onDFUFileCreateV2(IEspContext &context, IEspDFUFileCreateV2Reques
         fileDesc.setown(createFileDescriptor(tempFileName, planeName, numParts));
         numParts = fileDesc->numParts();
 
-        auto externalService = k8s::getDafileServiceFromConfig("stream");
+        auto externalService = k8s::getDafileServiceFromConfig("stream", true, true);
         dafilesrvHost.set(externalService.first.c_str());
         port = externalService.second;
         secure = true;

diff --git a/helm/hpcc/templates/dali.yaml b/helm/hpcc/templates/dali.yaml
@@ -29,10 +29,12 @@ Pass in root and me
 {{- define "hpcc.isForeignAccessConfigured" }}
 {{- $service := .me.service | default dict -}}
 {{- $visibility := $service.visibility | default "cluster" -}}
+{{- $match := false -}}
 {{- if (not (eq $visibility "cluster")) -}}
  {{ range .root.Values.dafilesrv -}}
-  {{- if not .disabled -}}
+  {{- if and (not $match) (not .disabled) -}}
    {{- if (eq "directio" .application) -}}
+    {{- $match = true -}}
 true
    {{- end -}}
   {{- end -}}
@@ -200,7 +202,6 @@ kind: ConfigMap
 {{/*
 Expose dali as a external service, only if there is a service definition and dafilesrv directio service is active.
 */}}
-{{- $dafilesrvCtx := dict -}}
 {{- $service := deepCopy (.service | default dict) -}}
 {{- $_ := set $service "visibility" ($service.visibility | default "cluster") -}}
 {{- $_ := set $service "servicePort" ($service.servicePort | default 7070) -}}

diff --git a/system/jlib/jcontainerized.cpp b/system/jlib/jcontainerized.cpp
@@ -370,7 +370,7 @@ std::pair<std::string, unsigned> getExternalService(const char *serviceName)
     return servicePair;
 }
 
-std::pair<std::string, unsigned> getDafileServiceFromConfig(const char *application)
+std::pair<std::string, unsigned> getDafileServiceFromConfig(const char *application, bool secure, bool errorIfMissing)
 {
 #ifndef _CONTAINERIZED
     UNIMPLEMENTED_X("getDafileServiceFromConfig");
@@ -381,29 +381,35 @@ std::pair<std::string, unsigned> getDafileServiceFromConfig(const char *applicat
      */
     VStringBuffer serviceXPath("services[@type='%s']", application);
     Owned<IPropertyTreeIterator> dafilesrvServices = getGlobalConfigSP()->getElements(serviceXPath);
-    if (!dafilesrvServices->first())
-        throw makeStringExceptionV(JLIBERR_K8sServiceError, "dafilesrv service '%s' not defined or disabled", application);
-    const IPropertyTree &dafilesrv = dafilesrvServices->query();
-    if (!dafilesrv.getPropBool("@public"))
-        throw makeStringExceptionV(JLIBERR_K8sServiceError, "dafilesrv service '%s' has no public service defined", application);
-    StringBuffer dafilesrvName;
-    dafilesrv.getProp("@name", dafilesrvName);
-    unsigned port = (unsigned)dafilesrv.getPropInt("@port");
-
-    StringBuffer hostname;
-    dafilesrv.getProp("@hostname", hostname);
-    if (hostname.length())
-        return { hostname.str(), port };
-    else
+    ForEach(*dafilesrvServices)
     {
-        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;
+        const IPropertyTree &dafilesrv = dafilesrvServices->query();
+        if (!dafilesrv.getPropBool("@public"))
+            continue;
+        if (secure != dafilesrv.getPropBool("@tls"))
+            continue;
+        StringBuffer dafilesrvName;
+        dafilesrv.getProp("@name", dafilesrvName);
+        unsigned port = (unsigned)dafilesrv.getPropInt("@port");
+
+        StringBuffer hostname;
+        dafilesrv.getProp("@hostname", hostname);
+        if (hostname.length())
+            return { hostname.str(), port };
+        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;
+        }
     }
+    if (errorIfMissing)
+        throw makeStringExceptionV(JLIBERR_K8sServiceError, "No suitable dafilesrv service '%s' enabled (Rquired be @public=true and @tls=%s)", application, boolToStr(secure));
+    return { "", 0 };
 }
 
 

diff --git a/system/jlib/jcontainerized.hpp b/system/jlib/jcontainerized.hpp
@@ -42,7 +42,7 @@ extern jlib_decl void runKubectlCommand(const char *title, const char *cmd, cons
 // return the k8s external host and port for serviceName
 extern jlib_decl std::pair<std::string, unsigned> getExternalService(const char *serviceName);
 
-extern jlib_decl std::pair<std::string, unsigned> getDafileServiceFromConfig(const char *application);
+extern jlib_decl std::pair<std::string, unsigned> getDafileServiceFromConfig(const char *application, bool secure, bool errorIfMissing);
 
 
 }