From c54fcea9afb68aae6ac108f889205fd01d707327 Mon Sep 17 00:00:00 2001 From: Antonio Musolino Date: Wed, 20 Jul 2022 10:17:25 +0200 Subject: [PATCH 1/3] fix(ldap): search stream does not await callbacks Signed-off-by: Antonio Musolino --- .changeset/slimy-zebras-reply.md | 5 +++++ plugins/catalog-backend-module-ldap/api-report.md | 2 +- plugins/catalog-backend-module-ldap/src/ldap/client.ts | 9 ++++++--- 3 files changed, 12 insertions(+), 4 deletions(-) create mode 100644 .changeset/slimy-zebras-reply.md diff --git a/.changeset/slimy-zebras-reply.md b/.changeset/slimy-zebras-reply.md new file mode 100644 index 0000000000..5b309f5129 --- /dev/null +++ b/.changeset/slimy-zebras-reply.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-catalog-backend-module-ldap': patch +--- + +Now the `searchStream` method in LDAP client awaits the callbacks diff --git a/plugins/catalog-backend-module-ldap/api-report.md b/plugins/catalog-backend-module-ldap/api-report.md index 90e58b3fb2..9876c163e8 100644 --- a/plugins/catalog-backend-module-ldap/api-report.md +++ b/plugins/catalog-backend-module-ldap/api-report.md @@ -90,7 +90,7 @@ export class LdapClient { searchStreaming( dn: string, options: SearchOptions, - f: (entry: SearchEntry) => void, + f: (entry: SearchEntry) => Promise | void, ): Promise; } diff --git a/plugins/catalog-backend-module-ldap/src/ldap/client.ts b/plugins/catalog-backend-module-ldap/src/ldap/client.ts index d9d0faf9a2..d5f6a7d0f6 100644 --- a/plugins/catalog-backend-module-ldap/src/ldap/client.ts +++ b/plugins/catalog-backend-module-ldap/src/ldap/client.ts @@ -147,9 +147,10 @@ export class LdapClient { async searchStreaming( dn: string, options: SearchOptions, - f: (entry: SearchEntry) => void, + f: (entry: SearchEntry) => Promise | void, ): Promise { try { + const awaitList: Array | void> = []; return await new Promise((resolve, reject) => { // Note that we clone the (frozen) options, since ldapjs rudely tries to // overwrite parts of them @@ -163,7 +164,7 @@ export class LdapClient { }); res.on('searchEntry', entry => { - f(entry); + awaitList.push(f(entry)); }); res.on('error', e => { @@ -176,7 +177,9 @@ export class LdapClient { } else if (r.status !== 0) { throw new Error(`Got status ${r.status}: ${r.errorMessage}`); } else { - resolve(); + Promise.all(awaitList) + .then(() => resolve()) + .catch(reject); } }); }); From 5097e540874f573e5dcccc1173e15774d0592758 Mon Sep 17 00:00:00 2001 From: Antonio Musolino Date: Wed, 20 Jul 2022 14:26:11 +0200 Subject: [PATCH 2/3] fix(ldap): added manual paging with pause in searchStreaming to limit memory usage Signed-off-by: Antonio Musolino --- .../src/ldap/client.ts | 75 +++++++++++-------- 1 file changed, 45 insertions(+), 30 deletions(-) diff --git a/plugins/catalog-backend-module-ldap/src/ldap/client.ts b/plugins/catalog-backend-module-ldap/src/ldap/client.ts index d5f6a7d0f6..d182875e10 100644 --- a/plugins/catalog-backend-module-ldap/src/ldap/client.ts +++ b/plugins/catalog-backend-module-ldap/src/ldap/client.ts @@ -16,7 +16,7 @@ import { ForwardedError } from '@backstage/errors'; import ldap, { Client, SearchEntry, SearchOptions } from 'ldapjs'; -import { cloneDeep } from 'lodash'; +import { cloneDeep, merge } from 'lodash'; import { Logger } from 'winston'; import { BindConfig, TLSConfig } from './config'; import { errorString } from './util'; @@ -150,39 +150,54 @@ export class LdapClient { f: (entry: SearchEntry) => Promise | void, ): Promise { try { - const awaitList: Array | void> = []; + let awaitList: Array | void> = []; return await new Promise((resolve, reject) => { // Note that we clone the (frozen) options, since ldapjs rudely tries to // overwrite parts of them - this.client.search(dn, cloneDeep(options), (err, res) => { - if (err) { - reject(new Error(errorString(err))); - } - - res.on('searchReference', () => { - this.logger.warn('Received unsupported search referral'); - }); - - res.on('searchEntry', entry => { - awaitList.push(f(entry)); - }); - - res.on('error', e => { - reject(new Error(errorString(e))); - }); - - res.on('end', r => { - if (!r) { - throw new Error('Null response'); - } else if (r.status !== 0) { - throw new Error(`Got status ${r.status}: ${r.errorMessage}`); - } else { - Promise.all(awaitList) - .then(() => resolve()) - .catch(reject); + this.client.search( + dn, + merge(cloneDeep(options), { paged: { pagePause: true } }), + (err, res) => { + if (err) { + reject(new Error(errorString(err))); } - }); - }); + + res.on('searchReference', () => { + this.logger.warn('Received unsupported search referral'); + }); + + res.on('searchEntry', entry => { + awaitList.push(f(entry)); + }); + + res.on('page', (_, cb) => { + // awaits completion before fetching next page + Promise.all(awaitList) + .then(() => { + // flush list + awaitList = []; + if (cb) cb(); + }) + .catch(reject); + }); + + res.on('error', e => { + reject(new Error(errorString(e))); + }); + + res.on('end', r => { + if (!r) { + throw new Error('Null response'); + } else if (r.status !== 0) { + throw new Error(`Got status ${r.status}: ${r.errorMessage}`); + } else { + Promise.all(awaitList) + .then(() => resolve()) + .catch(reject); + } + }); + }, + ); }); } catch (e) { throw new ForwardedError(`LDAP search at DN "${dn}" failed`, e); From 3f0db06a79e47015f5d1cbff527ded5323df48c6 Mon Sep 17 00:00:00 2001 From: Antonio Musolino Date: Mon, 8 Aug 2022 16:12:19 +0200 Subject: [PATCH 3/3] fix(ldap): allowing usage of searchStreaming function in no-paged Signed-off-by: Antonio Musolino --- .../src/ldap/client.ts | 94 ++++++++++--------- .../src/ldap/util.test.ts | 51 +++++++++- .../src/ldap/util.ts | 15 ++- 3 files changed, 114 insertions(+), 46 deletions(-) diff --git a/plugins/catalog-backend-module-ldap/src/ldap/client.ts b/plugins/catalog-backend-module-ldap/src/ldap/client.ts index d182875e10..0afeb16248 100644 --- a/plugins/catalog-backend-module-ldap/src/ldap/client.ts +++ b/plugins/catalog-backend-module-ldap/src/ldap/client.ts @@ -14,12 +14,12 @@ * limitations under the License. */ -import { ForwardedError } from '@backstage/errors'; +import { ForwardedError, stringifyError } from '@backstage/errors'; import ldap, { Client, SearchEntry, SearchOptions } from 'ldapjs'; -import { cloneDeep, merge } from 'lodash'; +import { cloneDeep } from 'lodash'; import { Logger } from 'winston'; import { BindConfig, TLSConfig } from './config'; -import { errorString } from './util'; +import { createOptions, errorString } from './util'; import { ActiveDirectoryVendor, DefaultLdapVendor, @@ -150,54 +150,60 @@ export class LdapClient { f: (entry: SearchEntry) => Promise | void, ): Promise { try { - let awaitList: Array | void> = []; return await new Promise((resolve, reject) => { // Note that we clone the (frozen) options, since ldapjs rudely tries to // overwrite parts of them - this.client.search( - dn, - merge(cloneDeep(options), { paged: { pagePause: true } }), - (err, res) => { - if (err) { - reject(new Error(errorString(err))); - } + this.client.search(dn, createOptions(options), (err, res) => { + if (err) { + reject(new Error(errorString(err))); + } + let awaitList: Array | void> = []; + let transformError = false; - res.on('searchReference', () => { - this.logger.warn('Received unsupported search referral'); - }); + const transformReject = (e: Error) => { + transformError = true; + reject( + new Error( + `Transform function threw an exception, ${stringifyError(e)}`, + ), + ); + }; - res.on('searchEntry', entry => { - awaitList.push(f(entry)); - }); + res.on('searchReference', () => { + this.logger.warn('Received unsupported search referral'); + }); - res.on('page', (_, cb) => { - // awaits completion before fetching next page + res.on('searchEntry', entry => { + if (!transformError) awaitList.push(f(entry)); + }); + + res.on('page', (_, cb) => { + // awaits completion before fetching next page + Promise.all(awaitList) + .then(() => { + // flush list + awaitList = []; + if (cb) cb(); + }) + .catch(transformReject); + }); + + res.on('error', e => { + reject(new Error(errorString(e))); + }); + + res.on('end', r => { + if (!r) { + throw new Error('Null response'); + } else if (r.status !== 0) { + throw new Error(`Got status ${r.status}: ${r.errorMessage}`); + } else { Promise.all(awaitList) - .then(() => { - // flush list - awaitList = []; - if (cb) cb(); - }) - .catch(reject); - }); - - res.on('error', e => { - reject(new Error(errorString(e))); - }); - - res.on('end', r => { - if (!r) { - throw new Error('Null response'); - } else if (r.status !== 0) { - throw new Error(`Got status ${r.status}: ${r.errorMessage}`); - } else { - Promise.all(awaitList) - .then(() => resolve()) - .catch(reject); - } - }); - }, - ); + .then(() => resolve()) + .catch(transformReject); + } + }); + }); }); } catch (e) { throw new ForwardedError(`LDAP search at DN "${dn}" failed`, e); diff --git a/plugins/catalog-backend-module-ldap/src/ldap/util.test.ts b/plugins/catalog-backend-module-ldap/src/ldap/util.test.ts index 5978012875..d8787b1226 100644 --- a/plugins/catalog-backend-module-ldap/src/ldap/util.test.ts +++ b/plugins/catalog-backend-module-ldap/src/ldap/util.test.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { errorString } from './util'; +import { errorString, createOptions } from './util'; describe('errorString', () => { it('formats', () => { @@ -22,3 +22,52 @@ describe('errorString', () => { expect(errorString(e)).toEqual('1 n: m'); }); }); + +describe('createOptions', () => { + it('should add pagePause', () => { + const options = { + filter: 'f', + paged: true, + timeLimit: 42, + sizeLimit: 100, + derefAliases: 0, + typesOnly: false, + }; + expect(createOptions(options)).toEqual({ + filter: 'f', + paged: { pagePause: true }, + timeLimit: 42, + sizeLimit: 100, + derefAliases: 0, + typesOnly: false, + }); + const options2 = { + filter: 'f', + paged: {}, + timeLimit: 42, + sizeLimit: 100, + derefAliases: 0, + typesOnly: false, + }; + expect(createOptions(options2)).toEqual({ + filter: 'f', + paged: { pagePause: true }, + timeLimit: 42, + sizeLimit: 100, + derefAliases: 0, + typesOnly: false, + }); + }); + + it('should not add pagePause', () => { + const options = { + filter: 'f', + paged: false, + timeLimit: 42, + sizeLimit: 100, + derefAliases: 0, + typesOnly: false, + }; + expect(createOptions(options)).toEqual(options); + }); +}); diff --git a/plugins/catalog-backend-module-ldap/src/ldap/util.ts b/plugins/catalog-backend-module-ldap/src/ldap/util.ts index 0f443c1a86..74b8fe4f88 100644 --- a/plugins/catalog-backend-module-ldap/src/ldap/util.ts +++ b/plugins/catalog-backend-module-ldap/src/ldap/util.ts @@ -14,7 +14,8 @@ * limitations under the License. */ -import { Error as LDAPError, SearchEntry } from 'ldapjs'; +import { Error as LDAPError, SearchEntry, SearchOptions } from 'ldapjs'; +import { cloneDeep } from 'lodash'; import { LdapVendor } from './vendors'; /** @@ -54,6 +55,18 @@ export function mapStringAttr( } } +export function createOptions(inputOptions: SearchOptions): SearchOptions { + const result = cloneDeep(inputOptions); + + if (result.paged === true) { + result.paged = { pagePause: true }; + } else if (typeof result.paged === 'object') { + result.paged.pagePause = true; + } + + return result; +} + export type RecursivePartial = { [P in keyof T]?: T[P] extends (infer U)[] ? RecursivePartial[]