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

MEDIUM: Add support for "mode tcp" #16

Merged
merged 2 commits into from
Apr 21, 2020

Conversation

daniel-corbett
Copy link
Contributor

New frontends and backends only had support for using models.FrontendModeHTTP
and models.BackendModeHTTP which limited them to "mode http" only. This commit
adds support for "mode tcp" using both upstream and downstream specific
configuration options. It defaults to http.

To use add a "config" option within an upstream similar to the below example:

Sample downstream service file for Redis

{
  "service": {
    "name": "redis",
    "port": 6379,
    "connect": {
      "sidecar_service": {
	"proxy" : {
	  "config": { "protocol" : "tcp" }
	}
      }
    }
  }
}

Sample upstream configuration:

  "upstreams": [{
 "destination_name": "redis",
 "local_bind_port": 6379,
 "config": { "protocol" : "tcp" }
  }

Copy link
Collaborator

@pierresouchay pierresouchay left a comment

Choose a reason for hiding this comment

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

Sounds Ok to me, we have to add to CHANGELOG.md (see #23) that this is a breaking change (since using TCP instead of HTTP by default now).
Since it is intended to mimic Envoy's Consul Connect, it seems legit to me.
@ShimmerGlass Any comment

@daniel-corbett
Copy link
Contributor Author

Sounds Ok to me, we have to add to CHANGELOG.md (see #23) that this is a breaking change (since using TCP instead of HTTP by default now).
Since it is intended to mimic Envoy's Consul Connect, it seems legit to me.
@ShimmerGlass Any comment

Just to be clear this won't change the default to TCP yet. I was recommending that we make that change in the future after accepting this PR. For now, this just adds TCP but keeps default to HTTP

@pierresouchay pierresouchay added enhancement New feature or request haproxy labels Apr 14, 2020
@pierresouchay
Copy link
Collaborator

@dcorbett-haproxy yes, you are right, that's only when you'll default it to TCP in a similar way as Envoy's. Let's wait for @ShimmerGlass to review it, but I think we are good.

@pierresouchay
Copy link
Collaborator

Hello @dcorbett-haproxy ,

Since we now run the unit tests on Travis (with end-to-end tests), can you please rebase with origin ?
git pull -r origin master && git push --force ?

Thank you

@@ -153,6 +162,10 @@ func (w *Watcher) startUpstream(up api.Upstream) {
Datacenter: up.Datacenter,
}

if up.Config["protocol"] != nil {
u.Protocol = up.Config["protocol"].(string)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be a type assertion here. If "protocol" is not a string it will crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this. I added a type assertion

New frontends and backends only had support for using models.FrontendModeHTTP
and models.BackendModeHTTP which limited them to "mode http" only. This commit
adds support for "mode tcp" using both upstream and downstream specific
configuration options. It defaults to http.

To use add a "config" option within an upstream similar to the below example:

Sample downstream service file for Redis

	{
	  "service": {
	    "name": "redis",
	    "port": 6379,
	    "connect": {
	      "sidecar_service": {
		"proxy" : {
		  "config": { "protocol" : "tcp" }
		}
	      }
	    }
	  }
	}

Sample upstream configuration:

      "upstreams": [{
	 "destination_name": "redis",
	 "local_bind_port": 6379,
	 "config": { "protocol" : "tcp" }
      }
@daniel-corbett
Copy link
Contributor Author

Thanks for the review. I have rebased and made the type assertion checks suggested by @ShimmerGlass

@pierresouchay pierresouchay merged commit 09384a3 into haproxytech:master Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request haproxy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants