fixup! Manage CRR endpoints without explicit port

Author: maeldonn

bin/replication.js

@@ -40,7 +40,6 @@ function _createSetupReplication(command, options, log) {
     const destinationEndpoint =
         config.getBootstrapList()
         .find(dest => Array.isArray(dest.servers));
-    const destSiteConfig = destinationEndpoint ? config.getReplicationSiteDestConfig(destinationEndpoint.site) : null;
     return new SetupReplication({
         source: {
             bucket: sourceBucket,
@@ -52,8 +51,9 @@ function _createSetupReplication(command, options, log) {
         target: {
             bucket: targetBucket,
             credentials: targetCredentials,
-            hosts: targetIsExternal ? undefined : new RoundRobin(destSiteConfig.replicationEndpoint.servers),
-            transport: destSiteConfig?.transport || destination.transport,
+            hosts: targetIsExternal ?
+                undefined : new RoundRobin(destinationEndpoint.servers),
+            transport: destination.transport,
             isExternal: targetIsExternal,
             siteName,
         },

lib/Config.js

@@ -90,14 +90,19 @@ class Config extends EventEmitter {
             throw new Error('dataMoverTopic is required when lifecycle transitions is supported');
         }
 
-        if (parsedConfig.extensions && parsedConfig.extensions.replication
-            && parsedConfig.extensions.replication.destination
-            && parsedConfig.extensions.replication.destination.bootstrapList) {
-            this.bootstrapList = parsedConfig.extensions.replication
-            .destination.bootstrapList;
-        } else {
-            this.bootstrapList = [];
-        }
+        const destination = parsedConfig.extensions?.replication?.destination;
+        this.bootstrapList = destination?.bootstrapList?.map(endpoint => {
+            if (!Array.isArray(endpoint.servers)) {
+                return endpoint;
+            }
+
+            const transport = destination.sites?.[endpoint.site]?.transport || destination.transport || 'http';
+            const defaultPort = transport === 'https' ? 443 : 80;
+            const servers = endpoint.servers.map(server => server.includes(':') ? server : `${server}:${defaultPort}`);
+
+            return { ...endpoint, servers };
+        }) ?? [];
+
         const shouldRestrictToSite = process.env.BOOTSTRAP_SITE_NAME;
         if (shouldRestrictToSite) {
             this.bootstrapList = this.bootstrapList.filter(item => item.site === shouldRestrictToSite);
@@ -309,42 +314,13 @@ class Config extends EventEmitter {
      * @returns {object} site specific destination configuration
      */
     getReplicationSiteDestConfig(site) {
-        const { destination } = this.extensions.replication;
-        const siteConfig = destination.sites?.[site] || {};
-
-        const transport = siteConfig.transport || destination.transport || 'http';
-        const auth = siteConfig.auth || destination.auth;
-        const endpoint = this.bootstrapList.find(e => e.site === site);
-
+        const destConfig = this.extensions.replication.destination;
         return {
-            transport,
-            auth,
-            replicationEndpoint: this._normalizeEndpointServers(endpoint, transport),
+            transport: destConfig.sites?.[site]?.transport || destConfig.transport,
+            auth: destConfig.sites?.[site]?.auth || destConfig.auth,
+            replicationEndpoint: this.bootstrapList.find(endpoint => endpoint.site === site),
         };
     }
-
-    /**
-     * Normalize replication endpoint servers by adding the default port
-     * based on the transport protocol when no explicit port is specified.
-     * @param {object|undefined} endpoint - replication endpoint object
-     * @param {string} transport - transport protocol ('http' or 'https')
-     * @returns {object|undefined} endpoint with normalized servers
-     */
-    _normalizeEndpointServers(endpoint, transport) {
-        if (!endpoint || !Array.isArray(endpoint.servers)) {
-            return endpoint;
-        }
-
-        const defaultPort = transport === 'https' ? 443 : 80;
-        const normalizedServers = endpoint.servers.map(server => {
-            if (server.includes(':')) {
-                return server;
-            }
-            return `${server}:${defaultPort}`;
-        });
-
-        return { ...endpoint, servers: normalizedServers };
-    }
 }
 
 module.exports = new Config();

tests/unit/conf/Config.js

@@ -91,134 +91,99 @@ describe('Site name', () => {
     });
 });
 
+
 describe('Config', () => {
     describe('getReplicationSiteDestConfig', () => {
         let ogConfigFileEnv;
+
         before(() => {
             ogConfigFileEnv = process.env.BACKBEAT_CONFIG_FILE;
         });
-        afterEach(() => {
-            sinon.restore();
-        });
+
+        afterEach(() => sinon.restore());
+
         after(() => {
             if (ogConfigFileEnv) {
                 process.env.BACKBEAT_CONFIG_FILE = ogConfigFileEnv;
             }
         });
-        it('should return replication site destination config', () => {
-            process.env.BACKBEAT_CONFIG_FILE = `${__dirname}/configs/replicationMultiDestConfig.json`;
-            const conf = new Config();
-            const destConfig = conf.getReplicationSiteDestConfig('aws3');
-            assert.deepStrictEqual(destConfig, {
-                transport: 'https',
-                auth: {
-                    type: 'service',
-                    account: 'service-replication-3',
-                },
-                replicationEndpoint: {
-                    site: 'aws3',
-                    type: 'aws_s3',
-                },
-            });
-        });
 
-        it('should normalize server entries with default port 443 for https transport', () => {
-            const conf = new Config();
-            conf.bootstrapList = [{ site: 'site1', servers: ['s3.example.com']}];
-            conf.extensions.replication.destination = {
-                transport: 'https',
-                sites: {
-                    site1: { transport: 'https' },
-                },
-                bootstrapList: [
-                    { site: 'site1', servers: ['s3.example.com'] },
-                ],
-            };
+        describe('bootstrapList server normalization', () => {
+            let conf;
 
-            const destConfig = conf.getReplicationSiteDestConfig('site1');
-            assert.deepStrictEqual(destConfig.replicationEndpoint.servers, ['s3.example.com:443']);
-        });
+            before(() => {
+                process.env.BACKBEAT_CONFIG_FILE = `${__dirname}/configs/replicationServersConfig.json`;
+                conf = new Config();
+            });
 
-        it('should normalize server entries with default port 80 for http transport', () => {
-            const conf = new Config();
-            conf.bootstrapList = [{ site: 'site1', servers: ['s3.example.com']}];
-            conf.extensions.replication.destination = {
-                transport: 'http',
-                sites: {
-                    site1: { transport: 'http' },
-                },
-                bootstrapList: [
-                    { site: 'site1', servers: ['s3.example.com'] },
-                ],
-            };
+            it('should normalize server entries with default port 443 for https transport', () => {
+                const entry = conf.bootstrapList.find(e => e.site === 'https-site');
+                assert.deepStrictEqual(entry.servers, ['s3.example.com:443']);
+            });
 
-            const destConfig = conf.getReplicationSiteDestConfig('site1');
-            assert.deepStrictEqual(destConfig.replicationEndpoint.servers, ['s3.example.com:80']);
-        });
+            it('should normalize server entries with default port 80 for http transport', () => {
+                const entry = conf.bootstrapList.find(e => e.site === 'http-site');
+                assert.deepStrictEqual(entry.servers, ['s3.example.com:80']);
+            });
 
-        it('should preserve explicit port in server entries', () => {
-            const conf = new Config();
-            conf.bootstrapList = [{ site: 'site1', servers: ['s3.example.com:8443']}];
-            conf.extensions.replication.destination = {
-                transport: 'https',
-                sites: {
-                    site1: { transport: 'https' },
-                },
-                bootstrapList: [
-                    { site: 'site1', servers: ['s3.example.com:8443'] },
-                ],
-            };
+            it('should preserve explicit port in server entries', () => {
+                const entry = conf.bootstrapList.find(e => e.site === 'explicit-port-site');
+                assert.deepStrictEqual(entry.servers, ['s3.example.com:8443']);
+            });
 
-            const destConfig = conf.getReplicationSiteDestConfig('site1');
-            assert.deepStrictEqual(destConfig.replicationEndpoint.servers, ['s3.example.com:8443']);
+            it('should not modify endpoint without servers array', () => {
+                const entry = conf.bootstrapList.find(e => e.site === 'aws-site');
+                assert.strictEqual(entry.servers, undefined);
+                assert.strictEqual(entry.type, 'aws_s3');
+            });
         });
 
-        it('should not modify endpoint without servers array', () => {
-            const conf = new Config();
-            conf.bootstrapList = [{ site: 'aws1', type: 'aws_s3' }];
-            conf.extensions.replication.destination = {
-                transport: 'http',
-                sites: {
-                    aws1: { transport: 'http' },
-                },
-                bootstrapList: [
-                    { site: 'aws1', type: 'aws_s3' },
-                ],
-            };
 
-            const destConfig = conf.getReplicationSiteDestConfig('aws1');
-            assert.deepStrictEqual(destConfig.replicationEndpoint, {
-                site: 'aws1',
-                type: 'aws_s3',
+        describe('getReplicationSiteDestConfig', () => {
+            it('should return replication site destination config', () => {
+                process.env.BACKBEAT_CONFIG_FILE = `${__dirname}/configs/replicationMultiDestConfig.json`;
+                const conf = new Config();
+                const destConfig = conf.getReplicationSiteDestConfig('aws3');
+                assert.deepStrictEqual(destConfig, {
+                    transport: 'https',
+                    auth: {
+                        type: 'service',
+                        account: 'service-replication-3',
+                    },
+                    replicationEndpoint: {
+                        site: 'aws3',
+                        type: 'aws_s3',
+                    },
+                });
             });
-        });
 
-        it('should return default replication destination config when site one is not available', () => {
-            process.env.BACKBEAT_CONFIG_FILE = `${__dirname}/configs/replicationMultiDestConfig.json`;
-            const conf = new Config();
-            sinon.stub(conf.extensions.replication, 'destination').value({
-                transport: 'https',
-                auth: {
-                    type: 'service',
-                    account: 'service-replication',
-                },
-                bootstrapList: [
-                    { site: 'aws1', type: 'aws_s3' },
-                    { site: 'aws2', type: 'aws_s3' },
-                    { site: 'aws3', type: 'aws_s3' }
-                ]
-            });
-            const destConfig = conf.getReplicationSiteDestConfig('aws3');
-            assert.deepStrictEqual(destConfig, {
-                transport: 'https',
-                auth: {
-                    type: 'service',
-                    account: 'service-replication',
-                },
-                replicationEndpoint: {
-                    site: 'aws3',
-                    type: 'aws_s3',
-                },
+            it('should return default replication destination config when site one is not available', () => {
+                process.env.BACKBEAT_CONFIG_FILE = `${__dirname}/configs/replicationMultiDestConfig.json`;
+                const conf = new Config();
+                sinon.stub(conf.extensions.replication, 'destination').value({
+                    transport: 'https',
+                    auth: {
+                        type: 'service',
+                        account: 'service-replication',
+                    },
+                    bootstrapList: [
+                        { site: 'aws1', type: 'aws_s3' },
+                        { site: 'aws2', type: 'aws_s3' },
+                        { site: 'aws3', type: 'aws_s3' }
+                    ]
+                });
+                const destConfig = conf.getReplicationSiteDestConfig('aws3');
+                assert.deepStrictEqual(destConfig, {
+                    transport: 'https',
+                    auth: {
+                        type: 'service',
+                        account: 'service-replication',
+                    },
+                    replicationEndpoint: {
+                        site: 'aws3',
+                        type: 'aws_s3',
+                    },
+                });
             });
         });
     });