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

feat(protonvpn): Added ProtonVPN feature selection #2182

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

kvanzuijlen
Copy link
Contributor

I know it's not completely done yet (missing tests, missing todo's for moving feature-related stuff to the new Features struct, docs, etc) but I wanted to open a PR to see if you agree with my chosen direction.

Closes #1582, partially #1103

Copy link
Owner

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

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

Nice it's looking pretty good, just a few minor comments 😉 👍

internal/configuration/settings/serverselection.go Outdated Show resolved Hide resolved
internal/configuration/settings/serverselection.go Outdated Show resolved Hide resolved
internal/configuration/settings/serverselection.go Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
internal/models/server.go Outdated Show resolved Hide resolved
internal/provider/protonvpn/updater/api.go Outdated Show resolved Hide resolved
internal/provider/protonvpn/updater/iptoserver.go Outdated Show resolved Hide resolved
internal/provider/utils/filtering.go Outdated Show resolved Hide resolved
internal/storage/filter.go Outdated Show resolved Hide resolved
@kvanzuijlen kvanzuijlen changed the title feat: Added ProtonVPN feature selection feat(protonvpn): Added ProtonVPN feature selection Mar 26, 2024
@kvanzuijlen
Copy link
Contributor Author

Hi,

I applied (most of) your suggestions from the code review. I also added minimal unit tests (basically added to all the _test files) for all files changed in this PR. I also built my own image and started using it already. It seems to work, as far as I can tell.

@kvanzuijlen
Copy link
Contributor Author

@qdm12 I've been running this code for a couple of days now myself (https://github.com/users/kvanzuijlen/packages/container/package/gluetun) and as far as I can tell it now fully works :).

@kvanzuijlen kvanzuijlen requested a review from qdm12 April 1, 2024 19:50
…iption tier

- `validateSubscriptionTierFilters` function
- `validateFeatureFilters` function
- idea introduced in qdm12#2182
- `SECURE_CORE_ONLY`
- `TOR_ONLY`
- `P2P_ONLY`
@qdm12 qdm12 force-pushed the added-protonvpn-feature-selection branch from 25deac1 to cb49a50 Compare July 29, 2024 06:38
@qdm12
Copy link
Owner

qdm12 commented Jul 29, 2024

  1. I rebased your branch on master

  2. Extracting features was actually semi-functional... but fear not, it's Go's switch which behaves strangely compared to other programming languages. When encountering a true case in the switch, it gets out of the switch and doesn't try other cases. That means if the features bits match one feature, it won't find other features. This is fixed by assigning the features struct fields directly without a switch:

    features := features{
    	secureCore: featuresBits&1 != 0,
    	tor:        featuresBits&2 != 0,
    	p2p:        featuresBits&4 != 0,
    	stream:     featuresBits&8 != 0,
    	// ipv6: featuresBits&16 != 0, - unused.
    }

    FYI this could also been fixed using the fallthrough keyword at the end of each case block in the switch 😉

  3. IPv6 only filter got removed. It's a bit of a confusing option IPV6_ONLY and also does not seem to be used.

  4. Re-updated servers data

  5. Added new environment variable options to Dockerfile

    SECURE_CORE_ONLY= \
    TOR_ONLY= \
    P2P_ONLY= \

qdm12
qdm12 previously approved these changes Jul 29, 2024
@kvanzuijlen kvanzuijlen dismissed qdm12’s stale review July 29, 2024 06:57

The merge-base changed after approval.

@qdm12 qdm12 merged commit cb99f90 into qdm12:master Jul 29, 2024
5 checks passed
@qdm12
Copy link
Owner

qdm12 commented Jul 29, 2024

@kvanzuijlen are you aware of a difference between 'p2p' and 'port forwarding' in terms of feature for Protonvpn? I'm looking at #1103 (comment) and thinking of removing P2P_ONLY in favor of PORT_FORWARD_ONLY. Also are all p2p servers able to port forward?

EDIT: Please answer on #1103 😉 Since I don't get notified for this PR anymore

@kvanzuijlen kvanzuijlen deleted the added-protonvpn-feature-selection branch July 29, 2024 14:34
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.

Feature request: Protonvpn filtering by features
2 participants