Skip to content

Commit

Permalink
Use InputPropertyError for validations in the node group components (#…
Browse files Browse the repository at this point in the history
…1592)

pu/pu introduced new error types for component authors to use. The
`InputPropertyError` and `InputPropertiesError` errors are pretty
printed by the engine and don't confuse users with a load of GRPC stack
traces (see
[docs](https://www.pulumi.com/docs/reference/pkg/nodejs/pulumi/pulumi/classes/InputPropertyError.html)).
Additionally those errors accumulate multiple validation errors and
allow presenting them all to users. Previously we exited on the first
error.

For adding EFA support
(#1564) we'd like to make use
of these new errors. In preparation for that I've updated the node group
components to make use of this new error type.
  • Loading branch information
flostadler authored Jan 24, 2025
1 parent 3328fbe commit 3f7f18c
Show file tree
Hide file tree
Showing 4 changed files with 253 additions and 237 deletions.
10 changes: 5 additions & 5 deletions nodejs/eks/nodes/ami.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,29 +46,29 @@ describe("toAmiType", () => {
});
describe("getOperatingSystem", () => {
test("should return the provided operating system if available", () => {
const operatingSystem = getOperatingSystem(undefined, OperatingSystem.AL2023, undefined);
const operatingSystem = getOperatingSystem(undefined, OperatingSystem.AL2023);
expect(operatingSystem).toBe(OperatingSystem.AL2023);
});

test("should return the default operating system if no AMI type or operating system is provided", () => {
const operatingSystem = getOperatingSystem(undefined, undefined, undefined);
const operatingSystem = getOperatingSystem(undefined, undefined);
expect(operatingSystem).toBe(DEFAULT_OS);
});

test("should resolve the operating system based on the provided AMI type", () => {
const operatingSystem = getOperatingSystem("AL2023_ARM_64_STANDARD", undefined, undefined);
const operatingSystem = getOperatingSystem("AL2023_ARM_64_STANDARD", undefined);
expect(operatingSystem).toBe(OperatingSystem.AL2023);
});

test("should throw an error for unknown AMI type", () => {
expect(() => {
getOperatingSystem("unknown-ami-type", undefined, undefined);
getOperatingSystem("unknown-ami-type", undefined);
}).toThrow("Cannot determine OS of unknown AMI type: unknown-ami-type");
});

test("should throw an error if the operating system does not match the AMI type", () => {
expect(() => {
getOperatingSystem("AL2023_ARM_64_STANDARD", OperatingSystem.Bottlerocket, undefined);
getOperatingSystem("AL2023_ARM_64_STANDARD", OperatingSystem.Bottlerocket);
}).toThrow(
"Operating system 'Bottlerocket' does not match the detected operating system 'AL2023' of AMI type 'AL2023_ARM_64_STANDARD'.",
);
Expand Down
17 changes: 8 additions & 9 deletions nodejs/eks/nodes/ami.ts
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,6 @@ export function toAmiType(str: string): AmiType | undefined {
export function getOperatingSystem(
amiType: string | undefined,
operatingSystem: OperatingSystem | undefined,
parent: pulumi.Resource | undefined,
): OperatingSystem {
if (operatingSystem && !amiType) {
return operatingSystem;
Expand All @@ -210,20 +209,20 @@ export function getOperatingSystem(

const resolvedAmiType = toAmiType(amiType);
if (!resolvedAmiType) {
throw new pulumi.ResourceError(
`Cannot determine OS of unknown AMI type: ${amiType}`,
parent,
);
throw new pulumi.InputPropertyError({
propertyPath: "amiType",
reason: `Cannot determine OS of unknown AMI type: ${amiType}`,
});
}

const resolvedOs = getAmiMetadata(resolvedAmiType).os;

// if users provided both OS and AMI type, we should check if they match
if (operatingSystem && operatingSystem !== resolvedOs) {
throw new pulumi.ResourceError(
`Operating system '${operatingSystem}' does not match the detected operating system '${resolvedOs}' of AMI type '${amiType}'.`,
parent,
);
throw new pulumi.InputPropertyError({
propertyPath: "operatingSystem",
reason: `Operating system '${operatingSystem}' does not match the detected operating system '${resolvedOs}' of AMI type '${amiType}'.`,
});
}

return resolvedOs;
Expand Down
93 changes: 30 additions & 63 deletions nodejs/eks/nodes/nodegroup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -307,40 +307,40 @@ const nonGravitonInstances = [
describe("isGravitonInstance", () => {
gravitonInstances.forEach((instanceType) => {
test(`${instanceType} should return true for a Graviton instance`, () => {
expect(isGravitonInstance(instanceType, undefined)).toBe(true);
expect(isGravitonInstance(instanceType, "instanceTypes")).toBe(true);
});
});

nonGravitonInstances.forEach((instanceType) => {
test(`${instanceType} should return false for non-Graviton instance`, () => {
expect(isGravitonInstance(instanceType, undefined)).toBe(false);
expect(isGravitonInstance(instanceType, "instanceTypes")).toBe(false);
});
});
describe("getArchitecture", () => {
test("should return 'x86_64' when only x86_64 instances are provided", () => {
const instanceTypes = ["c5.large", "m5.large", "t3.large"];
const architecture = getArchitecture(instanceTypes, undefined);
const architecture = getArchitecture(instanceTypes, "instanceTypes");
expect(architecture).toBe("x86_64");
});

test("should return 'arm64' when only arm64 instances are provided", () => {
const instanceTypes = ["c6g.large", "m6g.large", "t4g.large"];
const architecture = getArchitecture(instanceTypes, undefined);
const architecture = getArchitecture(instanceTypes, "instanceTypes");
expect(architecture).toBe("arm64");
});

test("should throw an error when both x86_64 and arm64 instances are provided", () => {
const instanceTypes = ["c5.large", "c6g.large"];
expect(() => {
getArchitecture(instanceTypes, undefined);
}).toThrowError(
getArchitecture(instanceTypes, "instanceTypes");
}).toThrow(
"Cannot determine architecture of instance types. The provided instance types do not share a common architecture",
);
});

test("should return 'x86_64' when no instance types are provided", () => {
const instanceTypes: string[] = [];
const architecture = getArchitecture(instanceTypes, undefined);
const architecture = getArchitecture(instanceTypes, "instanceTypes");
expect(architecture).toBe("x86_64");
});
});
Expand Down Expand Up @@ -484,71 +484,52 @@ describe("resolveInstanceProfileName", function () {

test("no args, no c.nodeGroupOptions throws", async () => {
expect(() =>
resolveInstanceProfileName(
"nodegroup-name",
{},
{
nodeGroupOptions: {},
} as pulumi.UnwrappedObject<CoreData>,
undefined as any,
),
).toThrowError("an instanceProfile or instanceProfileName is required");
resolveInstanceProfileName({}, {
nodeGroupOptions: {},
} as pulumi.UnwrappedObject<CoreData>),
).toThrow("an instanceProfile or instanceProfileName is required");
});

test("both args.instanceProfile and args.instanceProfileName throws", async () => {
const instanceProfile = new aws.iam.InstanceProfile("instanceProfile", {});
const name = "nodegroup-name";
expect(() =>
resolveInstanceProfileName(
name,
{
instanceProfile: instanceProfile,
instanceProfileName: "instanceProfileName",
},
{
nodeGroupOptions: {},
} as pulumi.UnwrappedObject<CoreData>,
undefined as any,
),
).toThrowError(
`invalid args for node group ${name}, instanceProfile and instanceProfileName are mutually exclusive`,
);
).toThrow(`instanceProfile and instanceProfileName are mutually exclusive`);
});

test("both c.nodeGroupOptions.instanceProfileName and c.nodeGroupOptions.instanceProfile throws", async () => {
const instanceProfile = new aws.iam.InstanceProfile("instanceProfile", {});
const name = "nodegroup-name";
expect(() =>
resolveInstanceProfileName(
name,
{},
{
nodeGroupOptions: {
instanceProfile: instanceProfile,
instanceProfileName: "instanceProfileName",
},
} as pulumi.UnwrappedObject<CoreData>,
undefined as any,
),
).toThrowError(
`invalid args for node group ${name}, instanceProfile and instanceProfileName are mutually exclusive`,
);
resolveInstanceProfileName({}, {
nodeGroupOptions: {
instanceProfile: instanceProfile,
instanceProfileName: "instanceProfileName",
},
} as pulumi.UnwrappedObject<CoreData>),
).toThrow(`instanceProfile and instanceProfileName are mutually exclusive`);
});

test("args.instanceProfile returns passed instanceProfile", async () => {
const instanceProfile = new aws.iam.InstanceProfile("instanceProfile", {
name: "passedInstanceProfile",
});
const nodeGroupName = "nodegroup-name";
const resolvedInstanceProfileName = resolveInstanceProfileName(
nodeGroupName,
{
instanceProfile: instanceProfile,
},
{
nodeGroupOptions: {},
} as pulumi.UnwrappedObject<CoreData>,
undefined as any,
);
const expected = await promisify(instanceProfile.name);
const recieved = await promisify(resolvedInstanceProfileName);
Expand All @@ -559,53 +540,39 @@ describe("resolveInstanceProfileName", function () {
const instanceProfile = new aws.iam.InstanceProfile("instanceProfile", {
name: "passedInstanceProfile",
});
const nodeGroupName = "nodegroup-name";
const resolvedInstanceProfileName = resolveInstanceProfileName(
nodeGroupName,
{},
{
nodeGroupOptions: {
instanceProfile: instanceProfile,
},
} as pulumi.UnwrappedObject<CoreData>,
undefined as any,
);
const resolvedInstanceProfileName = resolveInstanceProfileName({}, {
nodeGroupOptions: {
instanceProfile: instanceProfile,
},
} as pulumi.UnwrappedObject<CoreData>);
const expected = await promisify(instanceProfile.name);
const recieved = await promisify(resolvedInstanceProfileName);
expect(recieved).toEqual(expected);
});

test("args.instanceProfileName returns passed InstanceProfile", async () => {
const nodeGroupName = "nodegroup-name";
const nodeGroupName = "-name";
const existingInstanceProfileName = "existingInstanceProfileName";
const resolvedInstanceProfileName = resolveInstanceProfileName(
nodeGroupName,
{
instanceProfileName: existingInstanceProfileName,
},
{
nodeGroupOptions: {},
} as pulumi.UnwrappedObject<CoreData>,
undefined as any,
);
const expected = existingInstanceProfileName;
const received = await promisify(resolvedInstanceProfileName);
expect(received).toEqual(expected);
});

test("nodeGroupOptions.instanceProfileName returns existing InstanceProfile", async () => {
const nodeGroupName = "nodegroup-name";
const existingInstanceProfileName = "existingInstanceProfileName";
const resolvedInstanceProfileName = resolveInstanceProfileName(
nodeGroupName,
{},
{
nodeGroupOptions: {
instanceProfileName: existingInstanceProfileName,
},
} as pulumi.UnwrappedObject<CoreData>,
undefined as any,
);
const resolvedInstanceProfileName = resolveInstanceProfileName({}, {
nodeGroupOptions: {
instanceProfileName: existingInstanceProfileName,
},
} as pulumi.UnwrappedObject<CoreData>);
const expected = existingInstanceProfileName;
const received = await promisify(resolvedInstanceProfileName);
expect(received).toEqual(expected);
Expand Down
Loading

0 comments on commit 3f7f18c

Please sign in to comment.