Repository: cdk8s-team/cdk8s-plus (ts)

Issue:
`exposeViaService` does not expose port names
<!--Please make sure to read the "Reporting Issues" section of the contribution guide before submitting an issue
https://github.com/cdk8s-team/cdk8s/blob/master/CONTRIBUTING.md#reporting-issues
-->

### Description of the bug:
Given a port definition in a `Deployment`: 
```
ports: [
    { number: 3000, name: 'http' },
    { number: 8080, name: 'dashboard' }
]
```
`deployment.exposeViaService()` produces: 
```
  ports:
    - port: 3000
      targetPort: 3000
    - port: 8080
      targetPort: 8080
```
which is incorrect according to [Kubernetes documentation](https://kubernetes.io/docs/concepts/services-networking/service/#multi-port-services): 

> When using multiple ports for a Service, you must give all of your ports names so that these are unambiguous.

Since the names are already there in the definition, I would have expected them to be applied automatically. In theory, this should be as easy as replacing the line:
```
    const ports: service.ServicePort[] = options.ports ?? myPorts.map(p => ({ port: p.number, targetPort: p.number, protocol: p.protocol }));
```
with 
```
    const ports: service.ServicePort[] = options.ports ?? myPorts.map(p => ({ port: p.number, targetPort: p.number, protocol: p.protocol, name: p.name }));
```
---

This is :bug: Bug Report

Pull request:
fix(deployment): `exposeViaService` does not expose port names
Fixes #1981 

Currently, `deployment.exposeViaService()` does not return the names of the ports; [Kubernetes documentation]([Kubernetes documentation](https://kubernetes.io/docs/concepts/services-networking/service/#multi-port-services)) states that this is incorrect behavior: "When using multiple ports for a Service, you must give all of your ports names so that these are unambiguous."

This adds port names and a check in the `exposeViaService` function of `Deployment` by throwing an error if port names are not specified when there are multiple ports. 

Patch:
diff --git a/src/deployment.ts b/src/deployment.ts
index 1c784e41..b0a8eefd 100644
--- a/src/deployment.ts
+++ b/src/deployment.ts
@@ -177,12 +177,19 @@ export class Deployment extends workload.Workload implements IScalable {
   public exposeViaService(options: DeploymentExposeViaServiceOptions = {}): service.Service {
     const myPorts = container.extractContainerPorts(this);
     const myPortNumbers = myPorts.map(p => p.number);
-    const ports: service.ServicePort[] = options.ports ?? myPorts.map(p => ({ port: p.number, targetPort: p.number, protocol: p.protocol }));
+    const ports: service.ServicePort[] = options.ports ?? myPorts.map(p => ({
+      port: p.number, targetPort: p.number, protocol: p.protocol, name: p.name,
+    }));
     if (ports.length === 0) {
       throw new Error(`Unable to expose deployment ${this.name} via a service: `
         + 'Deployment port cannot be determined.'
         + 'Either pass \'ports\', or configure ports on the containers of the deployment');
     }
+    const portNames = ports.map(p => p.name);
+    if (ports.length > 1 && portNames.includes(undefined)) {
+      throw new Error(`Unable to expose deployment ${this.name} via a service: `
+        + 'When using multiple ports for a service, all ports must have port names so they are unambiguous.');
+    }
 
     // validate the ports are owned by our containers
     for (const port of ports) {


Test patch:
diff --git a/test/__snapshots__/deployment.test.ts.snap b/test/__snapshots__/deployment.test.ts.snap
index 77b95800..4a656e0c 100644
--- a/test/__snapshots__/deployment.test.ts.snap
+++ b/test/__snapshots__/deployment.test.ts.snap
@@ -260,9 +260,11 @@ Array [
               "ports": Array [
                 Object {
                   "containerPort": 8080,
+                  "name": "port1",
                 },
                 Object {
                   "containerPort": 9090,
+                  "name": "port2",
                 },
               ],
               "resources": Object {
@@ -306,10 +308,12 @@ Array [
       "externalIPs": Array [],
       "ports": Array [
         Object {
+          "name": "port1",
           "port": 8080,
           "targetPort": 8080,
         },
         Object {
+          "name": "port2",
           "port": 9090,
           "targetPort": 9090,
         },
diff --git a/test/deployment.test.ts b/test/deployment.test.ts
index 67b333d0..96d8eab5 100644
--- a/test/deployment.test.ts
+++ b/test/deployment.test.ts
@@ -676,9 +676,11 @@ test('expose captures all container ports', () => {
       ports: [
         {
           number: 8080,
+          name: 'port1',
         },
         {
           number: 9090,
+          name: 'port2',
         },
       ],
     }],
@@ -713,4 +715,42 @@ test('cannot expose with a port not owned by the container', () => {
     ports: [{ port: 2020 }],
   })).toThrowError('Unable to expose deployment');
 
+});
+
+test('expose via service with multiple ports throws error when names are not provided', () => {
+
+  const chart = Testing.chart();
+
+  const deployment = new kplus.Deployment(chart, 'Deployment', {
+    containers: [{
+      image: 'image',
+      ports: [
+        {
+          number: 8080,
+          name: 'port1',
+        },
+        {
+          number: 9090,
+        },
+      ],
+    }],
+  });
+
+  expect(() => deployment.exposeViaService()).toThrowError('Unable to expose deployment');
+
+  const deployment1 = new kplus.Deployment(chart, 'Deployment2', {
+    containers: [{
+      image: 'image',
+      ports: [
+        {
+          number: 8080,
+        },
+        {
+          number: 9090,
+        },
+      ],
+    }],
+  });
+
+  expect(() => deployment1.exposeViaService()).toThrowError('Unable to expose deployment');
 });
\ No newline at end of file