Skip to content

Commit

Permalink
refactor(kubernetes): simplify the forward config interface (podman-d…
Browse files Browse the repository at this point in the history 
…esktop#9787)

* refactor(kubernetes): simplify the forward config interface

Signed-off-by: axel7083 <[email protected]>

* fix: kube ports list tests

Signed-off-by: axel7083 <[email protected]>

* fix: remove unnecessary check

Signed-off-by: axel7083 <[email protected]>

* fix: remove unnecessary check

Signed-off-by: axel7083 <[email protected]>

* fix: remove unnecessary documentation param

Signed-off-by: axel7083 <[email protected]>

* fix: test expect arguments

Signed-off-by: axel7083 <[email protected]>

* fix: remove getPortForwardKey as suggested by @benoitf

Signed-off-by: axel7083 <[email protected]>

---------

Signed-off-by: axel7083 <[email protected]>
  • Loading branch information
@axel7083
axel7083 authored Nov 7, 2024
1 parent 58a46fd commit 02644a3
Show file tree
Hide file tree
Showing 18 changed files with 140 additions and 320 deletions.
4 changes: 2 additions & 2 deletions packages/api/src/kubernetes-port-forward-model.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,9 @@ export interface ForwardConfig {
kind: WorkloadKind;

/**
* The list of port mappings.
* The port mapping.
*/
forwards: PortMapping[];
forward: PortMapping;
}

/**
Expand Down
6 changes: 3 additions & 3 deletions packages/main/src/plugin/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ import type { ImageInspectInfo } from '/@api/image-inspect-info.js';
import type { ImageSearchOptions, ImageSearchResult, ImageTagsListOptions } from '/@api/image-registry.js';
import type { KubeContext } from '/@api/kubernetes-context.js';
import type { ContextGeneralState, ResourceName } from '/@api/kubernetes-contexts-states.js';
import type { ForwardOptions, PortMapping, UserForwardConfig } from '/@api/kubernetes-port-forward-model.js';
import type { ForwardOptions, UserForwardConfig } from '/@api/kubernetes-port-forward-model.js';
import type { ManifestCreateOptions, ManifestInspectInfo, ManifestPushOptions } from '/@api/manifest-info.js';
import type { NetworkInspectInfo } from '/@api/network-info.js';
import type { NotificationCard, NotificationCardOptions } from '/@api/notification.js';
Expand Down Expand Up @@ -2477,8 +2477,8 @@ export class PluginSystem {

this.ipcHandle(
'kubernetes-client:deletePortForward',
async (_listener, config: UserForwardConfig, mapping?: PortMapping): Promise<void> => {
return kubernetesClient.deletePortForward(config, mapping);
async (_listener, config: UserForwardConfig): Promise<void> => {
return kubernetesClient.deletePortForward(config);
},
);

Expand Down
6 changes: 3 additions & 3 deletions packages/main/src/plugin/kubernetes/kubernetes-client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ import type { KubernetesPortForwardService } from '/@/plugin/kubernetes/kubernet
import { KubernetesPortForwardServiceProvider } from '/@/plugin/kubernetes/kubernetes-port-forward-service.js';
import type { KubeContext } from '/@api/kubernetes-context.js';
import type { ContextGeneralState, ResourceName } from '/@api/kubernetes-contexts-states.js';
import type { ForwardOptions, PortMapping, UserForwardConfig } from '/@api/kubernetes-port-forward-model.js';
import type { ForwardOptions, UserForwardConfig } from '/@api/kubernetes-port-forward-model.js';
import type { V1Route } from '/@api/openshift-types.js';

import type { ApiSenderType } from '../api.js';
Expand Down Expand Up @@ -1772,7 +1772,7 @@ export class KubernetesClient {
return newConfig;
}

public async deletePortForward(config: UserForwardConfig, mapping?: PortMapping): Promise<void> {
return this.ensurePortForwardService().deleteForward(config, mapping);
public async deletePortForward(config: UserForwardConfig): Promise<void> {
return this.ensurePortForwardService().deleteForward(config);
}
}
49 changes: 6 additions & 43 deletions packages/main/src/plugin/kubernetes/kubernetes-port-forward-connection.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -696,10 +696,7 @@ describe('PortForwardConnectionService', () => {
kind: WorkloadKind.POD,
name: 'test-pod',
namespace: 'default',
forwards: [
{ localPort: 3000, remotePort: 80 },
{ localPort: 3001, remotePort: 8080 },
],
forward: { localPort: 3000, remotePort: 80 },
};

const pod: V1Pod = {
Expand All @@ -711,19 +708,17 @@ describe('PortForwardConnectionService', () => {
mockCoreV1Api.readNamespacedPod.mockResolvedValueOnce(pod);

const disposable1 = new MockDisposable();
const disposable2 = new MockDisposable();

vi.spyOn(service, 'performForward').mockResolvedValueOnce(disposable1).mockResolvedValueOnce(disposable2);
vi.spyOn(service, 'performForward').mockResolvedValueOnce(disposable1);

const disposable = await service.startForward(forwardConfig);

expect(service.performForward).toHaveBeenCalledTimes(2);
expect(service.performForward).toHaveBeenCalledOnce();
expect(disposable.dispose).toBeInstanceOf(Function);

disposable.dispose();

expect(disposable1.dispose).toHaveBeenCalled();
expect(disposable2.dispose).toHaveBeenCalled();
});

test('should start port forwarding on specified mapping', async () => {
Expand All @@ -733,7 +728,7 @@ describe('PortForwardConnectionService', () => {
kind: WorkloadKind.POD,
name: 'test-pod',
namespace: 'default',
forwards: [{ localPort: 3000, remotePort: 80 }, mapping],
forward: mapping,
};

const pod: V1Pod = {
Expand All @@ -748,7 +743,7 @@ describe('PortForwardConnectionService', () => {

vi.spyOn(service, 'performForward').mockResolvedValueOnce(disposable1);

const disposable = await service.startForward(forwardConfig, mapping);
const disposable = await service.startForward(forwardConfig);

expect(service.performForward).toHaveBeenCalledTimes(1);
expect(disposable.dispose).toBeInstanceOf(Function);
Expand All @@ -760,7 +755,7 @@ describe('PortForwardConnectionService', () => {
kind: WorkloadKind.POD,
name: 'test-pod',
namespace: 'default',
forwards: [{ localPort: 3000, remotePort: 80 }],
forward: { localPort: 3000, remotePort: 80 },
};

const pod: V1Pod = {
Expand All @@ -777,37 +772,5 @@ describe('PortForwardConnectionService', () => {

expect(service.performForward).toHaveBeenCalledTimes(1);
});

test('should dispose all successful forwards if any fail', async () => {
const forwardConfig: ForwardConfig = {
id: 'fake-id',
kind: WorkloadKind.POD,
name: 'test-pod',
namespace: 'default',
forwards: [
{ localPort: 3000, remotePort: 80 },
{ localPort: 3001, remotePort: 8080 },
],
};

const pod: V1Pod = {
apiVersion: 'v1',
kind: 'Pod',
metadata: { name: 'test-pod', namespace: 'default' },
};

mockCoreV1Api.readNamespacedPod.mockResolvedValueOnce(pod);

const disposable1 = new MockDisposable();

vi.spyOn(service, 'performForward')
.mockResolvedValueOnce(disposable1)
.mockRejectedValueOnce(new Error('Failed to forward port'));

await expect(service.startForward(forwardConfig)).rejects.toThrow('Failed to forward port');

expect(service.performForward).toHaveBeenCalledTimes(2);
expect(disposable1.dispose).toHaveBeenCalled();
});
});
});
26 changes: 3 additions & 23 deletions packages/main/src/plugin/kubernetes/kubernetes-port-forward-connection.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,38 +56,18 @@ export class PortForwardConnectionService {
/**
* Starts the port forwarding based on the provided configuration.
* @param config - The forwarding configuration.
* @param mapping - The mapping to start, if not specified all {@link ForwardConfig#forwards} will be started
* @returns A promise that resolves to a disposable resource to stop the forwarding.
* @throws If any of the port forwarding fail.
*/
async startForward(config: ForwardConfig, mapping?: PortMapping): Promise<IDisposable> {
async startForward(config: ForwardConfig): Promise<IDisposable> {
if (this.configRequirementsChecker) {
await this.configRequirementsChecker.checkRuntimeRequirements(config);
}

const resource = await this.getWorkloadResource(config.kind, config.name, config.namespace);

const results = await Promise.allSettled(
(mapping ? [mapping] : config.forwards).map(async forward => {
const forwardSetup = await this.getForwardingSetup(resource, forward);
return this.performForward(forwardSetup);
}),
);

const successForwards = results.filter(
result => result.status === 'fulfilled',
) as PromiseFulfilledResult<IDisposable>[];
const failedForwards = results.filter(result => result.status === 'rejected') as PromiseRejectedResult[];

if (failedForwards.length > 0) {
successForwards.forEach(attempt => attempt.value.dispose());

const reasons = failedForwards.map(attempt => attempt.reason).join('\n');
throw new Error(reasons);
}

const disposables: IDisposable[] = successForwards.map(attempt => attempt.value);
return Disposable.from(...disposables);
const forwardSetup = await this.getForwardingSetup(resource, config.forward);
return this.performForward(forwardSetup);
}

/**
Expand Down
86 changes: 2 additions & 84 deletions packages/main/src/plugin/kubernetes/kubernetes-port-forward-service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,30 +76,14 @@ describe('KubernetesPortForwardService', () => {
name: 'test-name',
namespace: 'test-namespace',
kind: WorkloadKind.POD,
forwards: [{ localPort: 8080, remotePort: 80 }],
forward: { localPort: 8080, remotePort: 80 },
};

const sampleUserForwardConfig: UserForwardConfig = {
...sampleForwardConfig,
displayName: 'test-display-name',
};

const complexForwardConfig: ForwardConfig = {
id: 'fake-id',
name: 'test-name',
namespace: 'test-namespace',
kind: WorkloadKind.POD,
forwards: [
{ localPort: 8080, remotePort: 80 },
{ localPort: 9090, remotePort: 90 },
],
};

const complexUserForwardConfig: UserForwardConfig = {
...complexForwardConfig,
displayName: 'complex-display-name',
};

beforeEach(() => {
vi.clearAllMocks();
mockConfigManagementService = {
Expand All @@ -123,7 +107,7 @@ describe('KubernetesPortForwardService', () => {

test('should create a forward configuration', async () => {
vi.mocked(mockConfigManagementService.listForwards).mockResolvedValue([]);
const forward = sampleUserForwardConfig.forwards[0];
const forward = sampleUserForwardConfig.forward;
if (!forward) throw new Error('undefined forward');

const result = await service.createForward({
Expand All @@ -150,15 +134,6 @@ describe('KubernetesPortForwardService', () => {
expect(mockConfigManagementService.listForwards).toHaveBeenCalled();
});

test('should start port forwarding for a given configuration', async () => {
const disposable = await service.startForward(sampleForwardConfig);
expect(disposable).toHaveProperty('dispose');
expect(mockForwardingConnectionService.startForward).toHaveBeenCalledWith(
sampleForwardConfig,
sampleForwardConfig.forwards[0],
);
});

test('should dispose for a given configuration', async () => {
const disposeMock = vi.fn();
vi.mocked(mockForwardingConnectionService.startForward).mockResolvedValue({
Expand All @@ -171,61 +146,4 @@ describe('KubernetesPortForwardService', () => {
service.dispose();
expect(disposeMock).toHaveBeenCalled();
});

test('delete a multiple mapping should dispose all disposables', async () => {
const disposeMock = vi.fn();
vi.mocked(mockForwardingConnectionService.startForward).mockResolvedValue({
dispose: disposeMock,
});
await service.startForward(complexForwardConfig);

await service.deleteForward(complexUserForwardConfig);

expect(disposeMock).toHaveBeenCalledTimes(2);
expect(mockConfigManagementService.deleteForward).toHaveBeenCalledWith(complexUserForwardConfig);
expect(mockConfigManagementService.updateForward).not.toHaveBeenCalled();
});

test('delete a mapping in a multiple mapping should update config without the one removed', async () => {
const disposeMock = vi.fn();
vi.mocked(mockForwardingConnectionService.startForward).mockResolvedValue({
dispose: disposeMock,
});
await service.startForward(complexForwardConfig);

await service.deleteForward(complexUserForwardConfig, complexUserForwardConfig.forwards[0]);

expect(disposeMock).toHaveBeenCalledTimes(1);
expect(mockConfigManagementService.deleteForward).not.toHaveBeenCalled();
expect(mockConfigManagementService.updateForward).toHaveBeenCalledWith(complexUserForwardConfig, {
...complexUserForwardConfig,
forwards: [complexUserForwardConfig.forwards[1]],
});
});

test('calling start second time with an updated version should not start the same mapping twice', async () => {
const mappingA = complexForwardConfig.forwards[0];
const mappingB = complexForwardConfig.forwards[1];

if (!mappingA || !mappingB) throw new Error('undefined mapping');

await service.startForward({
...complexForwardConfig,
forwards: [mappingA],
});

expect(mockForwardingConnectionService.startForward).toHaveBeenCalledWith(expect.anything(), mappingA);
expect(mockForwardingConnectionService.startForward).not.toHaveBeenCalledWith(expect.anything(), mappingB);

await service.startForward({
...complexForwardConfig,
forwards: [mappingA, mappingB],
});

expect(mockForwardingConnectionService.startForward).toHaveBeenNthCalledWith(1, expect.anything(), mappingA);
expect(mockForwardingConnectionService.startForward).toHaveBeenNthCalledWith(2, expect.anything(), mappingB);

// ensure only called twice, once for mappingA one for mappingB
expect(mockForwardingConnectionService.startForward).toHaveBeenCalledTimes(2);
});
});
Loading

0 comments on commit 02644a3

Please sign in to comment.