apigee / apigeelint

Static code analysis for Apigee proxy bundles to encourage API developers to use best practices and avoid anti-patterns.
Apache License 2.0
92 stars 71 forks source link

Make sample extra policy work with unknown policies #273

Closed henschkowski closed 2 years ago

henschkowski commented 2 years ago

The sample extra check plugin fails if the checked apiproxy has an unknown policyType in it, like "DecodeJWT". This change makes the plugin ignore those policies.

henschkowski commented 2 years ago

added the cla info

ssvaidyanathan commented 2 years ago

Isn't that the correct approach - fail if an unknown policy is added? Please add an issue when the plugin needs an update to accommodate a new policy

@DinoChiesa - WDYT?

kurtkanaskie commented 2 years ago

I just noticed this. Can you provide an example of the situation you are fixing?

For example, I changed a policy from <CORS ... to <CORS-X ... and apigeelint didn't complain, in fact it totally ignored the policy.

Is that the issue? What version of apigeelint did you use? I'm using 2.11.0

henschkowski commented 2 years ago

Was giving a demo to a customer, showing that they could include their own check plugins with that --externalPluginsDirectory option. But the program did not pick up that plugin - at least that was the experience. I used a simple proxy with a DecodeJWT policy.

IMHO the plugin should just ignore that unknown policy and move on (better: log it / warn at the end of output).

kurtkanaskie commented 2 years ago

Hi henschkowski, Please try the same on version 2.11.0, which I just tested.

I'm still not following your use case. Running the following in the source directory: apigeelint -x ./externalPlugins -e PO007 -s ./apiproxy Uses the EX-PO007-NamingConventions.js external plugin and excludes the use of the built in plugin PO007.

If there is a policy (e.g. CORS) which that external plugin does not know about, it quietly ignores it.

Furthermore, if I use the classic Apigee profile (default) I am warned about a policy that is not available for that profile (e.g. CORS), but that's unrelated.

/apiproxy/policies/CORS.xml

║ Line     │ Column   │ Type     │ Message                                                │ Rule ID              ║
╟──────────┼──────────┼──────────┼────────────────────────────────────────────────────────┼──────────────────────╢
║ 0        │ 0        │ warning  │ The policy type (CORS) is not available in the         │ PO028                ║
║          │          │          │ profile apigee.                                        │                      ║
henschkowski commented 2 years ago

Hello kurtkanaskie,

Tried with 2.11.0 (git master), same result. Maybe it depends on the position of the policy, I test with a DecodeJWT policy right at the beginning.

Warnings from the EX-P0007 plugin will then not show up, as the plugin is quietly ignored for all policies. With the change that I proposed those warnings will show up, albeit only for known policies.

It's not a big thing, just making it more intuitive for first-time users, IMHO.

kurtkanaskie commented 2 years ago

OK, I'm trying to reproduce the issue and demonstrate your fix.\ I ran the following on the attached proxy.

./cli -x ./externalPlugins -s ./apiproxy -f table.js --profile apigeex

Kindly demonstrate what you've fixed, because I'm still not seeing it apiproxy.zip

henschkowski commented 2 years ago

That apiproxy does not show it as it contains two "unknown" policies. Please add an "Extract-Variables" policy like this and retry:

Extract-Variables-1.xml:

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<ExtractVariables name="Extract-Variables-1">
    <DisplayName>Extract Variables-1</DisplayName>
    <Properties/>
    <Header name="Authorization">
        <Pattern ignoreCase="false">Bearer {private.jwt}</Pattern>
    </Header>
    <Source clearPayload="false">request</Source>
</ExtractVariables>

You should see (without the change):

/apiproxy/policies/CORS.xml

║ LineColumnTypeMessageRule ID              ║
╟──────────┼──────────┼──────────┼────────────────────────────────────────────────────────┼──────────────────────╢
║ 2        │ 51       │ warning  │ Non-standard prefix (null). Valid prefixes for         │ PO007                ║
║          │          │          │ CORS include: ["cors"]                                 │                      ║
║ 0        │ 0        │ warning  │ The policy type (CORS) is not available in the         │ PO028                ║
║          │          │          │ profile apigee.                                        │                      ║

/apiproxy/policies/Extract-Variables-1.xml

║ LineColumnTypeMessageRule ID              ║
╟──────────┼──────────┼──────────┼────────────────────────────────────────────────────────┼──────────────────────╢
║ 0        │ 0        │ warning  │ Extract-Variables-1 is not attached to a Step in       │ BN005                ║
║          │          │          │ the bundle.  Remove unused policies.                   │                      ║
║ 3        │ 5        │ warning  │ Filename "Extract-Variables-1.xml" does not match      │ PO008                ║
║          │          │          │ policy display name "Extract Variables-1". To          │                      ║
║          │          │          │ avoid confusion when working online and offline        │                      ║
║          │          │          │ use the same name for files and display name in        │                      ║
║          │          │          │ policies (excluding .xml extension).                   │                      ║

╔════════════════════════════════════════════════════════════════════════════════════════════════════════════════╗
║ 0 Errors                                                                                                       ║
╟────────────────────────────────────────────────────────────────────────────────────────────────────────────────╢
║ 4 Warnings                                                                                                     ║
╚════════════════════════════════════════════════════════════════════════════════════════════════════════════════╝

And with the change:

/apiproxy/policies/CORS.xml

║ LineColumnTypeMessageRule ID              ║
╟──────────┼──────────┼──────────┼────────────────────────────────────────────────────────┼──────────────────────╢
║ 2        │ 51       │ warning  │ Non-standard prefix (null). Valid prefixes for         │ PO007                ║
║          │          │          │ CORS include: ["cors"]                                 │                      ║
║ 0        │ 0        │ warning  │ The policy type (CORS) is not available in the         │ PO028                ║
║          │          │          │ profile apigee.                                        │                      ║

/apiproxy/policies/Extract-Variables-1.xml

║ LineColumnTypeMessageRule ID              ║
╟──────────┼──────────┼──────────┼────────────────────────────────────────────────────────┼──────────────────────╢
║ 0        │ 0        │ warning  │ Extract-Variables-1 is not attached to a Step in       │ BN005                ║
║          │          │          │ the bundle.  Remove unused policies.                   │                      ║
║ 3        │ 5        │ warning  │ Filename "Extract-Variables-1.xml" does not match      │ PO008                ║
║          │          │          │ policy display name "Extract Variables-1". To          │                      ║
║          │          │          │ avoid confusion when working online and offline        │                      ║
║          │          │          │ use the same name for files and display name in        │                      ║
║          │          │          │ policies (excluding .xml extension).                   │                      ║
║ 0        │ 0        │ warning  │ Naming Conventions: Policy "Extract Variables-1"       │ EX-PO007             ║
║          │          │          │ of type "ExtractVariables" should have an              │                      ║
║          │          │          │ indicative prefix. Valid prefixes include: ["EV-"]     │                      ║

╔════════════════════════════════════════════════════════════════════════════════════════════════════════════════╗
║ 0 Errors                                                                                                       ║
╟────────────────────────────────────────────────────────────────────────────────────────────────────────────────╢
║ 5 Warnings                                                                                                     ║
╚════════════════════════════════════════════════════════════════════════════════════════════════════════════════╝
kurtkanaskie commented 2 years ago

Ah, now I see it, thanks, that's a subtle one.

In my words:\ The sample external plugin EX-PO007-NamingConventions.js errors and exits quietly if the checked apiproxy has a policyType in it that is not represented in it's policyMetaData variable, like "DecodeJWT". This change makes the plugin skip those unmentioned policies.

The error:

apigeelint:bundleLinter plugin error: EX-PO007-NamingConventions.js TypeError: Cannot read property 'indications' of undefined +0ms
ssvaidyanathan commented 2 years ago

Closing this as #284 has the fix for this

@kurtkanaskie - can you please run the same test with the latest code from master

kurtkanaskie commented 2 years ago

Ran tests using latest code and it works.

ssvaidyanathan commented 2 years ago

v2.12.0 has this fix