Skip to content

Commit

Permalink
Merge pull request #5497 from snyk/feat/CLI-508_writeDataToFile
Browse files Browse the repository at this point in the history
feat: conditionally write data to file instead of keeping it in memory
  • Loading branch information
j-luong authored Oct 3, 2024
2 parents 24ea937 + 7f11919 commit ede28ab
Show file tree
Hide file tree
Showing 6 changed files with 216 additions and 29 deletions.
11 changes: 6 additions & 5 deletions cliv2/cmd/cliv2/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -528,11 +528,6 @@ func MainWithErrorCode() int {
cliAnalytics.GetInstrumentation().SetStage(instrumentation.DetermineStage(cliAnalytics.IsCiEnvironment()))
cliAnalytics.GetInstrumentation().SetStatus(analytics.Success)

if !globalConfiguration.GetBool(configuration.ANALYTICS_DISABLED) {
defer sendAnalytics(cliAnalytics, globalLogger)
}
defer sendInstrumentation(globalEngine, cliAnalytics.GetInstrumentation(), globalLogger)

setTimeout(globalConfiguration, func() {
os.Exit(constants.SNYK_EXIT_CODE_EX_UNAVAILABLE)
})
Expand Down Expand Up @@ -573,7 +568,13 @@ func MainWithErrorCode() int {
cliAnalytics.GetInstrumentation().SetStatus(analytics.Failure)
}

if !globalConfiguration.GetBool(configuration.ANALYTICS_DISABLED) {
sendAnalytics(cliAnalytics, globalLogger)
}
sendInstrumentation(globalEngine, cliAnalytics.GetInstrumentation(), globalLogger)

// cleanup resources in use
// WARNING: deferred actions will execute AFTER cleanup; only defer if not impacted by this
_, err = globalEngine.Invoke(basic_workflows.WORKFLOWID_GLOBAL_CLEANUP)
if err != nil {
globalLogger.Printf("Failed to cleanup %v", err)
Expand Down
8 changes: 6 additions & 2 deletions cliv2/internal/proxy/proxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ func setup(t *testing.T, baseCache string, version string) configuration.Configu
assert.Nil(t, err)
config := configuration.NewInMemory()
config.Set(configuration.CACHE_PATH, baseCache)
config.Set(basic_workflows.ConfigurationCleanupGlobalTempDirectory, true)
config.Set(basic_workflows.ConfigurationCleanupGlobalCertAuthority, true)
caData, err = basic_workflows.GetGlobalCertAuthority(config, &debugLogger)
assert.Nil(t, err)
return config
Expand All @@ -82,7 +84,9 @@ func setup(t *testing.T, baseCache string, version string) configuration.Configu
func teardown(t *testing.T, baseCache string) {
t.Helper()
err := os.RemoveAll(baseCache)
basic_workflows.CleanupGlobalCertAuthority(&debugLogger)
config := setup(t, "testcache", "1.1.1")

basic_workflows.CleanupGlobalCertAuthority(config, &debugLogger)
assert.Nil(t, err)
}

Expand All @@ -96,7 +100,7 @@ func Test_CleanupCertFile(t *testing.T) {

assert.FileExistsf(t, caData.CertFile, "CertFile exist")

basic_workflows.CleanupGlobalCertAuthority(&debugLogger)
basic_workflows.CleanupGlobalCertAuthority(config, &debugLogger)

assert.NoFileExists(t, caData.CertFile, "CertFile does not exist anymore")
}
Expand Down
23 changes: 21 additions & 2 deletions cliv2/pkg/basic_workflows/globalresources.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,14 @@ var caMutex sync.Mutex

var WORKFLOWID_GLOBAL_CLEANUP workflow.Identifier = workflow.NewWorkflowIdentifier("internal.cleanup")

const (
ConfigurationCleanupGlobalCertAuthority = "internal_cleanup_global_cert_auth_enabled"
ConfigurationCleanupGlobalTempDirectory = "internal_cleanup_global_temp_dir_enabled"
)

func initCleanup(engine workflow.Engine) error {
engine.GetConfiguration().AddDefaultValue(ConfigurationCleanupGlobalCertAuthority, configuration.StandardDefaultValueFunction(true))
engine.GetConfiguration().AddDefaultValue(ConfigurationCleanupGlobalTempDirectory, configuration.StandardDefaultValueFunction(true))
entry, err := engine.Register(WORKFLOWID_GLOBAL_CLEANUP, workflow.ConfigurationOptionsFromFlagset(pflag.NewFlagSet("cleanup", pflag.ContinueOnError)), globalCleanupWorkflow)
if err != nil {
return err
Expand All @@ -40,13 +47,19 @@ func globalCleanupWorkflow(
logger := invocation.GetEnhancedLogger()
config := invocation.GetConfiguration()

CleanupGlobalCertAuthority(logger)
CleanupGlobalCertAuthority(config, logger)
CleanupGlobalTempDirectory(config, logger)

return output, err
}

func CleanupGlobalCertAuthority(debugLogger *zerolog.Logger) {
func CleanupGlobalCertAuthority(config configuration.Configuration, debugLogger *zerolog.Logger) {
enabled := config.GetBool(ConfigurationCleanupGlobalCertAuthority)
if !enabled {
debugLogger.Print("Cleanup of global certificate authority is disabled")
return
}

caMutex.Lock()
defer caMutex.Unlock()
if caSingleton != nil {
Expand Down Expand Up @@ -96,6 +109,12 @@ func GetGlobalCertAuthority(config configuration.Configuration, debugLogger *zer
}

func CleanupGlobalTempDirectory(config configuration.Configuration, debugLogger *zerolog.Logger) {
enabled := config.GetBool(ConfigurationCleanupGlobalTempDirectory)
if !enabled {
debugLogger.Print("Cleanup of global temporary directory is disabled")
return
}

tmpDirectory := config.GetString(configuration.TEMP_DIR_PATH)
err := os.RemoveAll(tmpDirectory)
if err != nil {
Expand Down
55 changes: 36 additions & 19 deletions cliv2/pkg/basic_workflows/globalresources_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,33 +77,50 @@ func Test_ParallelGetGobalCertAuthority(t *testing.T) {

func Test_RestoreCertAuthority(t *testing.T) {
config := configuration.NewInMemory()
// set as we don't call initCleanup()
config.Set(ConfigurationCleanupGlobalCertAuthority, true)
logger := zerolog.New(os.Stderr)

ca1, err := GetGlobalCertAuthority(config, &logger)

assert.NoError(t, err)
assert.FileExists(t, ca1.CertFile)

// case: manual removal of file
os.Remove(ca1.CertFile)
t.Run("manual removal of file", func(t *testing.T) {
os.Remove(ca1.CertFile)

ca2, err := GetGlobalCertAuthority(config, &logger)
assert.NoError(t, err)
assert.FileExists(t, ca2.CertFile)
assert.Equal(t, ca1.CertFile, ca2.CertFile)
ca2, err := GetGlobalCertAuthority(config, &logger)
assert.NoError(t, err)
assert.FileExists(t, ca2.CertFile)
assert.Equal(t, ca1.CertFile, ca2.CertFile)
})

// case: manual removal of file and deletion of cached values
os.Remove(ca1.CertFile)
caSingleton.CertPem = ""
caSingleton.CertFile = ""
t.Run("manual removal of file and deletion of cached values", func(t *testing.T) {
os.Remove(ca1.CertFile)
caSingleton.CertPem = ""
caSingleton.CertFile = ""

ca3, err := GetGlobalCertAuthority(config, &logger)
assert.Error(t, err)
assert.NotEqual(t, ca1.CertFile, ca3.CertFile)
ca2, err := GetGlobalCertAuthority(config, &logger)
assert.Error(t, err)
assert.NotEqual(t, ca1.CertFile, ca2.CertFile)
})

// case: use cleanup function
CleanupGlobalCertAuthority(&logger)
t.Run("use cleanup function", func(t *testing.T) {
CleanupGlobalCertAuthority(config, &logger)

ca4, err := GetGlobalCertAuthority(config, &logger)
assert.NoError(t, err)
assert.FileExists(t, ca4.CertFile)
assert.NotEqual(t, ca1.CertFile, ca4.CertFile)
ca2, err := GetGlobalCertAuthority(config, &logger)
assert.NoError(t, err)
assert.FileExists(t, ca2.CertFile)
assert.NotEqual(t, ca1.CertFile, ca2.CertFile)
})

t.Run("skips cleanup function", func(t *testing.T) {
config.Set(ConfigurationCleanupGlobalCertAuthority, false)
CleanupGlobalCertAuthority(config, &logger)

ca2, err := GetGlobalCertAuthority(config, &logger)
assert.NoError(t, err)
assert.FileExists(t, ca2.CertFile)
assert.NotEqual(t, ca1.CertFile, ca2.CertFile)
})
}
6 changes: 5 additions & 1 deletion test/jest/acceptance/analytics.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,11 @@ describe('analytics module', () => {

expect(code).toBe(0);

const lastRequest = server.popRequest();
const requests = server.getRequests().filter((value) => {
return value.url == '/api/v1/analytics/cli';
});
const lastRequest = requests.at(-1);

expect(lastRequest).toMatchObject({
headers: {
host: `localhost:${port}`,
Expand Down
142 changes: 142 additions & 0 deletions test/jest/acceptance/cli-in-memory-threshold-bytes.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
import * as os from 'os';
import * as path from 'path';
import { resolve } from 'path';

import { runSnykCLI } from '../util/runSnykCLI';
import { matchers } from 'jest-json-schema';
import * as fs from 'fs';

const projectRoot = resolve(__dirname, '../../..');

expect.extend(matchers);

// For golang implementation only
describe('conditionally write data to disk', () => {
const projectWithCodeIssues = resolve(
projectRoot,
'test/fixtures/sast/with_code_issues',
);

const env = {
// Use an org with consistent ignores enabled - uses golang/native workflow
SNYK_API: process.env.TEST_SNYK_API_DEV,
SNYK_TOKEN: process.env.TEST_SNYK_TOKEN_DEV,
SNYK_LOG_LEVEL: 'trace',
INTERNAL_CLEANUP_GLOBAL_TEMP_DIR_ENABLED: 'false', // disable cleanup of temp dir for testing
};

jest.setTimeout(60000);

// GAF automatically creates the temp dir
// GAF will also automatically deletes it
// but we disable this for testing
const tempDirName = `tempDir-${Date.now()}`;
const tempDirPath = path.join(os.tmpdir(), tempDirName);

afterEach(async () => {
// delete tempDirPath
try {
await fs.promises.rm(tempDirPath, { recursive: true, force: true });
} catch {
console.warn('teardown failed');
}
});

describe('when temp dir and threshold are set', () => {
const tempDirVars = {
SNYK_TMP_PATH: tempDirPath,
INTERNAL_IN_MEMORY_THRESHOLD_BYTES: '1',
};

it('should write to temp dir if payload is bigger than threshold', async () => {
await runSnykCLI(`code test ${projectWithCodeIssues}`, {
env: {
...process.env,
...env,
...tempDirVars,
},
});

// assert that tempDirPath exists
await expect(
fs.promises.access(tempDirPath, fs.constants.F_OK),
).resolves.toBeUndefined();

// assert that tempDirPath contains workflow files
const files = await fs.promises.readdir(tempDirPath);
const workflowFiles = files.filter((file) => file.includes('workflow.'));
expect(workflowFiles.length).toBeGreaterThan(0);
});
});

describe('when only threshold is set', () => {
const tempDirVars = {
INTERNAL_IN_MEMORY_THRESHOLD_BYTES: '1',
INTERNAL_CLEANUP_GLOBAL_TEMP_DIR_ENABLED: 'true', // re-enable as we're not setting the temp dir, and we want to ensure we cleanup
};

it('should write to default temp dir if payload is bigger than threshold', async () => {
await runSnykCLI(`code test ${projectWithCodeIssues}`, {
env: {
...process.env,
...env,
...tempDirVars,
},
});

// note we can't determine whether we write to disk or memory without inspecting logs
// GAF uses the default OS cache dir to write to, which we cannot access in the test

// assert that tempDirPath does not exist
await expect(
fs.promises.access(tempDirPath, fs.constants.F_OK),
).rejects.toThrow();
});
});

describe('when temp dir and threshold are NOT set', () => {
const tempDirVars = {
INTERNAL_CLEANUP_GLOBAL_TEMP_DIR_ENABLED: 'true', // re-enable as we're not setting the temp dir, and we want to ensure we cleanup
};

it('should use 512MB as default threshold', async () => {
await runSnykCLI(`code test ${projectWithCodeIssues}`, {
env: {
...process.env,
...env,
...tempDirVars,
},
});

// note we can't determine whether we write to disk or memory without inspecting logs
// GAF uses the default OS cache dir to write to, which we cannot access in the test

// assert that tempDirPath does not exist
await expect(
fs.promises.access(tempDirPath, fs.constants.F_OK),
).rejects.toThrow();
});
});

describe('when feature is disabled', () => {
const tempDirVars = {
INTERNAL_IN_MEMORY_THRESHOLD_BYTES: '-1',
INTERNAL_CLEANUP_GLOBAL_TEMP_DIR_ENABLED: 'true', // re-enable as we're not setting the temp dir, and we want to ensure we cleanup
};

it('should keep payload memory', async () => {
await runSnykCLI(`code test ${projectWithCodeIssues}`, {
env: {
...process.env,
...env,
...tempDirVars,
},
});

// assert that tempDirPath does not exist
await expect(
fs.promises.access(tempDirPath, fs.constants.F_OK),
).rejects.toThrow();
});
});
});

0 comments on commit ede28ab

Please sign in to comment.