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

[release/7.0] Fix filtered exceptions when checking for ip forwarding #81157

Merged
merged 1 commit into from
Feb 11, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jan 25, 2023

Closes #81061, #75883.

Backport of #76383 to release/7.0

/cc @rzikm

Customer Impact

Regression in 7.0 from 6.0 breaks customers who need to query IP forwarding details on some constrained systems where IP forwarding information is not available (the appropriate file /proc/sys/net/ipv(4|6)/conf/{interface}/forwarding does not exist).
A specific example would be Orleans users running in gVisor - see #81061 for more details.
It will also help WSL1 (which is not officially supported) - see #75883.

Testing

Manual testing in WSL1 environment. The affected method no longer throws the exception.

Risk

Low. The change is small and there have been no related problems observed in main (8.0) after merging PR #76383 on 2022/9/30.

@ghost
Copy link

ghost commented Jan 25, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #76383 to release/7.0

/cc @rzikm

Customer Impact

Testing

Risk

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Net

Milestone: -

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

@karelz karelz added this to the 7.0.x milestone Jan 31, 2023
@rzikm
Copy link
Member

rzikm commented Feb 1, 2023

Test failure seems unrelated. (Microsoft.VisualBasic.Core.Tests)

@rzikm
Copy link
Member

rzikm commented Feb 1, 2023

@ElanHasson Would you be able to test the fix in your repro?

The process is:

  • publish your app as self-contained dotnet publish --self-contained (but not single-file)
  • replace System.Net.NetworkInformation.dll with artifact from this PR (I grabbed only the linux flavor of the dll to reduce zip size, let me know if you need other).
  • deploy and test inside gVisor.

@karelz karelz added the Servicing-consider Issue for next servicing release review label Feb 7, 2023
@karelz
Copy link
Member

karelz commented Feb 7, 2023

@ElanHasson any chance to try if it really fixes the problem in your specific environment?
It would be a great help for us, thank you!

@rbhanda rbhanda modified the milestones: 7.0.x, 7.0.4 Feb 7, 2023
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 7, 2023
@ElanHasson
Copy link

@rzikm @karelz yeah, been away from work for a while.

I'll go ahead and give this is a try and report back. Linux x64 is fine.

Thanks!

@carlossanlop
Copy link
Member

Approved by Tactics for 7.0.4.
Signed-off by area owners.
CI failure unrelated: #81544
No OOB changes needed for System.Net.NetworkInformation.
This is ready to merge, but...

@karelz @rzikm @wfurt would you like us to wait for @ElanHasson 's confirmation before I merge this?

@wfurt
Copy link
Member

wfurt commented Feb 9, 2023

I think it is OK to merge @carlossanlop . The fix is pretty small so let's assume it will work as expected. It is best interest of @ElanHasson to still do the validation and let us know if it does not for some reason.

@karelz
Copy link
Member

karelz commented Feb 9, 2023

@ElanHasson it would be really, really useful for next servicing release if you could check it works in next ~24 hours. Do you think it is possible?
Thanks! And sorry for the push.

@karelz karelz closed this Feb 9, 2023
@karelz karelz reopened this Feb 9, 2023
@karelz
Copy link
Member

karelz commented Feb 9, 2023

Argh! Accidental misclick on Close PR, sorry! ;(

@ElanHasson
Copy link

ElanHasson commented Feb 10, 2023

@karelz Yep! It's deploying shortly

ElanHasson added a commit to web-scheduler/web-scheduler that referenced this pull request Feb 10, 2023
ElanHasson added a commit to web-scheduler/web-scheduler that referenced this pull request Feb 10, 2023
@ElanHasson
Copy link

@karelz @rzikm @wfurt @wfurt @carlossanlop Thanks! This works on my end in same gVisor environment that had the issue when I reported.

Thank you!

@carlossanlop carlossanlop force-pushed the backport/pr-76383-to-release/7.0 branch from 9b70d1f to dc23afb Compare February 10, 2023 22:26
@carlossanlop carlossanlop merged commit 18c6bf0 into release/7.0 Feb 11, 2023
@carlossanlop carlossanlop deleted the backport/pr-76383-to-release/7.0 branch February 11, 2023 13:24
@ghost ghost locked as resolved and limited conversation to collaborators Mar 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants