fix: [M3-7877] - Incorrect Linode network interface configuration being displayed by hkhalil-akamai · Pull Request #10690 · linode/manager

hkhalil-akamai · 2024-07-17T18:27:30Z

Description 📝

Fixes a bug in the Linode configuration dialog causing network interfaces to be displayed incorrectly.

Changes 🔄

Fix race condition causing network interfaces to be displayed incorrectly
Add unit test to verify fix
- Add LinodeKernelFactory to mock kernels
- I haven't been able to replicate the bug with this unit test -- see this comment for more details

Bug pathology 🪳

The following represents my best explanation of the bug but may not be correct:

The bug is caused by a race condition between two `useEffect`s:

Updates the formik form state when config values become available

manager/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx

Lines 467 to 531 in e60faef

    
             React.useEffect(() => { 
        
               if (open) { 
        
                 /** 
        
                  * If config is defined, we're editing. Set the state 
        
                  * to the values of the config. 
        
                  */ 
        
                 if (config) { 
        
                   const devices = createStringsFromDevices(config.devices); 
        
                   /* 
        
                   If device slots are populated out of sequential order (e.g. sda and sdb are assigned 
        
                   but no others are until sdf), ascertain the last assigned slot to determine how many 
        
                   device slots to display initially. 
        
                   */ 
        
                   const assignedDevices = Object.keys(devices); 
        
                   const lastAssignedDeviceSlot = 
        
                     assignedDevices[assignedDevices.length - 1]; 
        
                   const positionInSequentialSlots = deviceSlots.indexOf( 
        
                     lastAssignedDeviceSlot 
        
                   ); 
        
                   setDeviceCounter(positionInSequentialSlots); 
        
                   setUseCustomRoot( 
        
                     !pathsOptions.some( 
        
                       (thisOption) => thisOption.value === config?.root_device 
        
                     ) 
        
                   ); 
        
                   const indexOfExistingPrimaryInterface = config.interfaces.findIndex( 
        
                     (_interface) => _interface.primary === true 
        
                   ); 
        
                   if (indexOfExistingPrimaryInterface !== -1) { 
        
                     setPrimaryInterfaceIndex(indexOfExistingPrimaryInterface); 
        
                   } 
        
                   resetForm({ 
        
                     values: { 
        
                       comments: config.comments, 
        
                       devices, 
        
                       helpers: config.helpers, 
        
                       initrd: initrdFromConfig, 
        
                       interfaces: interfacesToState(config.interfaces), 
        
                       kernel: config.kernel, 
        
                       label: config.label, 
        
                       memory_limit: config.memory_limit, 
        
                       root_device: config.root_device, 
        
                       run_level: config.run_level, 
        
                       setMemoryLimit: 
        
                         config.memory_limit !== 0 ? 'set_limit' : 'no_limit', 
        
                       useCustomRoot: isUsingCustomRoot(config.root_device), 
        
                       virt_mode: config.virt_mode, 
        
                     }, 
        
                   }); 
        
                 } else { 
        
                   // Create mode; make sure loading/error states are cleared. 
        
                   resetForm({ values: defaultFieldsValues }); 
        
                   setUseCustomRoot(false); 
        
                   setDeviceCounter(deviceCounterDefault); 
        
                   setPrimaryInterfaceIndex(0); 
        
                 } 
        
               } 
        
             }, [open, config, initrdFromConfig, resetForm, queryClient]);

Modifies VPC interfaces to add IPv4 information

manager/packages/manager/src/features/Linodes/LinodesDetail/LinodeSettings/InterfaceSelect.tsx

Lines 230 to 282 in e60faef

    
             React.useEffect(() => { 
        
               if (purpose !== 'vpc') { 
        
                 return handleChange({ 
        
                   ipam_address: ipamAddress, 
        
                   label, 
        
                   purpose, 
        
                 }); 
        
               } 
        
               const changeObj = { 
        
                 ip_ranges: _additionalIPv4RangesForVPC, 
        
                 ipam_address: null, 
        
                 label: null, 
        
                 purpose, 
        
                 subnet_id: subnetId, 
        
                 vpc_id: vpcId, 
        
               }; 
        
               /** 
        
                * If a user checks the "Auto-assign a VPC IPv4 address" box, then we send the user inputted address, otherwise we send nothing/undefined. 
        
                * If a user checks the "Assign a public IPv4" address box, then we send nat_1_1: 'any' to the API for auto assignment. 
        
                */ 
        
               if (!autoAssignVPCIPv4 && autoAssignLinodeIPv4) { 
        
                 handleChange({ 
        
                   ...changeObj, 
        
                   ipv4: { 
        
                     nat_1_1: 'any', 
        
                     vpc: vpcIPv4, 
        
                   }, 
        
                 }); 
        
               } else if ( 
        
                 (autoAssignVPCIPv4 && autoAssignLinodeIPv4) || 
        
                 autoAssignLinodeIPv4 
        
               ) { 
        
                 handleChange({ 
        
                   ...changeObj, 
        
                   ipv4: { 
        
                     nat_1_1: 'any', 
        
                   }, 
        
                 }); 
        
               } else if (autoAssignVPCIPv4 && !autoAssignLinodeIPv4) { 
        
                 handleChange({ 
        
                   ...changeObj, 
        
                 }); 
        
               } else if (!autoAssignLinodeIPv4 && !autoAssignVPCIPv4) { 
        
                 handleChange({ 
        
                   ...changeObj, 
        
                   ipv4: { 
        
                     vpc: vpcIPv4, 
        
                   }, 
        
                 }); 
        
               } 
        
             }, [autoAssignVPCIPv4, autoAssignLinodeIPv4, purpose]);

The race condition occurs in the following sequence:

The LinodeConfigDialog is opened
config is updated from undefined to the selected Linode's config -> this sets off useEffect 1 which queues the new interface values in formik
- Due to Automatic Batching in React 18, the formik state will not actually be updated until the next render
InterfaceSelect is mounted -> this sets off useEffect 2, overriding the updates from step 2

The fix:

Update useEffect 2 to return early if the interface is non-VPC

Target release date 🗓️

7/22

Preview 📷

Before	After
Screen.Recording.2024-07-17.at.2.18.13.PM.mov	Screen.Recording.2024-07-17.at.2.18.36.PM.mov

How to test 🧪

Prerequisites

Create a Linode with a non-default network configuration
- For example:
  eth0: VPC
  eth1: Public internet

Reproduction steps

This bug is caused by a race condition and is tricky to reproduce consistently. The following steps seem to trigger the bug reliably:

Navigate to the newly created Linode (from Prerequisites)
Navigate to the "Configurations" tab and click "Edit" on the non-default configuration to open the configuration dialog
Exit out of the configuration dialog and navigate to the different section of the app (e.g., Volumes, NodeBalancers, etc.)
Return to the Linode configuration dialog and observe the network interface fields are not correct (i.e., they show the default network interfaces)

Verification steps

Perform the same steps as above but observe the correct network interfaces are always dispalyed

As an Author I have considered 🤔

Check all that apply

…ng displayed

hkhalil-akamai · 2024-07-17T20:49:35Z

packages/manager/src/features/Linodes/LinodesDetail/LinodeSettings/InterfaceSelect.tsx

-      return handleChange({
-        ipam_address: ipamAddress,
-        label,
-        purpose,
-      });


This is the actual fix.

@hana-linode can you verify this change has no unintended consequences? I wasn't able to figure out the original purpose of this.

@hkhalil-akamai It's been a while so I don't really remember, but I think the original purpose was to return early if the purpose wasn't vpc so that vpc properties would not be passed to the handleChange function

hkhalil-akamai · 2024-07-17T20:49:59Z

packages/manager/src/utilities/testHelpers.tsx

+  const renderResult = render(wrapWithTheme(ui, options));
+  return {
+    ...renderResult,
+    rerender: (ui) => renderResult.rerender(wrapWithTheme(ui, options)),


Adding support for the rerender method

hkhalil-akamai · 2024-07-17T20:51:54Z

packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.test.tsx

+  it('should display the correct network interfaces', async () => {
+    const props = {
+      isReadOnly: false,
+      linodeId: 0,
+      onClose: vi.fn(),
+    };
+
+    const { findByText, rerender } = renderWithTheme(
+      <LinodeConfigDialog config={undefined} open={false} {...props} />
+    );
+
+    rerender(
+      <LinodeConfigDialog
+        config={linodeConfigFactory.build({
+          interfaces: [vpcInterface, publicInterface],
+        })}
+        open
+        {...props}
+      />
+    );
+
+    await findByText('VPC');
+    await findByText('Public Internet');
+  });


In theory, this test should replicate the exact sequence causing the bug. However, I haven't been able to fail this test without the fix, which I believe is due to inconsistencies between how Automatic Batching works in tests vs in the browser.

I'm still keeping this test because it may catch regressions in the future.

github-actions · 2024-07-18T21:04:19Z

Coverage Report: ✅
Base Coverage: 82.46%
Current Coverage: 82.47%

bnussman-akamai

After making no changes, clicking "Save Changes", and refreshing, the config's VPC looked different than how it was originally configured. The after refreshing again, it went back to the expected values.

Not sure if this is related. I haven't been able to easily reproduce.

Screen.Recording.2024-07-22.at.9.58.27.AM.mov

hkhalil-akamai · 2024-07-22T20:55:21Z

After making no changes, clicking "Save Changes", and refreshing, the config's VPC looked different than how it was originally configured. The after refreshing again, it went back to the expected values.

I have been unable to reproduce this. There is a lot of complicated logic in the LinodeConfigDialog and InterfaceSelect components that needs to be cleaned up but I don't know if it's being caused by this change. Are you able to reproduce it in prod?

bnussman-akamai

I wasn't able to reproduce the issue I showed above, so I think we're good

hana-akamai · 2024-07-23T15:29:16Z

I was able to reproduce the bug @bnussman-akamai saw on this branch on a fresh Linode and new VPC created during the flow. I was not able to reproduce in prod 🤔

dwiley-akamai

Code review ✅
Observing correct network interfaces ✅

I was not able to reproduce the issue Banks observed, and I didn't observe any adverse impacts from the changes in InterfaceSelect.tsx

…rface settings

hkhalil-akamai · 2024-07-23T18:14:55Z

@bnussman-akamai @hana-linode @dwiley-akamai

I was able to trace back the issue to the same useEffect in InterfaceSelect.tsx. I believe I have solved the issue by removing the useEffect and rewriting the onChange handlers to modify the interface directly (545f6db).

The following steps seem to reliably reproduce the issue (before the fix):

Create a new Linode with the eth0 interface set to VPC.
- Deselect "Auto-assign a VPC IPv4 address for this Linode in the VPC" and manually set an IPv4 address such as "10.0.0.2"
- Select "Assign a public IPv4 address for this Linode"
- For example, your configuration could look like this:
Navigate to the Linode details page -> Configuration tab and edit the configuration
Verify the VPC interface appears as expected (i.e., with the first checkbox deselected and the second checkbox selected) -- you may need to refresh the page to ensure the interface appears as expected.
Navigate to a different page in CM (e.g., NodeBalancers, Volumes, etc.)
Return to the Linode details page -> Configuration tab -> Edit configuration and observe the VPC interface has been reset (the first checkbox is selected and the second checkbox is deselected)

…t-config-profile

hana-akamai · 2024-07-23T19:09:23Z

So it seems like editing the config doesn't update the UI now 💀 . See the checkbox still being checked on this branch vs unchecked on prod

Screen.Recording.2024-07-23.at.3.06.47.PM.mov

bnussman-akamai · 2024-07-23T19:11:26Z

I haven't been able to replicate the first bug I brought up, which is good ✅

I am seeing the form reset to a weird state when I try to remove a VPC IP range, which does not happen in production.

Screen.Recording.2024-07-23.at.3.08.54.PM.mov

bnussman-akamai

No more bugs found from my testing 🎉

hana-akamai

No more bugs found as well 🎉

…t-config-profile

Fix incorrect linode network config

44e2b8c

hkhalil-akamai added the VPC Relating to VPC project label Jul 17, 2024

hkhalil-akamai self-assigned this Jul 17, 2024

hkhalil-akamai added Bug Fixes for regressions or bugs Ready for Review labels Jul 17, 2024

hkhalil-akamai changed the title ~~fix: [M3-7877] - Fix incorrect Linode network interface configuration being displayed~~ fix: [M3-7877] - Incorrect Linode network interface configuration being displayed Jul 17, 2024

hkhalil-akamai added 2 commits July 17, 2024 14:28

Added changeset: Incorrect Linode network interface configuration bei…

bbdfc84

…ng displayed

Add unit test for LinodeConfigDialog

3fd04d8

hkhalil-akamai commented Jul 17, 2024

View reviewed changes

hkhalil-akamai requested a review from hana-akamai July 17, 2024 20:57

hkhalil-akamai marked this pull request as ready for review July 17, 2024 20:57

hkhalil-akamai requested a review from a team as a code owner July 17, 2024 20:57

hkhalil-akamai requested review from AzureLatte and dwiley-akamai and removed request for a team July 17, 2024 20:57

hkhalil-akamai added 2 commits July 18, 2024 16:32

Fix validation schema to allow null values

c14ab6f

Added changeset: Allow null values in Linode configuration

008f2e1

AzureLatte approved these changes Jul 19, 2024

View reviewed changes

bnussman-akamai reviewed Jul 22, 2024

View reviewed changes

bnussman-akamai approved these changes Jul 23, 2024

View reviewed changes

dwiley-akamai approved these changes Jul 23, 2024

View reviewed changes

Remove problematic useEffect and redundant source of truth for inte…

545f6db

…rface settings

Merge remote-tracking branch 'upstream/develop' into M3-7877-incorrec…

c3eb580

…t-config-profile

hkhalil-akamai added 3 commits July 24, 2024 11:40

Fix checkbox not displaying correct value @hana-linode

96850d8

Added changeset: Support null values in Interface type

7dc82a6

Fix changing IP ranges resets VPC settings @bnussman-akamai

1c0820f

hkhalil-akamai requested review from bnussman-akamai and dwiley-akamai July 25, 2024 18:38

bnussman-akamai approved these changes Jul 26, 2024

View reviewed changes

hana-akamai approved these changes Jul 29, 2024

View reviewed changes

bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Jul 29, 2024

hkhalil-akamai added 2 commits July 29, 2024 16:53

Fix bug causing duplicate primary interfaces

134f04f

Merge remote-tracking branch 'upstream/develop' into M3-7877-incorrec…

21d178f

…t-config-profile

hkhalil-akamai merged commit 7ee33e0 into linode:develop Jul 29, 2024

hkhalil-akamai deleted the M3-7877-incorrect-config-profile branch July 29, 2024 21:39

This was referenced Aug 1, 2024

Release v1.125.0 - release → staging #10741

Merged

Release v1.125.0 - staging → master #10748

Merged

	React.useEffect(() => {
	if (open) {
	/**
	* If config is defined, we're editing. Set the state
	* to the values of the config.
	*/
	if (config) {
	const devices = createStringsFromDevices(config.devices);

	/*
	If device slots are populated out of sequential order (e.g. sda and sdb are assigned
	but no others are until sdf), ascertain the last assigned slot to determine how many
	device slots to display initially.
	*/
	const assignedDevices = Object.keys(devices);
	const lastAssignedDeviceSlot =
	assignedDevices[assignedDevices.length - 1];

	const positionInSequentialSlots = deviceSlots.indexOf(
	lastAssignedDeviceSlot
	);

	setDeviceCounter(positionInSequentialSlots);

	setUseCustomRoot(
	!pathsOptions.some(
	(thisOption) => thisOption.value === config?.root_device
	)
	);

	const indexOfExistingPrimaryInterface = config.interfaces.findIndex(
	(_interface) => _interface.primary === true
	);

	if (indexOfExistingPrimaryInterface !== -1) {
	setPrimaryInterfaceIndex(indexOfExistingPrimaryInterface);
	}

	resetForm({
	values: {
	comments: config.comments,
	devices,
	helpers: config.helpers,
	initrd: initrdFromConfig,
	interfaces: interfacesToState(config.interfaces),
	kernel: config.kernel,
	label: config.label,
	memory_limit: config.memory_limit,
	root_device: config.root_device,
	run_level: config.run_level,
	setMemoryLimit:
	config.memory_limit !== 0 ? 'set_limit' : 'no_limit',
	useCustomRoot: isUsingCustomRoot(config.root_device),
	virt_mode: config.virt_mode,
	},
	});
	} else {
	// Create mode; make sure loading/error states are cleared.
	resetForm({ values: defaultFieldsValues });
	setUseCustomRoot(false);
	setDeviceCounter(deviceCounterDefault);
	setPrimaryInterfaceIndex(0);
	}
	}
	}, [open, config, initrdFromConfig, resetForm, queryClient]);

	React.useEffect(() => {
	if (purpose !== 'vpc') {
	return handleChange({
	ipam_address: ipamAddress,
	label,
	purpose,
	});
	}

	const changeObj = {
	ip_ranges: _additionalIPv4RangesForVPC,
	ipam_address: null,
	label: null,
	purpose,
	subnet_id: subnetId,
	vpc_id: vpcId,
	};

	/**
	* If a user checks the "Auto-assign a VPC IPv4 address" box, then we send the user inputted address, otherwise we send nothing/undefined.
	* If a user checks the "Assign a public IPv4" address box, then we send nat_1_1: 'any' to the API for auto assignment.
	*/
	if (!autoAssignVPCIPv4 && autoAssignLinodeIPv4) {
	handleChange({
	...changeObj,
	ipv4: {
	nat_1_1: 'any',
	vpc: vpcIPv4,
	},
	});
	} else if (
	(autoAssignVPCIPv4 && autoAssignLinodeIPv4) \|\|
	autoAssignLinodeIPv4
	) {
	handleChange({
	...changeObj,
	ipv4: {
	nat_1_1: 'any',
	},
	});
	} else if (autoAssignVPCIPv4 && !autoAssignLinodeIPv4) {
	handleChange({
	...changeObj,
	});
	} else if (!autoAssignLinodeIPv4 && !autoAssignVPCIPv4) {
	handleChange({
	...changeObj,
	ipv4: {
	vpc: vpcIPv4,
	},
	});
	}
	}, [autoAssignVPCIPv4, autoAssignLinodeIPv4, purpose]);

Conversation

hkhalil-akamai commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📝

Changes 🔄

Bug pathology 🪳

The bug is caused by a race condition between two useEffects:

The race condition occurs in the following sequence:

The fix:

Target release date 🗓️

Preview 📷

How to test 🧪

Prerequisites

Reproduction steps

Verification steps

As an Author I have considered 🤔

Uh oh!

hkhalil-akamai Jul 17, 2024

Choose a reason for hiding this comment

Uh oh!

hkhalil-akamai Jul 17, 2024

Choose a reason for hiding this comment

Uh oh!

hana-akamai Jul 23, 2024

Choose a reason for hiding this comment

Uh oh!

hkhalil-akamai Jul 17, 2024

Choose a reason for hiding this comment

Uh oh!

hkhalil-akamai Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnussman-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

hkhalil-akamai commented Jul 22, 2024

Uh oh!

bnussman-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

hana-akamai commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwiley-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

hkhalil-akamai commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hana-akamai commented Jul 23, 2024

Uh oh!

bnussman-akamai commented Jul 23, 2024

Uh oh!

bnussman-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

hana-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hkhalil-akamai commented Jul 17, 2024 •

edited

Loading

The bug is caused by a race condition between two `useEffect`s:

hkhalil-akamai Jul 17, 2024 •

edited

Loading

github-actions bot commented Jul 18, 2024 •

edited

Loading

hana-akamai commented Jul 23, 2024 •

edited

Loading

hkhalil-akamai commented Jul 23, 2024 •

edited

Loading