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.

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

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

Merged
merged 1 commit into from
Sep 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |   |   |   |   |
Expand Down
94 changes: 94 additions & 0 deletions lib/package/plugins/TD008-target-LB-multiple-fallbacks.js
Original file line number Diff line number Diff line change
@@ -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
};
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
8 changes: 0 additions & 8 deletions test/fixtures/resources/TD008/fail/t1.xml

This file was deleted.

Original file line number Diff line number Diff line change
@@ -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>
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<TargetEndpoint name="default">
<HTTPTargetConnection>
<LoadBalancer>
<Server name="server1">
<IsFallback>true</IsFallback>
</Server>
</LoadBalancer>
</HTTPTargetConnection>
</TargetEndpoint>
12 changes: 0 additions & 12 deletions test/fixtures/resources/TD008/fail/t3.xml

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<TargetEndpoint name="default">
<HTTPTargetConnection>
<LoadBalancer>
<Server name="server1"/>
</LoadBalancer>
</HTTPTargetConnection>
</TargetEndpoint>
7 changes: 0 additions & 7 deletions test/fixtures/resources/TD008/pass/t3.xml

This file was deleted.

Original file line number Diff line number Diff line change
@@ -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>
Original file line number Diff line number Diff line change
@@ -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>
Original file line number Diff line number Diff line change
@@ -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>
8 changes: 8 additions & 0 deletions test/fixtures/resources/TD010/fail/T5-no-servers.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<TargetEndpoint name="default">
<HTTPTargetConnection>
<LoadBalancer>
<!-- no Server element at all -->
<Swerve/>
</LoadBalancer>
</HTTPTargetConnection>
</TargetEndpoint>
10 changes: 10 additions & 0 deletions test/fixtures/resources/TD010/fail/t1-missing-name-attr.xml
Original file line number Diff line number Diff line change
@@ -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>
9 changes: 9 additions & 0 deletions test/fixtures/resources/TD010/fail/t2-duplicate-server.xml
Original file line number Diff line number Diff line change
@@ -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>
Original file line number Diff line number Diff line change
@@ -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>
13 changes: 13 additions & 0 deletions test/fixtures/resources/TD010/fail/t4-dupes-in-random-order.xml
Original file line number Diff line number Diff line change
@@ -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>
9 changes: 9 additions & 0 deletions test/fixtures/resources/TD010/pass/t1-two-servers.xml
Original file line number Diff line number Diff line change
@@ -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>
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<LoadBalancer>
<Server name="server1"/>
<Server name="server2"/>
<Server name="server1"/>
<Server name="server3"/>
</LoadBalancer>
</HTTPTargetConnection>
</TargetEndpoint>
8 changes: 8 additions & 0 deletions test/fixtures/resources/TD010/pass/t3-one-server.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<TargetEndpoint name="default">
<HTTPTargetConnection>
<LoadBalancer>
<!-- no TD010 problem here, just one server -->
<Server name="server1"/>
</LoadBalancer>
</HTTPTargetConnection>
</TargetEndpoint>
Original file line number Diff line number Diff line change
@@ -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>
Original file line number Diff line number Diff line change
@@ -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>
Original file line number Diff line number Diff line change
@@ -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>
8 changes: 4 additions & 4 deletions test/specs/TD008-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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) => {
Expand All @@ -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);
Expand Down
Loading
Loading