fix: add way to filter network interfaces #277
Conversation
erhudy
requested review from
atye,
bharadwajrs-dell,
boyamurthy,
delldubey,
mjsdell,
nidtara,
prablr79,
santhoshhs10,
SinuChacko,
shanmydell,
shaynafinocchiaro and
tssushma
as code owners
|
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 . |
main.go
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
Please rename arguments to reflect these are VM interfaces.
service/vsphere.go
Outdated
| if v.HardwareAddr.String() != "" { | ||
| return v.HardwareAddr.String(), nil | ||
| if ifaceExcludeFilter != nil || ifaceIncludeFilter != nil { | ||
| if ifaceIncludeFilter != nil && ifaceIncludeFilter.MatchString(v.Name) { |
There was a problem hiding this comment.
elaborate on the use-case for having the ifaceIncludeFilter?
anyhow all VMs except excludefilter will be selected
|
Sorry, this fell off the radar. The general purpose of includeFilter is to get it to select a particular interface, e.g. 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
force-pushed
the
add-interface-filters
branch
from
a5ce319
to
390f4a3
Compare
I simplified this PR by dropping IfaceIncludeFilter, so now it only excludes interfaces. Let me know what other changes you would like to see. |
erhudy
force-pushed
the
add-interface-filters
branch
from
390f4a3
to
f824df0
Compare
hi @erhudy, could you rebase the branch with latest main. |
erhudy
force-pushed
the
add-interface-filters
branch
2 times, most recently
from
a6d8bd0
to
1421bdf
Compare
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 |
Thank you @erhudy . |
|
@nidtara @boyamurthy : Please review the latest changes |
|
@erhudy , can you rebase this with the main? |
erhudy
force-pushed
the
add-interface-filters
branch
from
b13db66
to
c0e2f6e
Compare
|
@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. |
|
@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.
erhudy
force-pushed
the
add-interface-filters
branch
from
864d91b
to
ce4e1ed
Compare
donatwork
requested review from
abhi16394,
adarsh-dell,
AkshaySainiDell,
bpjain2004,
donatwork and
tdawe
as code owners
donatwork
requested review from
boyamurthy,
nidtara,
suryagupta4,
jooseppi-luna and
bjiang27
|
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: |
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:
Checklist:
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.