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

Make VPN port configurable in api service via environment variable #271

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matiasAS
Copy link

@matiasAS matiasAS commented Jul 2, 2024

Description
This pull request modifies the docker-compose.yml file to allow the VPN port of the api service to be configurable via an environment variable (VPN_PORT). If the environment variable is not set, the default value 443 will be used.

Reason

Hetzner server
Proxmox
pfSense for networking
Nginx Proxy Manager as a reverse proxy for services
Cloudflare in front
I encountered the following error on devices connecting to the VPN:

Jun 30 04:05:08 a179fab openvpn[6532]: 2024-06-30 04:05:08 WARNING: Bad encapsulated packet length from peer (18516), which must be > 0 and <= 1627 -- please ensure that --tun-mtu or --link-mtu is equal on both peers -- this condition could also indicate a possible active attack on the TCP link -- [Attempting restart...]

An alternative solution to getting a new dedicated public IP for the virtual machine hosting OpenBalena was to change the VPN port to 4443, and then set up port forwarding from the public IP of pfSense to port 443 of the virtual machine.

From my research, the error was due to using pfSense and/or Nginx Proxy Manager along with Cloudflare, causing OpenVPN to "confuse" it with an HTTPS connection.

I want this change to avoid modifying the docker-compose.yml file directly and to prevent errors when updating with git pull due to file modifications.

The ideal and correct solution might be to use a dedicated IP, but I also did this to save money, even if it's a little less than 2 euros; it's still worth it, right?

Best regards,
Matias Alvarez Sabate

@ab77
Copy link
Contributor

ab77 commented Jul 2, 2024

Could you please add a change-type (see 52d0eb6)

@matiasAS
Copy link
Author

matiasAS commented Jul 2, 2024

I've added the Change-type: minor to the commit message as requested. The changes are now ready for review. Thank you!

@ab77
Copy link
Contributor

ab77 commented Jul 2, 2024

We don't support merged commits in the CI workflow, see here. Can you please squash your work down to a single commit, annotated with the change-type property, rebase on master and re-push..

@matiasAS matiasAS force-pushed the configurable-vpn-port-api branch 2 times, most recently from 1af60eb to 45a29dd Compare July 2, 2024 15:42
@matiasAS
Copy link
Author

matiasAS commented Jul 2, 2024

I've squashed the commits into a single commit and added Change-type: minor. The branch has also been rebased on master. The changes are now ready for review.
Sorry for the inconvenience, this is the first time I collaborate on an opensource project.

Thank you!

@ab77 ab77 force-pushed the configurable-vpn-port-api branch from 45a29dd to 2d6c858 Compare July 2, 2024 16:08
@ab77 ab77 added the ok-to-test label Jul 2, 2024
@flowzone-app flowzone-app bot enabled auto-merge July 2, 2024 16:14
@ab77 ab77 had a problem deploying to compose-private-pki July 2, 2024 16:14 — with GitHub Actions Failure
@ab77 ab77 had a problem deploying to balena-public-pki July 2, 2024 16:14 — with GitHub Actions Failure
@ab77 ab77 had a problem deploying to compose-private-pki July 2, 2024 16:28 — with GitHub Actions Failure
@ab77 ab77 had a problem deploying to balena-public-pki July 2, 2024 16:28 — with GitHub Actions Failure
@ab77 ab77 had a problem deploying to compose-private-pki July 2, 2024 17:23 — with GitHub Actions Failure
@ab77 ab77 had a problem deploying to balena-public-pki July 2, 2024 17:23 — with GitHub Actions Failure
@ab77
Copy link
Contributor

ab77 commented Jul 2, 2024

Fails tests due to commit being out of tree, need to think about how to solve this for external contributors..

+ sudo -u balena git config --global --add safe.directory /home/balena/open-balena
+ cd /home/balena/open-balena
+ sudo -u balena git checkout 2d6c85804ce7d707a10d858dad817e259c071383
fatal: reference is not a tree: 2d6c85804ce7d707a10d858dad817e259c071383

auto-merge was automatically disabled July 2, 2024 17:50

Head branch was pushed to by a user without write access

@matiasAS
Copy link
Author

matiasAS commented Jul 2, 2024

With the help of chat gpt, I have updated the fork:
1)git remote add upstream https://github.com/balena-io/open-balena.git
2)git fetch upstream
3)git rebase upstream/master
4)git push -f origin configurable-vpn-port-api

Will that be enough?

.gitignore Outdated Show resolved Hide resolved
@ab77 ab77 force-pushed the configurable-vpn-port-api branch from 5ac70b3 to 90d20a9 Compare July 2, 2024 21:40
@ab77 ab77 self-requested a review July 2, 2024 21:40
@ab77 ab77 added the ok-to-test label Jul 2, 2024
@flowzone-app flowzone-app bot enabled auto-merge July 2, 2024 21:44
@ab77 ab77 had a problem deploying to compose-private-pki July 2, 2024 21:45 — with GitHub Actions Failure
@ab77 ab77 had a problem deploying to balena-public-pki July 2, 2024 21:45 — with GitHub Actions Failure
@@ -108,7 +108,7 @@ services:
TOKEN_AUTH_JWT_ALGO: ES256
TOKENS_CONFIG: API_SERVICE_API_KEY:hex,AUTH_RESINOS_REGISTRY_CODE:hex,COOKIE_SESSION_SECRET:hex,JSON_WEB_TOKEN_SECRET:hex,MIXPANEL_TOKEN:hex,SUPERUSER_PASSWORD:hex,TOKEN_AUTH_BUILDER_TOKEN:hex,VPN_GUEST_API_KEY:hex,VPN_SERVICE_API_KEY:hex,API_VPN_SERVICE_API_KEY:API_SERVICE_API_KEY,REGISTRY2_TOKEN:TOKEN_AUTH_BUILDER_TOKEN
TRUST_PROXY: 172.16.0.0/12
VPN_PORT: 443
VPN_PORT: ${VPN_PORT:-443}
Copy link
Contributor

Choose a reason for hiding this comment

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

@matiasAS thank you, there are a few blockers on our side in relation to your PR, one I've already mentioned in this comment and the other is the way balena-cli currently handles env-var interpolation (different to compose). We are working on resolving both of these blockers and once we have a resolution, we should be able to hopefully merge this work.

Copy link
Author

@matiasAS matiasAS Jul 2, 2024

Choose a reason for hiding this comment

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

ok, is the blocking of the previous comments ok now?

Is the problem with the environment variable related to the fact that the version of Docker Compose is 2.4 and that way of defining it is for a more current version like 3.8, for example?

Copy link
Author

Choose a reason for hiding this comment

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

@ab77
Sorry for asking again, do I have to do something else, or just wait for the other part of the environment variables to be resolved? Greetings

Copy link
Author

Choose a reason for hiding this comment

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

Hello @ab77 , any progress with my suggestion of making the VPN port customizable?

Copy link
Contributor

@ab77 ab77 Aug 12, 2024

Choose a reason for hiding this comment

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

Great, now only the balena-ci blocker remains, this is on us @matiasAS: balena-io/balena-cli#2818

Copy link
Author

Choose a reason for hiding this comment

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

@ab77
Excellent, I liked contributing to the project, even if it was a little small.

I am developing on my own an api in fast api that communicates with the openbalena api in cases where at least 3 requests have to be made, everything is simplified with 1.

Copy link
Author

Choose a reason for hiding this comment

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

Hello @ab77 , will my suggested change be applied one day or never?

Copy link
Contributor

Choose a reason for hiding this comment

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

@matiasAS we can't merge this change until there is support for env-var interpolation at the balena-compose level.

Copy link
Author

Choose a reason for hiding this comment

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

@ab77 do you like my proposal? Who does that depend on? I understand that it is not mandatory for my proposal to be accepted and perhaps I am the only openbalena user who needs it, I say this because if I manually modify docker compose and then do a git pull to update openbalena, an error will be generated in the merge, that is the only reason why I want you to apply that change

@ab77 ab77 force-pushed the configurable-vpn-port-api branch from 90d20a9 to cecfddb Compare August 12, 2024 16:26
@ab77 ab77 self-requested a review August 12, 2024 16:27
Copy link

github-actions bot commented Aug 12, 2024

Website deployed to CF Pages, 👀 preview link https://f4d61b54.open-balena.pages.dev

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