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

[client] Add NB_SKIP_SOCKET_MARK & fix crash instead of returing an error #2899

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

nazarewk
Copy link
Contributor

Describe your changes

I found some changes laying around on my computer when asked about #2530 (debugging my mobile router), can as well upstream those:

  • allows disabling SO_MARK without disabling other custom routing features
  • makes dialer return an error instead of crashing the process with log.Fatalf

Issue ticket number and link

#2530

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • Extended the README / documentation, if necessary

@lixmal lixmal self-requested a review November 18, 2024 19:29
@nazarewk
Copy link
Contributor Author

I am not sure what is it with failing tests, doesn't seem like failures are related to my changes.

util/grpc/dialer.go Outdated Show resolved Hide resolved
util/grpc/dialer.go Outdated Show resolved Hide resolved
util/grpc/dialer.go Outdated Show resolved Hide resolved
util/grpc/dialer.go Outdated Show resolved Hide resolved
util/net/dialer_nonios.go Outdated Show resolved Hide resolved
util/net/dialer_nonios.go Outdated Show resolved Hide resolved
util/net/net_linux.go Outdated Show resolved Hide resolved
util/net/net_linux.go Outdated Show resolved Hide resolved
util/net/net_linux.go Outdated Show resolved Hide resolved
}

// Check for the new environment variable
if skipSocketMark := os.Getenv(envSkipSocketMark); skipSocketMark == "true" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we add the env check here as well

return os.Getenv("NB_USE_LEGACY_ROUTING") == "true" || nbnet.CustomRoutingDisabled()

This way we will fallback to exclusion routes automatically (instead of using fwmarks), to avoid routing loops.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean NB_SKIP_SOCKET_MARK=true should automatically imply NB_USE_LEGACY_ROUTING=true?

@nazarewk nazarewk force-pushed the allow-disabling-somark branch 4 times, most recently from 1478e01 to c9f9533 Compare November 19, 2024 11:51
@nazarewk nazarewk requested a review from lixmal November 19, 2024 11:52
@nazarewk nazarewk force-pushed the allow-disabling-somark branch from c9f9533 to bfa4772 Compare November 19, 2024 12:01
@lixmal
Copy link
Contributor

lixmal commented Nov 19, 2024

I am not sure what is it with failing tests, doesn't seem like failures are related to my changes.

Can you merge main? There's been an issue with a domain for the tests

util/grpc/dialer.go Outdated Show resolved Hide resolved
@nazarewk nazarewk force-pushed the allow-disabling-somark branch from 9c41d2a to 634596a Compare November 19, 2024 12:53
fix created during investigation of netbirdio#2530
@nazarewk nazarewk force-pushed the allow-disabling-somark branch from 634596a to 4ed0d72 Compare November 19, 2024 12:53
@lixmal lixmal changed the title add NB_SKIP_SOCKET_MARK & fix crash instead of returing an error [client] Add NB_SKIP_SOCKET_MARK & fix crash instead of returing an error Nov 19, 2024
@lixmal
Copy link
Contributor

lixmal commented Nov 19, 2024

Thanks a lot

@lixmal lixmal merged commit eb5d056 into netbirdio:main Nov 19, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants