fix: add way to filter network interfaces #277

erhudy · 2024-03-05T16:13:57Z

Description

Under certain circumstances, net.Interfaces() will begin returning interfaces in unusual orders (e.g. if the interface index goes above 255 or 256). This could cause the driver to select the wrong interface, which in turn would mean that it could not find a matching VM in vSphere and would crash. This changeset adds arguments to control what interfaces can be selected by the driver.

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #

Checklist:

Have you run format,vet & lint checks against your submission?
Have you made sure that the code compiles?
Did you run the unit & integration tests successfully?
Have you maintained at least 90% code coverage?
Have you commented your code, particularly in hard-to-understand areas
Have you done corresponding changes to the documentation
Did you run tests in a real Kubernetes cluster?
Backward compatibility is not broken

How Has This Been Tested?

Built an image and deployed it in the affected Kubernetes cluster, along with changes to the Helm templates and values to surface the new environment variables so that they can be configured as needed.

boyamurthy · 2024-03-07T05:58:45Z

hi @erhudy , thank you for submitting the PR . We have already passed the code freeze date for CSM 1.10 release and we will consider this for CSM 1.11 .

nidtara · 2024-04-29T09:49:02Z

main.go

@@ -106,4 +106,10 @@ const usage = `    X_CSI_POWERMAX_ENDPOINT

    X_CSI_POWERMAX_DEBUG
        Turns on debugging of the PowerMax (REST interface to Unisphere) layer
+
+	X_CSI_IFACE_INCLUDE_FILTER


Please rename arguments to reflect these are VM interfaces.

nidtara

How has this been tested, please attach test logs to the PR

nidtara · 2024-04-29T10:00:19Z

service/vsphere.go

-		if v.HardwareAddr.String() != "" {
-			return v.HardwareAddr.String(), nil
+		if ifaceExcludeFilter != nil || ifaceIncludeFilter != nil {
+			if ifaceIncludeFilter != nil && ifaceIncludeFilter.MatchString(v.Name) {


elaborate on the use-case for having the ifaceIncludeFilter?
anyhow all VMs except excludefilter will be selected

erhudy · 2024-04-30T16:15:12Z

Sorry, this fell off the radar. The general purpose of includeFilter is to get it to select a particular interface, e.g. ens192 is the interface on our VMware VMs. That being said, I think you're right that it doesn't work exactly as intended in its present form, but it could probably be simplified to allow just specifying a particular interface name and that would avoid all the dynamic interface selection stuff entirely.

As far as testing this goes, I don't think I could run the integration test suite at that time because of some reason unrelated to the PR (it was failing on the main branch as well so probably some environment problem). The PR itself has been in use at my company since I made it, in order to filter out Calico's interfaces from selection.

erhudy · 2024-05-05T00:38:49Z

How has this been tested, please attach test logs to the PR

I simplified this PR by dropping IfaceIncludeFilter, so now it only excludes interfaces. Let me know what other changes you would like to see.

boyamurthy · 2024-05-07T09:46:16Z

How has this been tested, please attach test logs to the PR

I simplified this PR by dropping IfaceIncludeFilter, so now it only excludes interfaces. Let me know what other changes you would like to see.

hi @erhudy, could you rebase the branch with latest main.

erhudy · 2024-05-07T13:38:24Z

How has this been tested, please attach test logs to the PR

I simplified this PR by dropping IfaceIncludeFilter, so now it only excludes interfaces. Let me know what other changes you would like to see.

hi @erhudy, could you rebase the branch with latest main.

Rebased off main and fixed the linting error. I built a new container image from this branch after all my changes and am using it now in an internal test cluster, with the config option X_CSI_IFACE_EXCLUDE_FILTER="^cali.+" to exclude Calico interfaces.

boyamurthy · 2024-05-08T06:19:05Z

How has this been tested, please attach test logs to the PR

I simplified this PR by dropping IfaceIncludeFilter, so now it only excludes interfaces. Let me know what other changes you would like to see.

hi @erhudy, could you rebase the branch with latest main.

Rebased off main and fixed the linting error. I built a new container image from this branch after all my changes and am using it now in an internal test cluster, with the config option X_CSI_IFACE_EXCLUDE_FILTER="^cali.+" to exclude Calico interfaces.

Thank you @erhudy .

shanmydell · 2024-05-14T12:46:08Z

@nidtara @boyamurthy : Please review the latest changes

nidtara

lgtm

nidtara

lgtm

delldubey · 2024-06-25T10:44:44Z

@erhudy , can you rebase this with the main?
Thanks.

erhudy · 2024-06-25T23:46:07Z

@delldubey branch updated.

Unrelated to this branch, but if I have a crash report with an NPE, what's the best way to report that? The issues tracker on this repository is not open to me.

delldubey · 2024-07-12T08:28:11Z

@erhudy , here csm/issues

Under certain circumstances, net.Interfaces() will begin returning interfaces in unusual orders (e.g. if the interface index goes above 255 or 256). This could cause the driver to select the wrong interface, which in turn would mean that it could not find a matching VM in vSphere and would crash. This changeset adds an argument to control what interfaces can be selected by the driver, by allowing certain interfaces to be excluded from consideration.

shanmydell

This has been tested earlier and approved. LGTM

erhudy · 2024-10-14T23:20:55Z

What is required to complete this?

If somebody is waiting for me to provide unit test output, I cannot run the unit tests locally for reasons that do not appear to have anything to do with my changes:

( cd service; go clean -cache; CGO_ENABLED=0 GO111MODULE=on go test -v -coverprofile=c.out ./... )
=== RUN   TestApiRouter2
time="2024-10-14T23:18:31Z" level=info msg="starting http server on port :abc"
time="2024-10-14T23:18:31Z" level=error msg="unable to start http server to serve status requests due to listen tcp: lookup tcp/abc: unknown port"
time="2024-10-14T23:18:31Z" level=info msg="started http server to serve status requests at :abc"
--- PASS: TestApiRouter2 (0.00s)
=== RUN   TestApiRouter
time="2024-10-14T23:18:31Z" level=info msg="starting http server on port :8083"
time="2024-10-14T23:18:33Z" level=info msg="connectivityStatus called, status is <nil> \n"
time="2024-10-14T23:18:33Z" level=error msg="error probeStatus map in cache is empty"
time="2024-10-14T23:18:33Z" level=info msg="connectivityStatus called, status is &{{0 0} {[] {} 0x400061fb00} map[SymID2:0x40000801f0] 0} \n"
time="2024-10-14T23:18:33Z" level=error msg="invalid data is stored in cache"
time="2024-10-14T23:18:33Z" level=error msg="error invalid data is stored in cache during marshaling to json"
time="2024-10-14T23:18:33Z" level=info msg="connectivityStatus called, status is &{{0 0} {[] {} 0x40003d8330} map[SymID:0x40003de078] 0} \n"
time="2024-10-14T23:18:33Z" level=info msg="sending connectivityStatus for all arrays "
time="2024-10-14T23:18:33Z" level=info msg="GetArrayConnectivityStatus called for array SymIDNotPresent \n"
time="2024-10-14T23:18:33Z" level=info msg="GetArrayConnectivityStatus called for array SymID3 \n"
time="2024-10-14T23:18:33Z" level=error msg="error json: unsupported type: chan int during marshaling to json"
time="2024-10-14T23:18:33Z" level=info msg="GetArrayConnectivityStatus called for array SymID \n"
time="2024-10-14T23:18:33Z" level=info msg="sending response {LastSuccess:1728947913 LastAttempt:1728947913} for array SymID \n"
--- PASS: TestApiRouter (2.00s)
=== RUN   TestMarshalSyncMapToJSON
=== RUN   TestMarshalSyncMapToJSON/storing_valid_value_in_map_cache
=== RUN   TestMarshalSyncMapToJSON/storing_valid_value_in_map_cache#01
time="2024-10-14T23:18:33Z" level=error msg="invalid data is stored in cache"
--- PASS: TestMarshalSyncMapToJSON (0.00s)
    --- PASS: TestMarshalSyncMapToJSON/storing_valid_value_in_map_cache (0.00s)
    --- PASS: TestMarshalSyncMapToJSON/storing_valid_value_in_map_cache#01 (0.00s)
=== RUN   TestStartAPIService
time="2024-10-14T23:18:33Z" level=info msg="startNodeToArrayConnectivityCheck is running probes at pollingFrequency 30 "
time="2024-10-14T23:18:33Z" level=info msg="starting http server on port :8083"
time="2024-10-14T23:18:33Z" level=error msg="unable to start http server to serve status requests due to listen tcp :8083: bind: address already in use"
time="2024-10-14T23:18:33Z" level=info msg="started http server to serve status requests at :8083"
--- PASS: TestStartAPIService (0.00s)
=== RUN   TestStartAPIServiceNoPodmon
time="2024-10-14T23:18:33Z" level=info msg="podmon is not enabled"
--- PASS: TestStartAPIServiceNoPodmon (0.00s)
=== RUN   TestGoDog
starting godog...
Feature: PowerMax CSI interface
  As a consumer of the CSI interface
  I want to test controller publish / unpublish interfaces
  So that they are known to work
startup time: 2.097824036s
goroutines start aPowerMaxService new 16 old groutines 0
&{CSI-Test-Node-1 0 1 0 false false   iSCSI [iqn.1993-08.org.centos:01:5ae577b352a0] [] [] 0 0}
&{CSI-Test-Node-2 0 2 0 false false   iSCSI [iqn.1993-08.org.centos:01:5ae577b352a1 iqn.1993-08.org.centos:01:5ae577b352a2] [] [] 0 0}
&{CSI-Test-Node-3-FC 0 2 0 false false   Fibre [20000090fa9278dd 20000090fa9278dc] [] [] 0 0}
*****added** &{DEL-snapshot-1 0 false false 719899255 Established}*******Total Snaps on 00001 are: 1*********added** &{DEL-snapshot-2 0 false false 719916755 Established}*******Total Snaps on 00002 are: 1****time="2024-10-14T23:18:33Z" level=info msg="server url: http://127.0.0.1:40423"
time="2024-10-14T23:18:33Z" level=info msg="Couldn't read ../../gopowermax/mock/symmetrix46.json"
time="2024-10-14T23:18:33Z" level=error msg="Failed to fetch details for array: 000197900046. [Not Found]"
time="2024-10-14T23:18:33Z" level=info msg="Couldn't read ../../gopowermax/mock/symmetrix47.json"
time="2024-10-14T23:18:33Z" level=error msg="Failed to fetch details for array: 000197900047. [Not Found]"
time="2024-10-14T23:18:33Z" level=info msg="Couldn't read ../../gopowermax/mock/symmetrix13.json"
time="2024-10-14T23:18:33Z" level=error msg="Failed to fetch details for array: 000000000013. [Not Found]"
time="2024-10-14T23:18:33Z" level=error msg="None of the managed arrays specified are locally connected"
FAIL	github.com/dell/csi-powermax/v2/service	2.149s
FAIL
make: *** [Makefile:64: unit-test] Error 1

erhudy requested review from atye, bharadwajrs-dell, boyamurthy, delldubey, mjsdell, nidtara, prablr79, santhoshhs10, SinuChacko, shanmydell, shaynafinocchiaro and tssushma as code owners March 5, 2024 16:13

nidtara reviewed Apr 29, 2024

View reviewed changes

nidtara requested changes Apr 29, 2024

View reviewed changes

erhudy force-pushed the add-interface-filters branch from a5ce319 to 390f4a3 Compare May 5, 2024 00:37

erhudy force-pushed the add-interface-filters branch from 390f4a3 to f824df0 Compare May 6, 2024 12:35

erhudy force-pushed the add-interface-filters branch 2 times, most recently from a6d8bd0 to 1421bdf Compare May 7, 2024 12:54

nidtara previously approved these changes Jun 11, 2024

View reviewed changes

boyamurthy dismissed nidtara’s stale review via a19d9b5 June 11, 2024 10:37

boyamurthy previously approved these changes Jun 19, 2024

View reviewed changes

nidtara previously approved these changes Jun 20, 2024

View reviewed changes

erhudy dismissed stale reviews from nidtara and boyamurthy via c0e2f6e June 25, 2024 23:44

erhudy force-pushed the add-interface-filters branch from b13db66 to c0e2f6e Compare June 25, 2024 23:44

erhudy force-pushed the add-interface-filters branch from 864d91b to ce4e1ed Compare July 25, 2024 14:17

Merge branch 'main' into add-interface-filters

be36270

donatwork requested review from abhi16394, adarsh-dell, AkshaySainiDell, bpjain2004, donatwork and tdawe as code owners September 4, 2024 20:03

donatwork requested review from boyamurthy, nidtara, suryagupta4, jooseppi-luna and bjiang27 September 4, 2024 20:05

Merge branch 'main' into add-interface-filters

44fc9c3

shanmydell approved these changes Sep 12, 2024

View reviewed changes

donatwork added 3 commits September 24, 2024 06:47

Merge branch 'main' into add-interface-filters

f2a0c04

Merge branch 'main' into add-interface-filters

19338aa

Merge branch 'main' into add-interface-filters

ffc71e8

Merge branch 'main' into add-interface-filters

0b43f21

donatwork approved these changes Oct 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add way to filter network interfaces #277

fix: add way to filter network interfaces #277

erhudy commented Mar 5, 2024

boyamurthy commented Mar 7, 2024

nidtara Apr 29, 2024

nidtara left a comment

nidtara Apr 29, 2024

erhudy commented Apr 30, 2024

erhudy commented May 5, 2024

boyamurthy commented May 7, 2024

erhudy commented May 7, 2024

boyamurthy commented May 8, 2024

shanmydell commented May 14, 2024

nidtara left a comment

nidtara left a comment

delldubey commented Jun 25, 2024

erhudy commented Jun 25, 2024

delldubey commented Jul 12, 2024

shanmydell left a comment

erhudy commented Oct 14, 2024

fix: add way to filter network interfaces #277

Are you sure you want to change the base?

fix: add way to filter network interfaces #277

Conversation

erhudy commented Mar 5, 2024

Description

GitHub Issues

Checklist:

How Has This Been Tested?

boyamurthy commented Mar 7, 2024

nidtara Apr 29, 2024

Choose a reason for hiding this comment

nidtara left a comment

Choose a reason for hiding this comment

nidtara Apr 29, 2024

Choose a reason for hiding this comment

erhudy commented Apr 30, 2024

erhudy commented May 5, 2024

boyamurthy commented May 7, 2024

erhudy commented May 7, 2024

boyamurthy commented May 8, 2024

shanmydell commented May 14, 2024

nidtara left a comment

Choose a reason for hiding this comment

nidtara left a comment

Choose a reason for hiding this comment

delldubey commented Jun 25, 2024

erhudy commented Jun 25, 2024

delldubey commented Jul 12, 2024

shanmydell left a comment

Choose a reason for hiding this comment

erhudy commented Oct 14, 2024