Skip to content
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

fix(machines): Allow ports at the end of IPMI addresses LP#2087965 #5560

Merged
merged 2 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,13 @@ import { PowerFieldScope, PowerFieldType } from "@/app/store/general/types";
import { machineActions } from "@/app/store/machine";
import type { RootState } from "@/app/store/root/types";
import * as factory from "@/testing/factories";
import { userEvent, render, screen, waitFor } from "@/testing/utils";
import {
userEvent,
render,
screen,
waitFor,
renderWithBrowserRouter,
} from "@/testing/utils";

const mockStore = configureStore();

Expand Down Expand Up @@ -42,6 +48,17 @@ beforeEach(() => {
],
name: PowerTypeNames.APC,
}),
factory.powerType({
fields: [
factory.powerField({
name: "ip_address",
label: "IP address",
field_type: PowerFieldType.IP_ADDRESS,
scope: PowerFieldScope.NODE,
}),
],
name: PowerTypeNames.IPMI,
}),
],
loaded: true,
}),
Expand All @@ -53,9 +70,15 @@ beforeEach(() => {
power_type: PowerTypeNames.AMT,
system_id: "abc123",
}),
factory.machineDetails({
permissions: ["edit"],
power_type: PowerTypeNames.IPMI,
system_id: "def456",
}),
],
statuses: factory.machineStatuses({
abc123: factory.machineStatus(),
def456: factory.machineStatus(),
}),
}),
});
Expand Down Expand Up @@ -116,6 +139,83 @@ it("renders read-only text fields until edit button is pressed", async () => {
).toBeInTheDocument();
});

it("can validate IPv6 addresses with a port for IPMI power type", async () => {
const store = mockStore(state);
renderWithBrowserRouter(<PowerForm systemId="def456" />, { store });

await userEvent.click(
screen.getAllByRole("button", { name: Labels.EditButton })[0]
);

await userEvent.selectOptions(
screen.getByRole("combobox", { name: "Power type" }),
PowerTypeNames.IPMI
);

await userEvent.clear(screen.getByRole("textbox", { name: "IP address" }));
await userEvent.type(
screen.getByRole("textbox", { name: "IP address" }),
"not an ip address"
);

await userEvent.tab();

expect(
screen.getByText("Please enter a valid IP address.")
).toBeInTheDocument();

await userEvent.clear(screen.getByRole("textbox", { name: "IP address" }));
await userEvent.type(
screen.getByRole("textbox", { name: "IP address" }),
// This is entered as [2001:db8::1]:8080, since square brackets are
// special characters in testing-library user events and can be escaped by doubling.
"[[2001:db8::1]:8080"
);

await userEvent.tab();

expect(
screen.queryByText("Please enter a valid IP address.")
).not.toBeInTheDocument();
});

it("can validate IPv4 addresses with a port for IPMI power type", async () => {
const store = mockStore(state);
renderWithBrowserRouter(<PowerForm systemId="def456" />, { store });

await userEvent.click(
screen.getAllByRole("button", { name: Labels.EditButton })[0]
);

await userEvent.selectOptions(
screen.getByRole("combobox", { name: "Power type" }),
PowerTypeNames.IPMI
);

await userEvent.clear(screen.getByRole("textbox", { name: "IP address" }));
await userEvent.type(
screen.getByRole("textbox", { name: "IP address" }),
"not an ip address"
);

await userEvent.tab();

expect(
screen.getByText("Please enter a valid IP address.")
).toBeInTheDocument();

await userEvent.clear(screen.getByRole("textbox", { name: "IP address" }));
await userEvent.type(
screen.getByRole("textbox", { name: "IP address" }),
"192.168.0.2:8080"
);

await userEvent.tab();

expect(
screen.queryByText("Please enter a valid IP address.")
).not.toBeInTheDocument();
});
it("correctly dispatches an action to update a machine's power", async () => {
const machine = factory.machineDetails({
permissions: ["edit"],
Expand Down
41 changes: 37 additions & 4 deletions src/app/store/general/utils/powerTypes.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { isIP } from "is-ip";
import { isIP, isIPv6 } from "is-ip";
import * as Yup from "yup";
import type { ObjectShape } from "yup/lib/object";

import type { PowerField, PowerType } from "@/app/store/general/types";
import { PowerFieldScope, PowerFieldType } from "@/app/store/general/types";
import type { PowerParameters } from "@/app/store/types/node";
import { isValidPortNumber } from "@/app/utils/isValidPortNumber";

/**
* Formats power parameters by what is expected by the api. Also, React expects
Expand Down Expand Up @@ -56,12 +57,44 @@ const getPowerFieldSchema = (fieldType: PowerFieldType) => {
case PowerFieldType.MULTIPLE_CHOICE:
return Yup.array().of(Yup.string());
case PowerFieldType.IP_ADDRESS:
case PowerFieldType.VIRSH_ADDRESS:
case PowerFieldType.LXD_ADDRESS:
return Yup.string().test({
name: "is-ip-address",
message: "Please enter a valid IP address.",
test: (value) => isIP(value as string),
test: (value) => {
if (typeof value !== "string") {
return false;
}
// reject if value contains whitespace
if (value.includes(" ")) {
return false;
}
if (value.includes("[") && value.includes("]")) {
// This is an IPv6 address with a port number
const openingBracketIndex = value.indexOf("[");
const closingBracketIndex = value.indexOf("]");
const ip = value.slice(
openingBracketIndex + 1,
closingBracketIndex
);
// We use +2 here to include the `:` before the port number
const port = value.slice(closingBracketIndex + 2);
return (
isIPv6(ip) &&
!isNaN(parseInt(port)) &&
isValidPortNumber(parseInt(port))
);
} else if (value.split(":").length === 2) {
// This is an IPv4 address with a port number
const [ip, port] = value.split(":");
return (
isIP(ip) &&
!isNaN(parseInt(port)) &&
isValidPortNumber(parseInt(port))
);
} else {
return isIP(value);
}
},
});
default:
return Yup.string();
Expand Down
1 change: 1 addition & 0 deletions src/app/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,4 @@ export {
timeSpanToMinutes,
} from "./timeSpan";
export { parseCommaSeparatedValues } from "./parseCommaSeparatedValues";
export { isValidPortNumber } from "./isValidPortNumber";
15 changes: 15 additions & 0 deletions src/app/utils/isValidPortNumber.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { isValidPortNumber } from ".";

it("returns true for any number between 0 and 65535", () => {
for (let i = 0; i <= 65535; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a computationally expensive test for the function it tests.... Edge cases and some common ports might be enough. But I'm happy to merge if it runs quickly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locally it seems to run quickly enough, but thought I'd get your thoughts first.

expect(isValidPortNumber(i)).toBe(true);
}
});

it("returns false for numbers larger than 65535", () => {
expect(isValidPortNumber(65536)).toBe(false);
});

it("returns false for numbers smaller than 0", () => {
expect(isValidPortNumber(-1)).toBe(false);
});
9 changes: 9 additions & 0 deletions src/app/utils/isValidPortNumber.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/**
* Checks if a given port number is valid (between 0 and 65535).
*
* @param port The port number to check.
* @returns True if valid, false otherwise.
*/
export const isValidPortNumber = (port: number): boolean => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that its port: number :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

me too :)

return port >= 0 && port <= 65535;
};
Loading