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

Add proposal to add a VirtualHost policy to Kuma #1882

Merged
merged 3 commits into from
May 11, 2021

Conversation

lahabana
Copy link
Contributor

This enables users to define custom hosts and ports to route to service or subsets of services. IMO it's a better implementation of what was suggested in: envoy-dns.

@lahabana lahabana requested a review from a team as a code owner April 26, 2021 17:31
Signed-off-by: Charly Molter <charly@koyeb.com>
Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz left a comment

Choose a reason for hiding this comment

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

I really like the idea. I think the biggest strength with this approach is that we could help with the migration of the current deployments, especially on Universal.

kuma.io/service: "*"
version: "*"
host: "{{tag version}}.{{tag kuma.io/service}}.mesh"
port: 8080
Copy link
Contributor

Choose a reason for hiding this comment

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

we could also consider making a port a string and try to infer it from tag if needed, so

port: "{{tag kuma.io/virtual-host-port}}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought some more about this and it's actually tricky to make something not too error prone especially when you start having virtual hosts that cross multiple Dataplanes with many different tags.
I think it's also a feature that can be added later on if needed.

host: "{{tag kuma.io/service}}.{{kuma.io/zone}}.mesh"
port: 80
```

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we have

type: VirtualHost
mesh: default
name: 1
tags:
  kuma.io/service: "*"
host: "{{tag kuma.io/service}}.mesh"
port: 80
type: VirtualHost
mesh: default
name: 2
tags:
  kuma.io/service: backend
  version: 2
host: backend.mesh
port: 80

which one takes precedence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm very good point. I guess getting the most specific one is probably the best way to go about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Meaning the most matching tags?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the most match tags yes (So in this case the second) but also the most specific meaning:
This:

service: "foo"
version: "v2"

would take precedence on:

service: "foo"
version: "*"

Simple service with no wildcard replacement:

```yaml
type: VirtualHost
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little bit hesitant here on the name. Envoy uses VirtualHost in the configuration for HTTP traffic. This might be confusing for someone more advanced and familiar with Envoy.

a couple of ideas: VirtualDestination, VirtualDNS? I cannot find a good name, maybe the rest of the team will have a better idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha seems like me when I wrote the proposal :)
VirtualDNS doesn't sound right as we're adding ports as well.
VirtualOutbound doesn't sound right to me either as it's not really an outbound.
VirtualDestination is very vague but seems the least wrong of all our ideas.

Let's see if someone brings something better.

Copy link
Contributor

Choose a reason for hiding this comment

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

VirtualEndpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

VirtualHostPort?

port: 80
```


Copy link
Contributor

Choose a reason for hiding this comment

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

What if I do this

type: VirtualHost
mesh: default
name: 1
tags:
  kuma.io/service: "*"
host: "{{tag kuma.io/service}}.{{tag version}}.mesh"
port: 80

and there is no version? Will this host for service be omitted?

If so, we could support multizone access to individual instance of the application

type: VirtualHost
mesh: default
name: 1
tags:
  kuma.io/service: "*"
host: "{{tag kuma.io/service}}.{{tag kuma.io/instance}}.mesh"
port: 80

something that is missing today

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was planning on having only keys present in the tags section.
If we don't do this what's the point of the tags section?
Should it instead be a selector to restrict which dps get a virtualHost?

If was planning to use the tags tuple for indexing but maybe :: is enough?
I need to think about this some more. Let me know if you have some answers to these questions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some thinking.
tags will become selector: match similar to TracingPolicy.
I think it's reasonable for all keys used in the template to have to be present in the selector.

@lobkovilya
Copy link
Contributor

lobkovilya commented Apr 28, 2021

I like this idea very much! Few thoughts here:

  • yes, I believe it should be a policy with selector to match a subset of data plane proxies

  • I think, it looks like outbound so I believe it should have "Outbound" in the policy name. Maybe VirtualOutbound is actually what we want. See, it looks really similar to outbound on Universal, where you can manually put as many tags as you wish:

    outbound:
    - address: 192.168.0.1
      port: 80
      tags:
        kuma.io/service: backend
        kuma.io/zone: us-east
        version: 1

    So probably we should preserve this similarity and treat VirtualHosts as just additional outbounds. The simplest scenario will be:

    type: VirtualOutbound
    mesh: default
    name: backend-v1
    selectors:
     - match:
         kuma.io/service: frontend # also could be '*'
    conf:
     host: backend.v1.mesh
     port: 80
     tags:
       kuma.io/service: backend
       version: v1

    So every proxy in the frontend service will be able to consume v1 of backend on backend.v1.mesh:80.

  • For more complicated scenarios I think "*" in tags is a bit confusing when you read the policy yaml. Probably makes sense to show that we infer tags from the host rather than vice versa:

    type: VirtualOutbound
    mesh: default
    name: new-outbound-for-backend
    selectors:
      - match:
          kuma.io/service: *
    conf:
      host: {{ service }}.{{ version }}.mesh
      port: 80
      tags:
        kuma.io/service: "$service"
        version: "$version"

    I believe it's absolutely the same from the implementation perspective, but it makes people think that we construct tags based on what we've parsed from host.

  • probably we should have validation if {{ version }} is defined in the host and is not used in the tags section


- This new setup will have a different IPAM and will run concurrently to the existing one.
- Add a flag to disable the old way to allocate vips.
- Once it is proven reliable enough we will be able to add a default `virtualHost` and remove the auto generated vips.
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the plan +1

Copy link
Contributor

Choose a reason for hiding this comment

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

One more thing - Looks like we should be able to replace the ExternalServices with this mechanism entirely. Please add that to the plan if we all agree this is feasible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ExternalService is a lot more than just a DNS name no?
From: https://kuma.io/docs/1.1.4/policies/external-services/#usage seems like things like certs etc are not covered by this proposal.
Are you suggesting that in the long term a user adding an external service and wanting to access "httpbin.org" would need to add the externalServcie and then a VirtualEndpoint?

This provides us with two new features:

- Possibility to have vips with specific ports.
- Possibility to have multiple hostnames for a service or a subset of a service. This might be useful for more complex setups or simply for debugging by adding temporary policies.
Copy link
Contributor

Choose a reason for hiding this comment

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

Subset of a service? Do you suggest VIP to "/path/to/api/next" mapping also? Or it is just based on tag version: next?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You second option (tags subsets). I believe the first one should really be a rewrite path policy or something like this I don't think it should be the responsibility of this policy.

The main changes will be:
- Replace `vips` by `virtualHosts` which will be a map of a selector to an ip and a port.
- The selector for vips is: `<policyName>{key1=value1,key2=value2}` (similar to what is done for SNIs in ingress).
- Expand the `vip_allocator` to allocate vips based on tuples of matches defined in the tags section of each policy.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to have a bit of IPv6 in the mix here. If we are expanding the VIP allocation, let's just allocate IPv6 addresses next to the IPv4.
Check https://golang.org/src/net/ip.go :

// IPv4 returns the IP address (in 16-byte form) of the
// IPv4 address a.b.c.d.
func IPv4(a, b, c, d byte) IP {
	p := make(IP, IPv6len)
	copy(p, v4InV6Prefix)
	p[12] = a
	p[13] = b
	p[14] = c
	p[15] = d
	return p
}

var v4InV6Prefix = []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really should add AAAA records at the same time indeed!

@lahabana
Copy link
Contributor Author

Re name: @lobkovilya suggested VirtualOutbound.
I think it works but if we do so we'd probably want to add the hostname to the Outbound section of the DP.
Also how much are we attached to reusing the same IPs for the same hostnames on different DPs? It seems to only be a nice to have from a user point of view.
If we don't it makes the whole thing simpler as we don't have to keep a list of allocated ips.

The implementation would be very similar to what's already present for current IP support and the user would get the benefit of seeing the host/port directly in their DP's outbound.

WDYT?

@lahabana
Copy link
Contributor Author

@lobkovilya @nickolaev @jakubdyszkiewicz thank you very much for the feedback. I've just updated with your suggestions and I quite like where this is going PTAL.

Signed-off-by: Charly Molter <charly@koyeb.com>
Copy link
Contributor

@nickolaev nickolaev left a comment

Choose a reason for hiding this comment

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

I am OK with the proposal. Can't wait to see this implemented! Great stuff.

@lahabana
Copy link
Contributor Author

Here's a WIP branch: https://github.com/lahabana/kuma/tree/feat/virtual_outbound

@lobkovilya @nickolaev if you don't have any opposition I'll merge the proposal later today

@lahabana
Copy link
Contributor Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented May 10, 2021

Command update: success

Branch has been successfully updated

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ lahabana
❌ mergify[bot]
You have signed the CLA already but the status is still pending? Let us recheck it.

@lahabana lahabana merged commit 0cac048 into kumahq:master May 11, 2021
@lahabana lahabana deleted the docs/vip-policy branch March 29, 2024 12:26
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.

None yet

5 participants