Fix tcp in server name when adding firewall rule (#233)

* Test TCP connection * Add debug message * Remove tcp from server name * Explicitly exclude tcp * Rebuild main.js
Azure · Jul 1, 2024 · 048d916 · 048d916
1 parent 83d6b65
commit 048d916
Show file tree

Hide file tree

Showing 8 changed files with 31 additions and 25 deletions.
diff --git a/.github/workflows/adhoc-tests.yml b/.github/workflows/adhoc-tests.yml
@@ -43,29 +43,29 @@ jobs:
     - name: Test DACPAC Action
       uses: ./
       with:
-        connection-string: '${{ secrets.ADHOC_CONNECTION_STRING_NO_DATABASE }}Initial Catalog=${{ env.TEST_DB }};'
+        connection-string: 'Server=tcp:${{ secrets.TEST_SERVER }};Initial Catalog=${{ env.TEST_DB }};Authentication=Active Directory Default;'
         path: ./__testdata__/sql-action.dacpac
         action: 'publish'
 
     # Build and publish sqlproj that should create a new view
     - name: Test Build and Publish
       uses: ./
       with:
-        connection-string: '${{ secrets.ADHOC_CONNECTION_STRING_NO_DATABASE }}Initial Catalog=${{ env.TEST_DB }};'
+        connection-string: 'Server=tcp:${{ secrets.TEST_SERVER }};Initial Catalog=${{ env.TEST_DB }};Authentication=Active Directory Default;'
         path: ./__testdata__/TestProject/sql-action.sqlproj
         action: 'publish'
 
     # Execute testsql.sql via script action on server
     - name: Test SQL Action
       uses: ./
       with:
-        connection-string: '${{ secrets.ADHOC_CONNECTION_STRING_NO_DATABASE }}Initial Catalog=${{ env.TEST_DB }};'
+        connection-string: 'Server=tcp:${{ secrets.TEST_SERVER }};Initial Catalog=${{ env.TEST_DB }};Authentication=Active Directory Default;'
         path: ./__testdata__/testsql.sql
 
     - name: Cleanup Test Database
       if: always()
       uses: ./
       with: 
-        connection-string: '${{ secrets.ADHOC_CONNECTION_STRING_NO_DATABASE }}Initial Catalog=master;'
+        connection-string: 'Server=${{ secrets.TEST_SERVER }};Initial Catalog=master;Authentication=Active Directory Default;'
         path: ./__testdata__/cleanup.sql
-        arguments: '-v DbName="${{ env.TEST_DB }}"'
+        arguments: '-v DbName="${{ env.TEST_DB }}"'
diff --git a/__tests__/AzureSqlResourceManager.test.ts b/__tests__/AzureSqlResourceManager.test.ts
@@ -44,7 +44,7 @@ describe.only('AzureSqlResourceManager tests', () => {
         let resourceManager = await AzureSqlResourceManager.getResourceManager('testServer.database.windows.net', await AuthorizerFactory.getAuthorizer());
         let server = resourceManager.getSqlServer();
 
-        expect(server!.name).toMatch('testServer');
+        expect(server!.name).toBe('testServer');
         expect(getRequestUrlSpy).toHaveBeenCalledTimes(1);
         expect(beginRequestSpy).toHaveBeenCalledTimes(1);
     });
@@ -116,7 +116,7 @@ describe.only('AzureSqlResourceManager tests', () => {
             let firewallRule = await resourceManager.addFirewallRule('1.2.3.4', '1.2.3.4');
 
             expect(beginRequestSpy).toHaveBeenCalledTimes(1);
-            expect(firewallRule.name).toMatch('FirewallRuleName');
+            expect(firewallRule.name).toBe('FirewallRuleName');
             beginRequestSpy.mockReset();
         }) 
 

diff --git a/__tests__/FirewallManager.test.ts b/__tests__/FirewallManager.test.ts
@@ -40,7 +40,7 @@ describe('FirewallManager tests', () => {
             firewallManager.removeFirewallRule();
 
             expect(removeFirewallRuleSpy).toHaveBeenCalledTimes(1);
-            expect(removeFirewallRuleSpy.mock.calls[0][0].name).toMatch('FirewallRuleName');
+            expect(removeFirewallRuleSpy.mock.calls[0][0].name).toBe('FirewallRuleName');
         });
     });
 

diff --git a/__tests__/SqlConnectionConfig.test.ts b/__tests__/SqlConnectionConfig.test.ts
@@ -23,10 +23,10 @@ describe('SqlConnectionConfig tests', () => {
         it.each(validConnectionStrings)('Input `%s` %s', (connectionStringInput, testDescription, passwordOutput) => {
             const connectionString = new SqlConnectionConfig(connectionStringInput);
 
-            expect(connectionString.Password).toMatch(passwordOutput);
-            expect(connectionString.UserId).toMatch(`user`);
-            expect(connectionString.Database).toMatch('testdb');
-            expect(connectionString.Server).toMatch('test1.database.windows.net');
+            expect(connectionString.Password).toBe(passwordOutput);
+            expect(connectionString.UserId).toBe(`user`);
+            expect(connectionString.Database).toBe('testdb');
+            expect(connectionString.Server).toBe('test1.database.windows.net');
         });
     })
 
@@ -80,17 +80,17 @@ describe('SqlConnectionConfig tests', () => {
         it.each(connectionStrings)('should parse different authentication types successfully', (connectionStringInput, expectedAuthType) => {
             const config = new SqlConnectionConfig(connectionStringInput);
 
-            expect(config.Server).toMatch('test1.database.windows.net');
-            expect(config.Database).toMatch('testdb');
+            expect(config.Server).toBe('test1.database.windows.net');
+            expect(config.Database).toBe('testdb');
             expect(config.EscapedConnectionString.replace(/""/g, '"')).toMatch(connectionStringInput);
-            expect(config.FormattedAuthentication ?? '').toMatch(expectedAuthType);
+            expect(config.FormattedAuthentication ?? '').toBe(expectedAuthType);
             switch (expectedAuthType) {
                 case '':
                 case 'sqlpassword':
                 case 'activedirectorypassword':
                 case 'activedirectoryserviceprincipal': {
-                    expect(config.UserId).toMatch('user');
-                    expect(config.Password).toMatch('placeholder');
+                    expect(config.UserId).toBe('user');
+                    expect(config.Password).toBe('placeholder');
                     break;
                 }
                 case 'activedirectorydefault': {
@@ -114,9 +114,9 @@ describe('SqlConnectionConfig tests', () => {
         it.each(connectionStrings)('should parse server name successfully', (connectionStringInput, expectedServerName, expectedPortNumber) => {
             const config = new SqlConnectionConfig(connectionStringInput);
 
-            expect(config.Server).toMatch(expectedServerName);
-            expect(config.Port?.toString() || '').toMatch(expectedPortNumber);
-            expect(config.Database).toMatch('testdb');
+            expect(config.Server).toBe(expectedServerName);
+            expect(config.Port?.toString() || '').toBe(expectedPortNumber);
+            expect(config.Database).toBe('testdb');
             expect(config.EscapedConnectionString).toMatch(connectionStringInput);
         });
     });

diff --git a/lib/main.js b/lib/main.js
diff --git a/src/SqlConnectionConfig.ts b/src/SqlConnectionConfig.ts
@@ -17,9 +17,14 @@ export default class SqlConnectionConfig {
     }
 
     public get Server(): string {
-        const server = this._parsedConnectionString['data source'] as string;
+        let server = this._parsedConnectionString['data source'] as string;
+        // Remove port number
         if (server?.includes(',')) {
-            return server.split(',')[0].trim();
+            server = server.split(',')[0].trim();
+        }
+        // Remove tcp protocol
+        if (server?.startsWith('tcp:')) {
+            server = server.slice(4).trim();
         }
         return server;
     }

diff --git a/src/SqlUtils.ts b/src/SqlUtils.ts
@@ -75,8 +75,8 @@ export default class SqlUtils {
             };
         }
         catch (error) {
-            console.log(`${error.message}`);
-            console.log(`SqlCmd stderr: ${sqlCmdError}`);
+            core.debug(`${error.message}`);
+            core.debug(`SqlCmd stderr: ${sqlCmdError}`);
             return {
                 success: false,
                 errorMessage: sqlCmdError,

diff --git a/src/main.ts b/src/main.ts
@@ -28,6 +28,7 @@ export default async function run() {
         if (inputs.skipFirewallCheck !== true) {
             const runnerIPAddress = await SqlUtils.detectIPAddress(inputs.connectionConfig);
             if (runnerIPAddress) {
+                core.debug(`Temporarily adding '${runnerIPAddress}' to the firewall of ${inputs.connectionConfig.Server}.`);
                 let azureResourceAuthorizer = await AuthorizerFactory.getAuthorizer();
                 let azureSqlResourceManager = await AzureSqlResourceManager.getResourceManager(inputs.connectionConfig.Server, azureResourceAuthorizer);
                 firewallManager = new FirewallManager(azureSqlResourceManager);