From 3681aacb6f4372836e988ce4a2f6019000de5070 Mon Sep 17 00:00:00 2001
From: Akash Askoolum <akash1810@gmail.com>
Date: Thu, 3 Feb 2022 12:33:26 +0000
Subject: [PATCH] fix: Enforce a CNAME DNS entry has a single answer

There are currently two ways to create `Guardian::DNS::RecordSet` resource:
  1. `GuCname`
  2. `GuDnsRecordSet`

A CNAME should not return multiple answers without "the correct filters in place (such as SELECT_FIRST_N 1) to limit them to a single answer at resolution time".

`Guardian::DNS::RecordSet` does not create any filters (and it's unlikely to).

The props of `GuCname` enforce this single answer restriction,
however if you're still able to use `GuDnsRecordSet` to create a CNAME that violates this restriction.

In this change we throw if a CNAME is being created with multiple answers.
---
 src/constructs/dns/dns-records.test.ts | 15 +++++++++++++
 src/constructs/dns/dns-records.ts      | 31 +++++++++++++++++++++-----
 2 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/src/constructs/dns/dns-records.test.ts b/src/constructs/dns/dns-records.test.ts
index 778e1eee91..81c4ee326c 100644
--- a/src/constructs/dns/dns-records.test.ts
+++ b/src/constructs/dns/dns-records.test.ts
@@ -28,6 +28,21 @@ describe("The GuDnsRecordSet construct", () => {
     });
     expect(stack).toHaveResourceOfTypeAndLogicalId("Guardian::DNS::RecordSet", "ThisExactLogicalId");
   });
+
+  it("should throw if a CNAME is created with multiple answers", () => {
+    const stack = simpleGuStackForTesting();
+
+    expect(() => {
+      new GuDnsRecordSet(stack, "ThisExactLogicalId", {
+        name: "banana.example.com",
+        recordType: RecordType.CNAME,
+        resourceRecords: ["apple.example.com", "banana.example.com"],
+        ttl: Duration.hours(1),
+      });
+    }).toThrowError(
+      "According to RFC, a CNAME record should not return multiple answers. Doing so may cause problems during resolution."
+    );
+  });
 });
 
 describe("The GuCname construct", () => {
diff --git a/src/constructs/dns/dns-records.ts b/src/constructs/dns/dns-records.ts
index db7c1fa42e..475eea7a21 100644
--- a/src/constructs/dns/dns-records.ts
+++ b/src/constructs/dns/dns-records.ts
@@ -26,16 +26,37 @@ export interface GuDnsRecordSetProps {
  */
 export class GuDnsRecordSet {
   constructor(scope: GuStack, id: string, props: GuDnsRecordSetProps) {
+    const { name, recordType, resourceRecords, ttl } = props;
+    const { stage } = scope;
+
+    // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- more `RecordType`s will be added soon!
+    if (recordType === RecordType.CNAME) {
+      /*
+      If you try to create a CNAME with multiple records within NS1, you are greeted with:
+
+        According to RFC, a CNAME record should not return multiple answers.
+        Doing so may cause problems during resolution.
+        If you want to use multiple answers, you should ensure you have the correct filters in place (such as SELECT_FIRST_N 1) to limit them to a single answer at resolution time.
+
+      `Guardian::DNS::RecordSet` does not implement "correct filters", so fail fast by throwing.
+       */
+      if (resourceRecords.length !== 1) {
+        throw new Error(
+          "According to RFC, a CNAME record should not return multiple answers. Doing so may cause problems during resolution."
+        );
+      }
+    }
+
     // The spec for this private resource type can be found here:
     // https://github.com/guardian/cfn-private-resource-types/tree/main/dns/guardian-dns-record-set-type/docs#syntax
     new CfnResource(scope, id, {
       type: "Guardian::DNS::RecordSet",
       properties: {
-        Name: props.name,
-        ResourceRecords: props.resourceRecords,
-        RecordType: props.recordType,
-        TTL: props.ttl.toSeconds(),
-        Stage: scope.stage,
+        Name: name,
+        ResourceRecords: resourceRecords,
+        RecordType: recordType,
+        TTL: ttl.toSeconds(),
+        Stage: stage,
       },
     });
   }