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.

Sign up for GitHub

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

Jump to bottom

FXVPN-377 Fix DNS Inclusion #10257

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

FXVPN-377 Fix DNS Inclusion #10257

wants to merge 5 commits into from

Conversation

strseb
Copy link
Collaborator

@strseb strseb commented Feb 6, 2025 •
edited
Loading

Fix the DNS handling in StateOnPartial

If you put a custom DNS that is not in your LAN, it did not get added to the allowed-ip list in state-on-partial. This caused our firewall rule to catch this and block it.

strseb added 2 commits February 6, 2025 16:43
@strseb
Fix the DNS handling in StateOnPartial
b30ebaa 
wip fix dns

Add test code

FIX test code

remove unsued tests

cleanup
@strseb
cleanup
6863259
@strseb strseb requested a review from mcleinman February 6, 2025 15:52
mcleinman
mcleinman reviewed Feb 6, 2025
View reviewed changes
Copy link
Collaborator

@mcleinman mcleinman left a comment

Choose a reason for hiding this comment

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

This makes sense. Just one question out of curiosity - why a new class for this?

@strseb
Copy link
Collaborator Author

strseb commented Feb 6, 2025

For tests :) - We have in all the unit-tests a moccontroller.cpp - So any code in controller.cpp is not compiled into that unit_test binary - it seemed simpler to just add a new aux namespace that can be tested vs a refactoring controller.cpp so we can compile that into there.

@mcleinman mcleinman self-requested a review February 6, 2025 21:01
mcleinman
mcleinman approved these changes Feb 6, 2025
View reviewed changes
Copy link
Collaborator

@mcleinman mcleinman left a comment

Choose a reason for hiding this comment

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

👍🏻

@strseb strseb enabled auto-merge (squash) February 12, 2025 17:19
@github-actions github-actions bot added the 🛬 Landing This PR is marked as "auto-merge" label Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcleinman mcleinman mcleinman approved these changes

Assignees
No one assigned
Labels
🛬 Landing This PR is marked as "auto-merge"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@strseb @mcleinman