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

Allow for binding HTTP or TCP ingress rules to specific addresses #649

Merged
merged 2 commits into from
Nov 14, 2017
Merged

Allow for binding HTTP or TCP ingress rules to specific addresses #649

merged 2 commits into from
Nov 14, 2017

Conversation

deuill
Copy link
Contributor

@deuill deuill commented Oct 21, 2017

Currently, Voyager will allow (or enforce, in the case of TCP rules) specifying a specific port to
expose services against, regardless of their internal service port. This commit extends this
functionality by allowing for specifying an optional bind address (IPv4 or IPv6), which defaults to
a wildcard ('*'), which binds to all available addresses.

An example 'Ingress' definition might look like:

apiVersion: voyager.appscode.com/v1beta1
kind: Ingress
metadata:
  name: ingress
  namespace: default
  labels:
	app: voyager
  annotations:
	ingress.appscode.com/type: HostPort
spec:
  rules:
	- host: deuill.org
	  http:
		address: 203.0.113.101
		paths:
		  - backend:
			  serviceName: deuill.web
			  servicePort: 80
	- host: mail.deuill.org
	  tcp:
		address: 203.0.113.102
		port: 25
		backend:
		  serviceName: postfix.mail
		  servicePort: 25

Noted that wildcard and non-wildcard rules cannot be mixed for the same external port number, due to
how the underlying HAProxy configuration is set up. This essentially means that, if you have a
single HTTP host binding to a specific address, all other HTTP hosts must specify an address as
well.

@deuill
Copy link
Contributor Author

deuill commented Oct 21, 2017

Tests are coming once this has gone though a first round of design review.

@deuill
Copy link
Contributor Author

deuill commented Oct 21, 2017

Some design rationale:

I initially wanted to allow for specifying a range of IPs, similar to how externalIPs works for service definitions. However this would complicate the code quite a bit, since a single rule could now end up needing to write to multiple frontends (since other rules may specify the same address for the same port). It would also make the address definition non-orthogonal to the port definition, which only allows for a single port binding.

Overall the design here satisfies my needs: the ability to have a service expose itself on a single, specific address and port. Not sure if I should go the extra mile and potentially complicate the code unnecessarily,

var wildcard = fmt.Sprintf("*:%d", port)
if address == "*" {
if _, ok := defined[wildcard]; !ok && len(defined) > 0 {
return fmt.Errorf("cannot define wildcard on same port as specific bind address")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feel free to propose a better error message, not sure if this is self-explanatory enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't use wildcard address for port %d, since ones or more rules define specific bind address.

@tamalsaha tamalsaha self-requested a review October 22, 2017 15:22
Copy link
Contributor

@tamalsaha tamalsaha left a comment

Choose a reason for hiding this comment

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

Thanks @deuill . I think the overall design looks good to me. Added few comments.

func checkExclusiveWildcard(address string, port int, defined map[string]*address) error {
var wildcard = fmt.Sprintf("*:%d", port)
if address == "*" {
if _, ok := defined[wildcard]; !ok && len(defined) > 0 {
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 check for entries that have <non-*-addr>:port defined.

Copy link
Contributor Author

@deuill deuill Oct 23, 2017

Choose a reason for hiding this comment

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

Apologies if I'm misunderstanding the comment, but this is what this code does (unless, of course, I've botched it). Specifically, there's two cases:

If we're attempting to register a wildcard address, check whether there's an existing wildcard address registered for the same port. If not, check whether there's any other rules registered. If there are, we can assume they're specific addresses, and thus fail out.

If we're attempting to register a non-wildcard address, check whether there's an already-existing wildcard address. If there is, fail out.

Pretty sure this covers everything, right?

P.S.: I should probably add some comments here regardless of outcome.

Copy link
Contributor

Choose a reason for hiding this comment

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

If not, check whether there's any other rules registered. If there are, we can assume they're specific addresses, and thus fail out.

This defined map will have rules for all other ports, too. So, I think you need to check the there are other rules for the same port but with specific bind address, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, correct, my mistake. I'll fix.

var wildcard = fmt.Sprintf("*:%d", port)
if address == "*" {
if _, ok := defined[wildcard]; !ok && len(defined) > 0 {
return fmt.Errorf("cannot define wildcard on same port as specific bind address")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't use wildcard address for port %d, since ones or more rules define specific bind address.

}
} else {
if _, ok := defined[wildcard]; ok {
return fmt.Errorf("cannot define specific bind address on same port as wildcard")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't use specific bind address for port %d, since ones or more rules use wildcard bind address.

@tamalsaha
Copy link
Contributor

Hey, any progress on this?

@deuill
Copy link
Contributor Author

deuill commented Nov 6, 2017

Apologies for the delay, I was out last week and didn't have access to my test server. I'll fix the issues here and fix conflicts.

Thanks for your patience.

@deuill
Copy link
Contributor Author

deuill commented Nov 7, 2017

Pushed fixes for the comments, tests are still WIP (I'll need to set up E2E tests and see about implementation).

@tamalsaha
Copy link
Contributor

@deuill can you please rebase master and squash your commits? If it is ok with you we can add the tests and merge the change so that your contribution is clear.

@deuill
Copy link
Contributor Author

deuill commented Nov 10, 2017

@tamalsaha sure, no problem. Again, apologies for the lack of timely updates here, I appreciate you pushing this through!

deuill and others added 2 commits November 14, 2017 12:34
Currently, Voyager will allow (or enforce, in the case of TCP rules) specifying a specific port to
expose services against, regardless of their internal service port. This commit extends this
functionality by allowing for specifying an optional bind address (IPv4 or IPv6), which defaults to
a wildcard ('*'), which binds to all available addresses.

An example 'Ingress' definition might look like:

	apiVersion: voyager.appscode.com/v1beta1
	kind: Ingress
	metadata:
	  name: ingress
	  namespace: default
	  labels:
		app: voyager
	  annotations:
		ingress.appscode.com/type: HostPort
	spec:
	  rules:
		- host: deuill.org
		  http:
			address: 203.0.113.101
			paths:
			  - backend:
				  serviceName: deuill.web
				  servicePort: 80
		- host: mail.deuill.org
		  tcp:
			address: 203.0.113.102
			port: 25
			backend:
			  serviceName: postfix.mail
			  servicePort: 25

Noted that wildcard and non-wildcard rules cannot be mixed for the same external port number, due to
how the underlying HAProxy configuration is set up. This essentially means that, if you have a
single HTTP host binding to a specific address, all other HTTP hosts must specify an address as
well.

Closes: #602
@tamalsaha tamalsaha merged commit 65e1115 into voyagermesh:master Nov 14, 2017
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.

3 participants