Fix default of router.status.port in jobs/gorouter/spec

[nota-ja]

The status port value is described as 8080 in the spec,
but it is actually 8082 in the source code of gorouter.

cf.

• https://github.com/cloudfoundry/gorouter/blob/1d44eeb855da9def0af9c32baef2c9be321d4dd6/config/config.go#L31
  (the commit 1d44eeb is referred from the HEAD of the latest develop branch of routing-release)

[cfdreddbot]

Hey nota-ja!

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.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/140086223

The labels on this github issue will be updated when the story is started.

[shashwathi]

@nota-ja :
gorouter source code defaults are meant for developing purpose. I do not think we should update status port on release level to match source code defaults.

We have a story in backlog to cleanup defaults and as a part of that story we will remove source code defaults and add a sample config to run gorouter locally.

@shalako @abbyachau opinions ? Thoughts?

Regards
Shash

[nota-ja]

If we don't specify router.status.port, router.status.user, and router.status.password explicitly in our bosh deployment manifest, the router(s) become listening the port 8082 (not 8080) in the deployed environment.

I think it is very confusing because the word "default" generally means the value used when we don't specify explicity.

However, if you don't think that is confusing, I will respect your opinion.

[aaronshurley]

@nota-ja

As @shashwathi mentioned, we do have a story to sync up the "default" config with what we provide in job specs: https://www.pivotaltracker.com/story/show/139315175

We don't see the behavior of listening on port 8082 instead of 8080. We have a few questions:

• Could you please provide some output to show that port 8082 is in use?
• Could you give us more details on your deployment (such as IaaS, bosh vs bosh-lite, etc.)?
• How do you generate your manifest?
• On the router VM in /var/vcap/jobs/gorouter/config/gorouter.yml, which port do you see for status.port?

[nota-ja]

@aaronshurley

As @shashwathi mentioned, we do have a story to sync up the "default" config with what we provide in job specs: https://www.pivotaltracker.com/story/show/139315175

Sounds great!

We don't see the behavior of listening on port 8082 instead of 8080. We have a few questions:

• Could you please provide some output to show that port 8082 is in use?

I found this behavior when I was deploying gorouter and doppler collocated in a same VM.
But the doppler job failed.

Then I found the following logs from /var/vcap/sys/log/doppler/doppler.stderr.log:

2017/02/16 13:53:30 Startup: Setting up the doppler server
2017/02/16 13:53:30 dropsondeUnmarshallerCollection: created 5 unmarshallers
2017/02/16 13:53:30 Listening for GRPC connections on 8082
2017/02/16 13:53:30 Failed to start listener (port=8082) for gRPC: listen tcp :8082: bind: address already in use
panic: Failed to create doppler: listen tcp :8082: bind: address already in use
(..omit..)

Doppler tried to listen the port 8082 for GRPC (this is the default), but the port was already in use.

So I ran netstat and found the port 8082 was used by gorouter:

# netstat -ltnp
Active Internet connections (only servers)
Proto Recv-Q Send-Q Local Address           Foreign Address         State       PID/Program name
tcp        0      0 127.0.0.1:8400          0.0.0.0:*               LISTEN      8992/consul
tcp        0      0 10.0.0.24:28081         0.0.0.0:*               LISTEN      9253/trafficcontrol
tcp        0      0 127.0.0.1:33331         0.0.0.0:*               LISTEN      941/bosh-agent
tcp        0      0 127.0.0.1:8500          0.0.0.0:*               LISTEN      8992/consul
tcp        0      0 127.0.0.1:53            0.0.0.0:*               LISTEN      8992/consul
tcp        0      0 0.0.0.0:22              0.0.0.0:*               LISTEN      1544/sshd
tcp        0      0 127.0.0.1:17016         0.0.0.0:*               LISTEN      9446/ssh-proxy
tcp        0      0 127.0.0.1:2822          0.0.0.0:*               LISTEN      2027/monit
tcp        0      0 127.0.0.1:2825          0.0.0.0:*               LISTEN      941/bosh-agent
tcp        0      0 127.0.0.1:6060          0.0.0.0:*               LISTEN      9253/trafficcontrol
tcp        0      0 127.0.0.1:17005         0.0.0.0:*               LISTEN      9411/file-server
tcp        0      0 127.0.0.1:6061          0.0.0.0:*               LISTEN      9066/metron
tcp        0      0 10.0.0.24:8301          0.0.0.0:*               LISTEN      8992/consul
tcp        0      0 0.0.0.0:41582           0.0.0.0:*               LISTEN      590/rpc.statd
tcp        0      0 0.0.0.0:111             0.0.0.0:*               LISTEN      540/rpcbind
tcp6       0      0 :::8080                 :::*                    LISTEN      9411/file-server
tcp6       0      0 :::80                   :::*                    LISTEN      9135/gorouter
tcp6       0      0 :::8082                 :::*                    LISTEN      9135/gorouter
tcp6       0      0 :::22                   :::*                    LISTEN      1544/sshd
tcp6       0      0 :::59717                :::*                    LISTEN      590/rpc.statd
tcp6       0      0 :::17003                :::*                    LISTEN      9135/gorouter
tcp6       0      0 :::2222                 :::*                    LISTEN      9446/ssh-proxy
tcp6       0      0 :::2223                 :::*                    LISTEN      9446/ssh-proxy
tcp6       0      0 :::111                  :::*                    LISTEN      540/rpcbind

• Could you give us more details on your deployment (such as IaaS, bosh vs bosh-lite, etc.)?

I am using OpenStack as IaaS.
This is my bosh manifest used for the deployment (confidential information has been reducted).

The part commented out is in question.

• How do you generate your manifest?

Manually.

• On the router VM in /var/vcap/jobs/gorouter/config/gorouter.yml, which port do you see for status.port?

This is the generated YAML file (confidential information has been reducted). There's no configuration for status.port.

I think that is reasonable because of the source code of gorouter.yml.erb, and thus gorouter uses the default port 8082.

[flawedmatrix]

@shalako @abbyachau @nota-ja

The reason this is happening is that if all three of router.status.port, router.status.user, and router.status.password are not present, the gorouter.yml will not have a status field and will fall back on gorouter source code defaults (which uses port 8082).

https://github.com/cloudfoundry-incubator/routing-release/blob/52d75abb9006d950fed0de6d79eef81d7efc7b65/jobs/gorouter/templates/gorouter.yml.erb#L2-L4

https://github.com/cloudfoundry/gorouter/blob/1d44eeb855da9def0af9c32baef2c9be321d4dd6/config/config.go#L31

How about removing the status endpoint in Gorouter if the manifest does not explicitly contain all three properties, instead of having a secret unexpected endpoint (which also happens to have no authentication)?

[nota-ja]

@flawedmatrix

How about removing the status endpoint in Gorouter if the manifest does not explicitly contain all three properties, instead of having a secret unexpected endpoint (which also happens to have no authentication)?

I have no objection, because I don't yet have any deployment of Cloud Foundry in production.
However, the change will probably break the current expectation of other deployers, so I think it should be carefully done.

[shalako]

I would suggest the manifest template require a value for status.password and fail the deployment before rolling the router if a value is not provided. This way the template defaults for status.port and status.password always apply.

We must listen on some port for /routes, /health, and /varz.

[nota-ja]

@shalako

I would suggest the manifest template require a value for status.

That means also to delete "default" value for router.status.port from develop/jobs/gorouter/spec?
Is my understanding right?
If so, it's OK for me and I will close this pull request.

[shalako]

I think we'd keep the default port 8080 as that is providing the healthcheck endpoint for production deployments.

I'm proposing we add a default user and require a password (with no default).

[nota-ja]

@shalako
Now I think I've understood what you wrote.

So providing default values for router.status.user and router.status.password, generated gorouter.yml will always have configuration for router.{port,user,pass} even when one of them are omitted, and thus the default port will also be effective.

One last question just from curiosity.
Why gorouter.yml requires all the three value for custom configuration. If this code were something like the following, I think this issue may not occur.

status:
  <% if_p("router.status.port") %>
  port: <%= p("router.status.port") %>
  <% end %>
  <% if_p("router.status.user") %>
  user: <%= p("router.status.user" %>
  <% end %>
  <% if_p("router.status.password") %>
  pass: "<%= p("router.status.password") %>"
  <% end %>

[aaronshurley]

@nota-ja Regarding your last question, health check port, username and password were considered a tuple so either the manifest values would be picked if all three are present otherwise source code defaults take precedence. We do see your point in wanting to only change a subset of these properties.

Do you agree with @shalako in requiring a password but we can set a default user?

Would you like to update the PR for these changes? Otherwise, it will be a feature request and a story will be created for it.

Thanks,
Aaron && @shashwathi

[nota-ja]

I agree with all you (the routing-release team) have decided with this issue.
I would not update this pull request so would you please file the propsed change as a feature request.
And thus, I would close this pull request right now.