Skip to content
This repository has been archived by the owner on Jan 21, 2022. It is now read-only.

missing -d flag from https://github.com/cloudfoundry-community/port-forwarding-boshrelease/blob/master/jobs/port_forwarding/templates/bin/forward_ports.sh.erb#L28 #13

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

drnic
Copy link

@drnic drnic commented Mar 21, 2017

Discussion of issue and missing -d flag on #bosh channel https://cloudfoundry.slack.com/archives/C02HPPYQ2/p1490081101496048

@cfdreddbot
Copy link

Hey drnic!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

@@ -30,7 +30,7 @@ sysctl net.ipv4.conf.all.route_localnet=0
<% internal_ip = rule['internal_ip'] || "127.0.0.1" %>
<% internal_port = rule['internal_port'] || raise("Expected non-empty 'internal_port' on '#{rule.inspect}' rule") %>

sudo iptables -t nat -A portforwarding-release -p tcp --dport <%= external_port %> -j DNAT --to <%= internal_ip %>:<%= internal_port %>
sudo iptables -t nat -A portforwarding-release -p tcp -d <%= spec.networks.send(spec.networks.methods(false).first).ip %> --dport <%= external_port %> -j DNAT --to <%= internal_ip %>:<%= internal_port %>
Copy link
Contributor

Choose a reason for hiding this comment

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

or spec.address? https://bosh.io/docs/jobs.html#properties

It might make more sense to add support for rule['external_ip'] though (maybe falling back to spec.address, unless people would genuinely want to listen on all IPs).

@drnic
Copy link
Author

drnic commented Mar 21, 2017

@dpb587-pivotal thx, updated

@drnic
Copy link
Author

drnic commented Oct 30, 2017

This issue also manifests on the host machine - without this PR I cannot even curl google.

@drnic
Copy link
Author

drnic commented Oct 30, 2017

I'm going to log this issue within this PR. When I apply this PR to a bosh-lite, the host vm and its containers do not have same routes available to them:

api/55564424-a752-4e6a-83f3-59c958200f24:~$ curl -k https://api.10-213-13-10.sslip.io/v2/info
curl: (7) Failed to connect to api.10-213-13-10.sslip.io port 443: Connection refused
api/55564424-a752-4e6a-83f3-59c958200f24:~$ curl google.com
<HTML><HEAD><meta http-equiv="content-type" content="text/html;charset=utf-8">
<TITLE>302 Moved</TITLE></HEAD><BODY>
<H1>302 Moved</H1>
The document has moved
<A HREF="http://www.google.es/?gfe_rd=cr&amp;dcr=0&amp;ei=9p32WZ_WHo3Y8gfrzqmYBQ">here</A>.
</BODY></HTML>

But curl -k https://api.10-213-13-10.sslip.io/v2/info works from outside the host vm.

@drnic
Copy link
Author

drnic commented Oct 30, 2017

I've added changes to allow loopbacks.

@drnic
Copy link
Author

drnic commented Oct 30, 2017

Damnit, whilst curl -k https://api.10-213-13-10.sslip.io/v2/info now works from the host vm, it still does not work from within garden containers (bosh-lite instances) within that VM.

@drnic
Copy link
Author

drnic commented Oct 30, 2017

I've rebuilt my bosh-lite using the original https://github.com/cloudfoundry-community/port-forwarding-boshrelease/blob/master/jobs/port_forwarding/templates/bin/forward_ports.sh.erb#L28 but I'm still getting the same behavior - containers within the bosh-lite vm are unable to curl -k https://api.10-213-13-10.sslip.io/v2/info.

I swear this used to work. Until the middle of this year I had lots of CI pipelines that deployed things to the same bosh-lite, registered service brokers, etc.

@dpb587-pivotal
Copy link
Contributor

I'm confused; are you saying this change is no needed or working as expected?

@drnic
Copy link
Author

drnic commented Nov 1, 2017 via email

@drnic
Copy link
Author

drnic commented Nov 9, 2017

@dpb587-pivotal this PR can be merged; the remaining issue of containers having egress access to host loopback can be solved later

@drnic
Copy link
Author

drnic commented Nov 10, 2017

I have closure on the unresolved issue above that I claimed use to work. It never worked. I've searched thru my CI history and found jobs where I used to have "it working" - but it turns out I was not running bosh errands, rather I was hard coding errand functionality into concourse tasks - so I had ingress traffic from a 3rd party client (concourse) rather than bosh-lite containers (bosh errands) trying for egress traffic to the host machine.

This PR is good to merge @dpb587-pivotal @cppforlife - or any final comments.

drnic added a commit to cloudfoundry-community/collection-of-pullrequests-boshrelease that referenced this pull request Nov 10, 2017
@drnic
Copy link
Author

drnic commented Dec 22, 2017

@cppforlife @dpb587-pivotal can we merge this please and cut a release?

@drnic
Copy link
Author

drnic commented Mar 21, 2018

@dpb587-pivotal @cppforlife this is the 1 year birthday of this ticket (I was randomly explaining why this PR exists and noticed its creation date). Could we merge + cut a release?

@@ -22,17 +22,19 @@ fi

iptables -F ${CHAIN} || true

# Reset in case when there is no localhost routing
sysctl net.ipv4.conf.all.route_localnet=0
sysctl net.ipv4.conf.all.route_localnet=1
Copy link
Contributor

Choose a reason for hiding this comment

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

i would prefer if this stays as is, conditional.

sudo iptables -t nat -A portforwarding-release -p tcp -d <%= external_ip %> --dport <%= external_port %> -j DNAT --to <%= internal_ip %>:<%= internal_port %>

# loopback
sudo iptables -t nat -A portforwarding-release -p tcp -d 127.0.0.1 --dport <%= external_port %> -j DNAT --to <%= internal_ip %>:<%= internal_port %> -o lo
Copy link
Contributor

Choose a reason for hiding this comment

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

this should really be done thru port_forwarding config, instead of hard coded.

<% internal_ip = rule['internal_ip'] || "127.0.0.1" %>
<% internal_port = rule['internal_port'] || raise("Expected non-empty 'internal_port' on '#{rule.inspect}' rule") %>
<%
external_ip = rule['external_ip'] || spec.address
Copy link
Contributor

Choose a reason for hiding this comment

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

spec.address -> spec.ip. spec.address may be a dns name.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants