chore: split TD008 into two plugins; new one is TD010 (#477)

README.md

@@ -335,8 +335,9 @@ This is the current list:
 |   |:white_check_mark:| TD005 | TargetEndpoint SSLInfo references | TargetEndpoint SSLInfo should use references for KeyStore and TrustStore. |
 |   |:white_check_mark:| TD006 | TargetEndpoint SSLInfo | When using a LoadBalancer, the SSLInfo should not be configured under HTTPTargetConnection. |
 |   |:white_check_mark:| TD007 | TargetEndpoint SSLInfo | TargetEndpoint HTTPTargetConnection SSLInfo should use TrustStore. |
-|   |:white_check_mark:| TD008 | TargetEndpoint LoadBalancer Servers | LoadBalancer should not have duplicate Server entries or multiple Fallbacks. |
+|   |:white_check_mark:| TD008 | TargetEndpoint LoadBalancer Servers | LoadBalancer should not have multiple IsFallback Server entries. |
 |   |:white_check_mark:| TD009 | TargetEndpoint LoadBalancer | TargetEndpoint HTTPTargetConnection should have at most one LoadBalancer. |
+|   |:white_check_mark:| TD010 | TargetEndpoint LoadBalancer Servers | LoadBalancer should have at least one Server entry, and no duplicate Server entries. |
 | Flow |   |   |   |   |
 |   |:white_check_mark:| FL001 | Unconditional Flows |  Only one unconditional flow will get executed. Error if more than one was detected. |
 | Step |   |   |   |   |

lib/package/plugins/TD008-target-LB-multiple-fallbacks.js

@@ -0,0 +1,94 @@
+/*
+  Copyright 2019-2024 Google LLC
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+      https://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+*/
+
+const ruleId = require("../myUtil.js").getRuleId(),
+  xpath = require("xpath");
+
+const plugin = {
+  ruleId,
+  name: "TargetEndpoint HTTPTargetConnection LoadBalancer with multiple fallback entries",
+  message: "Multiple Server entries with IsFallback=true",
+  fatal: false,
+  severity: 1, // 1 = warn, 2 = error
+  nodeType: "Endpoint",
+  enabled: true
+};
+
+const path = require("path"),
+  debug = require("debug")("apigeelint:" + ruleId);
+
+const onTargetEndpoint = function (endpoint, cb) {
+  const htc = endpoint.getHTTPTargetConnection(),
+    shortFilename = path.basename(endpoint.getFileName());
+  let flagged = false;
+
+  debug(`onTargetEndpoint shortfile(${shortFilename})`);
+  if (htc) {
+    try {
+      const loadBalancers = htc.select("LoadBalancer");
+      debug(`loadBalancers(${loadBalancers.length})`);
+
+      if (loadBalancers.length == 1) {
+        const loadBalancer = loadBalancers[0];
+        // check multiple fallbacks
+        const fallbacks = xpath.select(
+          "Server[IsFallback = 'true']",
+          loadBalancer
+        );
+        if (fallbacks.length > 1) {
+          endpoint.addMessage({
+            plugin,
+            line: loadBalancers[0].lineNumber,
+            column: loadBalancers[0].columnNumber,
+            message: plugin.message
+          });
+          flagged = true;
+        }
+        const servers = xpath.select("Server", loadBalancer);
+        if (servers.length == 1 && fallbacks.length == 1) {
+          endpoint.addMessage({
+            plugin,
+            line: loadBalancers[0].lineNumber,
+            column: loadBalancers[0].columnNumber,
+            message:
+              "Only one server in a Load balancer; should not be marked IsFallback"
+          });
+          flagged = true;
+        }
+      }
+    } catch (exc1) {
+      console.error("exception: " + exc1);
+      debug(`onTargetEndpoint exc(${exc1})`);
+      debug(`${exc1.stack}`);
+      endpoint.addMessage({
+        plugin,
+        message: "Exception while processing: " + exc1
+      });
+
+      flagged = true;
+    }
+  }
+
+  if (typeof cb == "function") {
+    debug(`onTargetEndpoint isFlagged(${flagged})`);
+    cb(null, flagged);
+  }
+};
+
+module.exports = {
+  plugin,
+  onTargetEndpoint
+};

lib/package/plugins/TD008-target-LB-servers.js → lib/package/plugins/TD010-target-LB-duplicate-servers.js

@@ -43,23 +43,16 @@ const onTargetEndpoint = function (endpoint, cb) {
 
       if (loadBalancers.length == 1) {
         const loadBalancer = loadBalancers[0];
-        // check multiple fallbacks
-        const fallbacks = xpath.select(
-          "Server[IsFallback = 'true']",
-          loadBalancer
-        );
-        if (fallbacks.length > 1) {
+        const servers = xpath.select("Server", loadBalancer);
+        if (servers.length == 0) {
           endpoint.addMessage({
             plugin,
-            line: loadBalancers[0].lineNumber,
-            column: loadBalancers[0].columnNumber,
-            message: "Multiple Server entries with IsFallback=true"
+            line: loadBalancer.lineNumber,
+            column: loadBalancer.columnNumber,
+            message: "No Server elements within LoadBalancer"
           });
           flagged = true;
-        }
-
-        const servers = xpath.select("Server", loadBalancer);
-        if (servers.length > 1) {
+        } else if (servers.length > 1) {
           const previouslyDetected = [];
           servers.slice(0, -1).forEach((item, i) => {
             if (!previouslyDetected.includes(i)) {

test/fixtures/resources/TD008/fail/t2-multiple-fallbacks-non-consecutive.xml

@@ -0,0 +1,14 @@
+<TargetEndpoint name="default">
+    <HTTPTargetConnection>
+      <LoadBalancer>
+         <!-- two servers marked fallback will fail the TD008 check -->
+         <Server name="server1">
+           <IsFallback>true</IsFallback>
+         </Server>
+         <Server name="server2"/>
+         <Server name="server3">
+           <IsFallback>true</IsFallback>
+         </Server>
+      </LoadBalancer>
+    </HTTPTargetConnection>
+</TargetEndpoint>

test/fixtures/resources/TD008/fail/t3-one-server-marked-as-fallback.xml

@@ -0,0 +1,9 @@
+<TargetEndpoint name="default">
+  <HTTPTargetConnection>
+    <LoadBalancer>
+      <Server name="server1">
+        <IsFallback>true</IsFallback>
+      </Server>
+    </LoadBalancer>
+  </HTTPTargetConnection>
+</TargetEndpoint>

test/fixtures/resources/TD008/pass/t3-one-server-no-fallbacks.xml

@@ -0,0 +1,7 @@
+<TargetEndpoint name="default">
+  <HTTPTargetConnection>
+    <LoadBalancer>
+      <Server name="server1"/>
+    </LoadBalancer>
+  </HTTPTargetConnection>
+</TargetEndpoint>

test/fixtures/resources/TD008/pass/t6-many-servers-some-dupes-no-fallbacks.xml

@@ -0,0 +1,13 @@
+<TargetEndpoint name="default">
+  <HTTPTargetConnection>
+    <LoadBalancer>
+      <!-- this should fail the TD010 check, but pass the TD008 check -->
+      <Server name="server1"/>
+      <Server name="server2"/>
+      <Server name="server3"/>
+      <Server name="server2"/>
+      <Server name="server4"/>
+      <Server name="server2"/>
+    </LoadBalancer>
+  </HTTPTargetConnection>
+</TargetEndpoint>

test/fixtures/resources/TD008/pass/t7-three-servers-with-duplicate-no-fallbacks.xml

@@ -0,0 +1,10 @@
+<TargetEndpoint name="default">
+  <HTTPTargetConnection>
+    <LoadBalancer>
+      <!-- this should fail the TD010 check, but pass the TD008 check -->
+      <Server name="server1"/>
+      <Server name="server2"/>
+      <Server name="server1"/>
+    </LoadBalancer>
+  </HTTPTargetConnection>
+</TargetEndpoint>

test/fixtures/resources/TD008/pass/t8-two-servers-duplicate-no-fallback.xml

@@ -0,0 +1,9 @@
+<TargetEndpoint name="default">
+  <HTTPTargetConnection>
+    <LoadBalancer>
+      <!-- this should fail the TD010 check, but pass the TD008 check -->
+      <Server name="server1"/>
+      <Server name="server1"/>
+    </LoadBalancer>
+  </HTTPTargetConnection>
+</TargetEndpoint>

test/fixtures/resources/TD010/fail/T5-no-servers.xml

@@ -0,0 +1,8 @@
+<TargetEndpoint name="default">
+  <HTTPTargetConnection>
+    <LoadBalancer>
+      <!-- no Server element at all -->
+      <Swerve/>
+    </LoadBalancer>
+  </HTTPTargetConnection>
+</TargetEndpoint>

test/fixtures/resources/TD010/fail/t1-missing-name-attr.xml

@@ -0,0 +1,10 @@
+<TargetEndpoint name="default">
+  <HTTPTargetConnection>
+    <LoadBalancer>
+      <Server name="server1"/>
+      <!-- TD010 should flag the missing name attribute -->
+      <Server />
+      <Server name="server3"/>
+    </LoadBalancer>
+  </HTTPTargetConnection>
+</TargetEndpoint>

test/fixtures/resources/TD010/fail/t2-duplicate-server.xml

@@ -0,0 +1,9 @@
+<TargetEndpoint name="default">
+  <HTTPTargetConnection>
+    <LoadBalancer>
+      <!-- a duplicate server name - this should fail the TD010 check -->
+      <Server name="server1"/>
+      <Server name="server1"/>
+    </LoadBalancer>
+  </HTTPTargetConnection>
+</TargetEndpoint>

test/fixtures/resources/TD010/fail/t3-non-consecutive-duplicate.xml

@@ -0,0 +1,10 @@
+<TargetEndpoint name="default">
+  <HTTPTargetConnection>
+    <LoadBalancer>
+      <!-- this should fail the TD010 check -->
+      <Server name="server1"/>
+      <Server name="server2"/>
+      <Server name="server1"/>
+    </LoadBalancer>
+  </HTTPTargetConnection>
+</TargetEndpoint>

test/fixtures/resources/TD010/fail/t4-dupes-in-random-order.xml

@@ -0,0 +1,13 @@
+<TargetEndpoint name="default">
+  <HTTPTargetConnection>
+    <LoadBalancer>
+      <!-- this should fail the TD010 check, but pass the TD008 check -->
+      <Server name="server1"/>
+      <Server name="server2"/>
+      <Server name="server3"/>
+      <Server name="server2"/>
+      <Server name="server4"/>
+      <Server name="server2"/>
+    </LoadBalancer>
+  </HTTPTargetConnection>
+</TargetEndpoint>

test/fixtures/resources/TD010/pass/t1-two-servers.xml

@@ -0,0 +1,9 @@
+<TargetEndpoint name="default">
+  <HTTPTargetConnection>
+    <LoadBalancer>
+      <!-- no TD010 problem here, these are not duplicates -->
+      <Server name="server1"/>
+      <Server name="server2"/>
+    </LoadBalancer>
+  </HTTPTargetConnection>
+</TargetEndpoint>

test/fixtures/resources/TD008/fail/t2.xml → test/fixtures/resources/TD010/pass/t2-three-servers-no-fallback.xml

@@ -3,7 +3,7 @@
       <LoadBalancer>
          <Server name="server1"/>
          <Server name="server2"/>
-         <Server name="server1"/>
+         <Server name="server3"/>
       </LoadBalancer>
     </HTTPTargetConnection>
 </TargetEndpoint>

test/fixtures/resources/TD010/pass/t3-one-server.xml

@@ -0,0 +1,8 @@
+<TargetEndpoint name="default">
+  <HTTPTargetConnection>
+    <LoadBalancer>
+      <!-- no TD010 problem here, just one server -->
+      <Server name="server1"/>
+    </LoadBalancer>
+  </HTTPTargetConnection>
+</TargetEndpoint>

test/fixtures/resources/TD010/pass/t4-three-servers-with-fallback.xml

@@ -0,0 +1,12 @@
+<TargetEndpoint name="default">
+  <HTTPTargetConnection>
+    <LoadBalancer>
+      <Server name="server1"/>
+      <Server name="server2"/>
+      <!-- single fallback is OK -->
+      <Server name="server3">
+        <IsFallback>true</IsFallback>
+      </Server>
+    </LoadBalancer>
+  </HTTPTargetConnection>
+</TargetEndpoint>

test/fixtures/resources/TD010/pass/t5-three-servers-multiple-fallbacks.xml

@@ -0,0 +1,14 @@
+<TargetEndpoint name="default">
+  <HTTPTargetConnection>
+    <LoadBalancer>
+      <Server name="server1"/>
+      <!-- two servers marked fallback will fail the TD008 check, but pass the TD010 check -->
+      <Server name="server2">
+        <IsFallback>true</IsFallback>
+      </Server>
+      <Server name="server3">
+        <IsFallback>true</IsFallback>
+      </Server>
+    </LoadBalancer>
+  </HTTPTargetConnection>
+</TargetEndpoint>

test/fixtures/resources/TD010/pass/t6-three-servers-multiple-fallbacks.xml

@@ -0,0 +1,14 @@
+<TargetEndpoint name="default">
+  <HTTPTargetConnection>
+    <LoadBalancer>
+      <!-- two servers marked fallback will fail the TD008 check, but pass the TD010 check -->
+      <Server name="server1">
+        <IsFallback>true</IsFallback>
+      </Server>
+      <Server name="server2"/>
+      <Server name="server3">
+        <IsFallback>true</IsFallback>
+      </Server>
+    </LoadBalancer>
+  </HTTPTargetConnection>
+</TargetEndpoint>

test/specs/TD008-test.js

@@ -37,7 +37,7 @@ const loadEndpoint = (sourceDir, shortFileName) => {
   return endpoint;
 };
 
-describe(`${testID} - endpoint passes duplicate server check`, function () {
+describe(`${testID} - endpoint passes multiple fallback server check`, function () {
   const sourceDir = path.join(rootDir, "pass");
   const testOne = (shortFileName) => {
     const endpoint = loadEndpoint(sourceDir, shortFileName);
@@ -59,13 +59,13 @@ describe(`${testID} - endpoint passes duplicate server check`, function () {
     .filter((shortFileName) => shortFileName.endsWith(".xml"));
 
   it(`checks that there are tests`, () => {
-    assert.ok(candidates.length > 0, "tests should exist");
+    assert.ok(candidates.length > 1, "tests should exist");
   });
 
   candidates.forEach(testOne);
 });
 
-describe(`${testID} - endpoint does not pass duplicate server check`, () => {
+describe(`${testID} - endpoint does not pass multiple fallback server check`, () => {
   const sourceDir = path.join(rootDir, "fail");
 
   const testOne = (shortFileName) => {
@@ -86,7 +86,7 @@ describe(`${testID} - endpoint does not pass duplicate server check`, () => {
     .filter((shortFileName) => shortFileName.endsWith(".xml"));
 
   it(`checks that there are tests`, () => {
-    assert.ok(candidates.length > 0, "tests should exist");
+    assert.ok(candidates.length > 1, "tests should exist");
   });
 
   candidates.forEach(testOne);