Skip to content

Commit

Permalink
src: start sending get request with query params
Browse files Browse the repository at this point in the history
We are incorrectly using formData in a get request. To move
away from this we send both query params and formData until
the server is fully upgraded. After which we can stop sending
formData.
  • Loading branch information
adityamaru committed Dec 9, 2024
1 parent 0186286 commit 0f99a0b
Show file tree
Hide file tree
Showing 8 changed files with 131 additions and 58 deletions.
2 changes: 0 additions & 2 deletions .github/workflows/verify-build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@ jobs:

- name: Check for changes
run: |
# Ignore yarn.lock changes since we're using npm
git update-index --assume-unchanged yarn.lock || true
if [[ -n "$(git status --porcelain)" ]]; then
echo "::error::Build generated new changes. Please commit the generated files."
git status
Expand Down
2 changes: 1 addition & 1 deletion dist/index.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/index.js.map

Large diffs are not rendered by default.

48 changes: 24 additions & 24 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
"@docker/actions-toolkit": "0.37.1",
"@iarna/toml": "^2.2.5",
"axios-retry": "^4.5.0",
"form-data": "^4.0.1",
"handlebars": "^4.7.7",
"portfinder": "^1.0.32"
},
Expand Down
54 changes: 54 additions & 0 deletions src/__tests__/setup-builder.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import * as reporter from '../reporter';
import { getStickyDisk } from '../setup_builder';
import FormData from 'form-data';

jest.mock('../reporter');

describe('getStickyDisk', () => {
const mockGet = jest.fn();

beforeEach(() => {
jest.resetAllMocks();
process.env.GITHUB_REPO_NAME = 'test-repo';
process.env.BLACKSMITH_REGION = 'test-region';
process.env.BLACKSMITH_INSTALLATION_MODEL_ID = 'test-model';
process.env.VM_ID = 'test-vm';

(reporter.createBlacksmithAgentClient as jest.Mock).mockResolvedValue({});
(reporter.get as jest.Mock).mockImplementation(mockGet);
mockGet.mockResolvedValue({
data: {
expose_id: 'test-expose-id',
disk_identifier: 'test-device'
}
});
});

it('sets both FormData and query parameters correctly', async () => {
const appendSpy = jest.spyOn(FormData.prototype, 'append');

await getStickyDisk();

expect(mockGet).toHaveBeenCalledTimes(1);
const [, url, formData] = mockGet.mock.calls[0];

// Verify query parameters
expect(url).toContain('stickyDiskKey=test-repo');
expect(url).toContain('region=test-region');
expect(url).toContain('installationModelID=test-model');
expect(url).toContain('vmID=test-vm');

// Verify FormData is correct type
expect(formData instanceof FormData).toBeTruthy();

// Verify the headers are set correctly
const headers = formData.getHeaders();
expect(headers['content-type']).toContain('multipart/form-data');

// Verify the correct fields were appended
expect(appendSpy).toHaveBeenCalledWith('stickyDiskKey', 'test-repo');
expect(appendSpy).toHaveBeenCalledWith('region', 'test-region');
expect(appendSpy).toHaveBeenCalledWith('installationModelID', 'test-model');
expect(appendSpy).toHaveBeenCalledWith('vmID', 'test-vm');
});
});
34 changes: 20 additions & 14 deletions src/reporter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as core from '@actions/core';
import axios, {AxiosError, AxiosInstance, AxiosResponse, AxiosStatic } from 'axios';
import axiosRetry from 'axios-retry';
import {ExportRecordResponse} from '@docker/actions-toolkit/lib/types/buildx/history';
import FormData from 'form-data';

// Configure base axios instance for Blacksmith API.
const createBlacksmithAPIClient = () => {
Expand Down Expand Up @@ -33,7 +34,11 @@ const createBlacksmithAPIClient = () => {
export async function createBlacksmithAgentClient(): Promise<AxiosInstance> {
const stickyDiskMgrUrl = 'http://192.168.127.1:5556';
const client = axios.create({
baseURL: stickyDiskMgrUrl
baseURL: stickyDiskMgrUrl,
headers: {
Authorization: `Bearer ${process.env.BLACKSMITH_STICKYDISK_TOKEN}`,
'X-Github-Repo-Name': process.env.GITHUB_REPO_NAME || '',
}
});

axiosRetry(client, {
Expand Down Expand Up @@ -78,11 +83,7 @@ export async function reportBuildCompleted(exportRes?: ExportRecordResponse, bla
formData.append('exposeID', exposeId || '');
formData.append('stickyDiskKey', process.env.GITHUB_REPO_NAME || '');

await agentClient.post('/stickydisks', formData, {
headers: {
'Content-Type': 'multipart/form-data'
}
});
await post(agentClient, '/stickydisks', formData);

// Report success to Blacksmith API
const requestOptions = {
Expand Down Expand Up @@ -131,11 +132,7 @@ export async function reportBuildFailed(dockerBuildId: string | null, dockerBuil
formData.append('exposeID', exposeId || '');
formData.append('stickyDiskKey', process.env.GITHUB_REPO_NAME || '');

await blacksmithAgentClient.post('/stickydisks', formData, {
headers: {
'Content-Type': 'multipart/form-data'
}
});
await post(blacksmithAgentClient, '/stickydisks', formData);

// Report failure to Blacksmith API
const requestOptions = {
Expand Down Expand Up @@ -180,9 +177,18 @@ export async function get(client: AxiosInstance, url: string, formData: FormData
return await client.get(url, {
...(formData && {data: formData}),
headers: {
Authorization: `Bearer ${process.env.BLACKSMITH_STICKYDISK_TOKEN}`,
'X-Github-Repo-Name': process.env.GITHUB_REPO_NAME || '',
'Content-Type': 'multipart/form-data'
...client.defaults.headers.common,
...(formData && {'Content-Type': 'multipart/form-data'})
},
signal: options?.signal
});
}

export async function post(client: AxiosInstance, url: string, formData: FormData | null, options?: {signal?: AbortSignal}): Promise<AxiosResponse> {
return await client.post(url, formData, {
headers: {
...client.defaults.headers.common,
...(formData && { 'Content-Type': 'multipart/form-data' }),
},
signal: options?.signal
});
Expand Down
46 changes: 30 additions & 16 deletions src/setup_builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {exec} from 'child_process';
import {promisify} from 'util';
import * as TOML from '@iarna/toml';
import * as reporter from './reporter';
import FormData from 'form-data';

const mountPoint = '/var/lib/buildkit';
const execAsync = promisify(exec);
Expand Down Expand Up @@ -143,25 +144,38 @@ async function getDiskSize(device: string): Promise<number> {
}
}

async function getStickyDisk(options?: {signal?: AbortSignal}): Promise<{expose_id: string; device: string}> {
export async function getStickyDisk(options?: {signal?: AbortSignal}): Promise<{expose_id: string; device: string}> {
const client = await reporter.createBlacksmithAgentClient();
const formData = new FormData();
// TODO(adityamaru): Support a stickydisk-per-build flag that will namespace the stickydisks by Dockerfile.
// For now, we'll use the repo name as the stickydisk key.
const repoName = process.env.GITHUB_REPO_NAME || '';
if (repoName === '') {

// Prepare data for both FormData and query params
const stickyDiskKey = process.env.GITHUB_REPO_NAME || '';
if (stickyDiskKey === '') {
throw new Error('GITHUB_REPO_NAME is not set');
}
formData.append('stickyDiskKey', repoName);
formData.append('region', process.env.BLACKSMITH_REGION || 'eu-central');
formData.append('installationModelID', process.env.BLACKSMITH_INSTALLATION_MODEL_ID || '');
formData.append('vmID', process.env.VM_ID || '');
core.debug(`Getting sticky disk for ${repoName}`);
core.debug('FormData contents:');
for (const pair of formData.entries()) {
core.debug(`${pair[0]}: ${pair[1]}`);
}
const response = await reporter.get(client, '/stickydisks', formData, options);
const region = process.env.BLACKSMITH_REGION || 'eu-central';
const installationModelID = process.env.BLACKSMITH_INSTALLATION_MODEL_ID || '';
const vmID = process.env.VM_ID || '';

// Create FormData (for backwards compatibility).
// TODO(adityamaru): Remove this once all of our VM agents are reading query params.
const formData = new FormData();
formData.append('stickyDiskKey', stickyDiskKey);
formData.append('region', region);
formData.append('installationModelID', installationModelID);
formData.append('vmID', vmID);

// Create query params string.
const queryParams = new URLSearchParams({
stickyDiskKey,
region,
installationModelID,
vmID
}).toString();

core.debug(`Getting sticky disk for ${stickyDiskKey}`);

// Send request with both FormData and query params
const response = await reporter.get(client, `/stickydisks?${queryParams}`, formData, options);
const exposeId = response.data?.expose_id || '';
const device = response.data?.disk_identifier || '';
return {expose_id: exposeId, device: device};
Expand Down

0 comments on commit 0f99a0b

Please sign in to comment.