Merge pull request #5490 from onematchfox/fix/proxy-config-env-vars

Remove requirement for proxy routes to be prefixed with `/`

.changeset/thick-cobras-switch.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-proxy-backend': patch
+---
+
+Prefix proxy routes with `/` if not present in configuration

docs/plugins/proxying.md

@@ -36,7 +36,7 @@ Example:
 ```yaml
 # in app-config.yaml
 proxy:
-  '/simple-example': http://simple.example.com:8080
+  simple-example: http://simple.example.com:8080
   '/larger-example/v1':
     target: http://larger.example.com:8080/svc.v1
     headers:
@@ -46,10 +46,11 @@ proxy:
 ```
 
 Each key under the proxy configuration entry is a route to match, below the
-prefix that the proxy plugin is mounted on. It must start with a slash. For
-example, if the backend mounts the proxy plugin as `/proxy`, the above
-configuration will lead to the proxy acting on backend requests to
-`/api/proxy/simple-example/...` and `/api/proxy/larger-example/v1/...`.
+prefix that the proxy plugin is mounted on. If it does not start with a slash,
+one will be prefixed automatically. For example, if the backend mounts the proxy
+plugin as `/proxy`, the above configuration will lead to the proxy acting on
+backend requests to `/api/proxy/simple-example/...` and
+`/api/proxy/larger-example/v1/...`.
 
 The value inside each route is either a simple URL string, or an object on the
 format accepted by

plugins/proxy-backend/src/service/router.test.ts

@@ -59,8 +59,8 @@ describe('buildMiddleware', () => {
     mockCreateProxyMiddleware.mockClear();
   });
 
-  it('accepts strings', async () => {
-    buildMiddleware('/api/', logger, 'test', 'http://mocked');
+  it('accepts strings prefixed by /', async () => {
+    buildMiddleware('/proxy', logger, '/test', 'http://mocked');
 
     expect(createProxyMiddleware).toHaveBeenCalledTimes(1);
 
@@ -74,13 +74,53 @@ describe('buildMiddleware', () => {
     expect(filter('', { method: 'PATCH', headers: {} })).toBe(true);
     expect(filter('', { method: 'DELETE', headers: {} })).toBe(true);
 
-    expect(fullConfig.pathRewrite).toEqual({ '^/api/test/': '/' });
+    expect(fullConfig.pathRewrite).toEqual({ '^/proxy/test/': '/' });
+    expect(fullConfig.changeOrigin).toBe(true);
+    expect(fullConfig.logProvider!(logger)).toBe(logger);
+  });
+
+  it('accepts routes not prefixed with / when path is not suffixed with /', async () => {
+    buildMiddleware('/proxy', logger, 'test', 'http://mocked');
+
+    expect(createProxyMiddleware).toHaveBeenCalledTimes(1);
+
+    const [filter, fullConfig] = mockCreateProxyMiddleware.mock.calls[0] as [
+      (pathname: string, req: Partial<http.IncomingMessage>) => boolean,
+      ProxyMiddlewareConfig,
+    ];
+    expect(filter('', { method: 'GET', headers: {} })).toBe(true);
+    expect(filter('', { method: 'POST', headers: {} })).toBe(true);
+    expect(filter('', { method: 'PUT', headers: {} })).toBe(true);
+    expect(filter('', { method: 'PATCH', headers: {} })).toBe(true);
+    expect(filter('', { method: 'DELETE', headers: {} })).toBe(true);
+
+    expect(fullConfig.pathRewrite).toEqual({ '^/proxy/test/': '/' });
+    expect(fullConfig.changeOrigin).toBe(true);
+    expect(fullConfig.logProvider!(logger)).toBe(logger);
+  });
+
+  it('accepts routes prefixed with / when path is suffixed with /', async () => {
+    buildMiddleware('/proxy/', logger, '/test', 'http://mocked');
+
+    expect(createProxyMiddleware).toHaveBeenCalledTimes(1);
+
+    const [filter, fullConfig] = mockCreateProxyMiddleware.mock.calls[0] as [
+      (pathname: string, req: Partial<http.IncomingMessage>) => boolean,
+      ProxyMiddlewareConfig,
+    ];
+    expect(filter('', { method: 'GET', headers: {} })).toBe(true);
+    expect(filter('', { method: 'POST', headers: {} })).toBe(true);
+    expect(filter('', { method: 'PUT', headers: {} })).toBe(true);
+    expect(filter('', { method: 'PATCH', headers: {} })).toBe(true);
+    expect(filter('', { method: 'DELETE', headers: {} })).toBe(true);
+
+    expect(fullConfig.pathRewrite).toEqual({ '^/proxy/test/': '/' });
     expect(fullConfig.changeOrigin).toBe(true);
     expect(fullConfig.logProvider!(logger)).toBe(logger);
   });
 
   it('limits allowedMethods', async () => {
-    buildMiddleware('/api/', logger, 'test', {
+    buildMiddleware('/proxy', logger, '/test', {
       target: 'http://mocked',
       allowedMethods: ['GET', 'DELETE'],
     });
@@ -97,13 +137,13 @@ describe('buildMiddleware', () => {
     expect(filter('', { method: 'PATCH', headers: {} })).toBe(false);
     expect(filter('', { method: 'DELETE', headers: {} })).toBe(true);
 
-    expect(fullConfig.pathRewrite).toEqual({ '^/api/test/': '/' });
+    expect(fullConfig.pathRewrite).toEqual({ '^/proxy/test/': '/' });
     expect(fullConfig.changeOrigin).toBe(true);
     expect(fullConfig.logProvider!(logger)).toBe(logger);
   });
 
   it('permits default headers', async () => {
-    buildMiddleware('/api/', logger, 'test', {
+    buildMiddleware('/proxy', logger, '/test', {
       target: 'http://mocked',
     });
 
@@ -143,7 +183,7 @@ describe('buildMiddleware', () => {
   });
 
   it('permits default and configured headers', async () => {
-    buildMiddleware('/api/', logger, 'test', {
+    buildMiddleware('/proxy', logger, '/test', {
       target: 'http://mocked',
       headers: {
         Authorization: 'my-token',
@@ -176,7 +216,7 @@ describe('buildMiddleware', () => {
   });
 
   it('permits configured headers', async () => {
-    buildMiddleware('/api/', logger, 'test', {
+    buildMiddleware('/proxy', logger, '/test', {
       target: 'http://mocked',
       allowedHeaders: ['authorization', 'cookie'],
     });
@@ -208,7 +248,7 @@ describe('buildMiddleware', () => {
   });
 
   it('responds default headers', async () => {
-    buildMiddleware('/api/', logger, 'test', {
+    buildMiddleware('/proxy', logger, '/test', {
       target: 'http://mocked',
     });
 
@@ -251,7 +291,7 @@ describe('buildMiddleware', () => {
   });
 
   it('responds configured headers', async () => {
-    buildMiddleware('/api/', logger, 'test', {
+    buildMiddleware('/proxy', logger, '/test', {
       target: 'http://mocked',
       allowedHeaders: ['set-cookie'],
     });
@@ -282,10 +322,10 @@ describe('buildMiddleware', () => {
 
   it('rejects malformed target URLs', async () => {
     expect(() =>
-      buildMiddleware('/api/', logger, 'test', 'backstage.io'),
+      buildMiddleware('/proxy', logger, '/test', 'backstage.io'),
     ).toThrowError(/Proxy target is not a valid URL/);
     expect(() =>
-      buildMiddleware('/api/', logger, 'test', { target: 'backstage.io' }),
+      buildMiddleware('/proxy', logger, '/test', { target: 'backstage.io' }),
     ).toThrowError(/Proxy target is not a valid URL/);
   });
 });

plugins/proxy-backend/src/service/router.ts

@@ -77,10 +77,24 @@ export function buildMiddleware(
       `Proxy target is not a valid URL: ${fullConfig.target ?? ''}`,
     );
   }
+
   // Default is to do a path rewrite that strips out the proxy's path prefix
   // and the rest of the route.
   if (fullConfig.pathRewrite === undefined) {
-    const routeWithSlash = route.endsWith('/') ? route : `${route}/`;
+    let routeWithSlash = route.endsWith('/') ? route : `${route}/`;
+
+    if (!pathPrefix.endsWith('/') && !routeWithSlash.startsWith('/')) {
+      // Need to insert a / between pathPrefix and routeWithSlash
+      routeWithSlash = `/${routeWithSlash}`;
+    } else if (pathPrefix.endsWith('/') && routeWithSlash.startsWith('/')) {
+      // Never expect this to happen at this point in time as
+      // pathPrefix is set using `getExternalBaseUrl` which "Returns the
+      // external HTTP base backend URL for a given plugin,
+      // **without a trailing slash.**". But in case this changes in future, we
+      // need to drop a / on either pathPrefix or routeWithSlash
+      routeWithSlash = routeWithSlash.substring(1);
+    }
+
     fullConfig.pathRewrite = {
       [`^${pathPrefix}${routeWithSlash}`]: '/',
     };