-
Notifications
You must be signed in to change notification settings - Fork 37
support for private AKS clusters #181
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,8 @@ | ||||||||||||||||||
| import * as core from '@actions/core' | ||||||||||||||||||
| import * as exec from '@actions/exec' | ||||||||||||||||||
| import * as fs from 'fs' | ||||||||||||||||||
| import * as os from 'os' | ||||||||||||||||||
| import * as path from 'path' | ||||||||||||||||||
| import { | ||||||||||||||||||
| CoreV1ApiCreateNamespacedSecretRequest, | ||||||||||||||||||
| CoreV1ApiDeleteNamespacedSecretRequest, | ||||||||||||||||||
|
|
@@ -51,7 +55,60 @@ export function buildContainerRegistryDockerConfigJSON( | |||||||||||||||||
| } | ||||||||||||||||||
| return dockerConfigJson //Buffer.from(JSON.stringify(dockerConfigJson)).toString('base64'); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| export async function runKubectlViaAz( | ||||||||||||||||||
| secret: V1Secret, | ||||||||||||||||||
| namespace: string, | ||||||||||||||||||
| secretName: string | ||||||||||||||||||
| ) { | ||||||||||||||||||
| const resourceGroup = core.getInput('cluster-resource-group') | ||||||||||||||||||
| const clusterName = core.getInput('cluster-name') | ||||||||||||||||||
| if (!resourceGroup || !clusterName) { | ||||||||||||||||||
| throw new Error( | ||||||||||||||||||
| 'cluster-resource-group and cluster-name are required for private cluster support' | ||||||||||||||||||
| ) | ||||||||||||||||||
| } | ||||||||||||||||||
| // Write secret to temp file | ||||||||||||||||||
| const tempFile = path.join(os.tmpdir(), `secret-${secretName}.json`) | ||||||||||||||||||
|
ReinierCC marked this conversation as resolved.
|
||||||||||||||||||
| const tempFile = path.join(os.tmpdir(), `secret-${secretName}.json`) |
Copilot
AI
Aug 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line redeclares the tempFile variable that was already declared on line 71, which will cause a compilation error in TypeScript due to duplicate variable declaration in the same scope.
| const tempFile = path.join(os.tmpdir(), `secret-${secretName}.json`) |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assignment to variable tempFile, which is declared constant.
| // Write secret to temp file | |
| const tempFile = path.join(os.tmpdir(), `secret-${secretName}.json`) |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assignment to variable tempFile, which is declared constant.
| // Write secret to temp file | |
| const tempFile = path.join(os.tmpdir(), `secret-${secretName}.json`) |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable tempDir is declared on line 73 but never has a corresponding declaration statement. This will cause a reference error since tempDir is used in the finally block (lines 107-109) but is only accessible within the scope after line 73. The tempDir variable needs to be declared before line 73 (e.g., let tempDir: string;) to be accessible in the finally block.
| const tempFile = path.join(os.tmpdir(), `secret-${secretName}.json`) | |
| // Write secret to temp file securely | |
| const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'secret-')); | |
| let tempDir: string; | |
| // Write secret to temp file securely | |
| tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'secret-')); |
Copilot
AI
Jul 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The secretName and namespace parameters are directly interpolated into the shell command without sanitization, which could lead to command injection vulnerabilities if these values contain malicious characters.
| `kubectl delete secret ${secretName} -n ${namespace} --ignore-not-found` | |
| 'kubectl', | |
| 'delete', | |
| 'secret', | |
| secretName, | |
| '-n', | |
| namespace, | |
| '--ignore-not-found' |
Copilot
AI
Aug 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Direct string interpolation in shell commands can lead to command injection vulnerabilities if secretName or namespace contain malicious characters. Consider validating these inputs or using a safer command construction method.
Copilot
AI
Jul 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The namespace parameter is directly interpolated into the shell command without sanitization, which could lead to command injection vulnerabilities if the namespace value contains malicious characters.
| `kubectl apply -f - -n ${namespace}`, | |
| 'kubectl apply -f -', | |
| '--namespace', | |
| namespace, |
Copilot
AI
Aug 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Direct string interpolation in shell commands can lead to command injection vulnerabilities if namespace contains malicious characters. Consider validating the namespace input or using a safer command construction method.
Copilot
AI
Aug 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The kubectl command uses -f - (stdin) but then specifies --file parameter with the temp file path. This is inconsistent - either use -f - with stdin or -f ${tempFile} with the file parameter, but not both.
| `kubectl apply -f - -n ${namespace}`, | |
| `kubectl apply -f ${secretName}.json -n ${namespace}`, |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cleanup logic is redundant. Lines 103-105 attempt to delete tempFile individually, but lines 107-109 already delete the entire tempDir recursively with { recursive: true, force: true }, which would remove tempFile as well. The individual file deletion (lines 103-105) is unnecessary and can be removed.
| // Remove temp file if it exists | |
| if (fs.existsSync(tempFile)) { | |
| fs.unlinkSync(tempFile) | |
| } |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checkClusterContext() function is still called unconditionally at the start of run(), which checks for the KUBECONFIG environment variable. However, when use-invoke-command is true, the KUBECONFIG is not needed since the action uses az aks command invoke instead. This will cause the action to fail for private clusters even when the invoke command path is selected. Consider making this check conditional or moving it to only execute when the Kubernetes API path is used.
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -1,7 +1,7 @@ | ||||
| import {kStringMaxLength} from 'buffer' | ||||
| import * as fs from 'fs' | ||||
| import * as path from 'path' | ||||
|
|
||||
| import * as exec from '@actions/exec' | ||||
| import * as core from '@actions/core' | ||||
|
|
||||
| const k8s = require('@kubernetes/client-node') | ||||
|
|
@@ -176,3 +176,57 @@ describe('buildContainerRegistryDockerConfigJSON', () => { | |||
| expect(dockerConfigJson).toEqual(exptectedDockerConfigJsonNoEmail) | ||||
| }) | ||||
| }) | ||||
|
|
||||
| describe('runKubectlViaAz', () => { | ||||
| const mockSecret = { | ||||
| apiVersion: 'v1', | ||||
| kind: 'Secret', | ||||
| metadata: {name: 'test-secret', namespace: 'test-ns'}, | ||||
| type: 'Opaque', | ||||
| data: {foo: 'YmFy'} | ||||
| } | ||||
|
|
||||
| beforeEach(() => { | ||||
| jest.spyOn(fs, 'writeFileSync').mockImplementation(() => {}) | ||||
| jest.spyOn(fs, 'unlinkSync').mockImplementation(() => {}) | ||||
| jest.spyOn(exec, 'exec').mockResolvedValue(0) | ||||
| }) | ||||
|
Comment on lines
+189
to
+193
|
||||
|
|
||||
| afterEach(() => jest.restoreAllMocks()) | ||||
|
|
||||
| it('calls az aks command invoke for delete and apply', async () => { | ||||
| jest | ||||
| .spyOn(core, 'getInput') | ||||
| .mockImplementation((name: string) => | ||||
| name === 'cluster-resource-group' | ||||
| ? 'rg' | ||||
| : name === 'cluster-name' | ||||
| ? 'aks' | ||||
| : '' | ||||
| ) | ||||
| const {runKubectlViaAz} = require('../src/run') | ||||
|
||||
| const {runKubectlViaAz} = require('../src/run') |
Copilot
AI
Dec 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test only verifies that exec.exec was called with certain arguments but doesn't verify:
- That it was called exactly twice (once for delete, once for apply)
- The complete command arguments including the kubectl commands
- That the temp file operations occurred in the correct order
Consider using toHaveBeenCalledTimes(2) and more specific matchers to verify both the delete and apply commands were executed with the correct full argument lists.
Copilot
AI
Aug 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using require() inside test functions can lead to module caching issues and makes it harder to mock dependencies consistently. Consider importing the function at the top level or using dynamic imports with proper cleanup.
| const {runKubectlViaAz} = require('../src/run') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The temporary file name is predictable and could lead to race conditions or security issues. Consider using a more secure temporary file creation method with random suffixes or proper file permissions.