Add istio-virtualservice source

[alfredkrohmer]

This adds a source for istio VirtualServices. DNS names are given via the hosts field in the VirtualService, the target for the DNS record is extracted from the associated istio Gateway.

Note: This also changes the behavior of istio Gateways:

• it now finds the Kubernetes Service corresponding to the gateway by searching for Services which have the same selector as the Gateway; previously it used the Gateway selector to query for services directly via label selector; I did this change because the Gateway selector selects Pods, not Services
• it now only finds corresponding Kubernetes Services in the same namespace as the Gateway; while it is actually possible to use Gateways to configure ingress gateway pods in other namespace, this is contrary to what's written in the documentation; I opened up a bug report for this here: Selector of Gateway custom resource is not bound by namespace istio/istio#19970
  I did these changes because I had to touch a bit of the code for the gateway source anyways. Let me know if this is out of scope and I should create a separate PR for this.

This closes #1340.

• make unit tests work
• add documentation

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: devkid
To complete the pull request process, please assign hjacobs
You can assign the PR to them by writing /assign @hjacobs in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rverma-jm]

Amazing, lets get this merged fast :)

[alfredkrohmer]

/assign @hjacobs

Please also see the description of the PR regarding the changes for istio Gateways.

[njuettner]

@devkid please one more rebase and sorry about that but I'm currently looking through a lot of PR's, which might take some time 🙏 .

[a8j8i8t8]

@devkid If it's taking long to merge this, is there a docker image for your code where I can test this feature?

[alfredkrohmer]

@a8j8i8t8 I'll rebase this week and probably revert the changes regarding the gateway again - I don't really want to introduce a breaking change with this PR. You can just build a docker image from my branch to test this (would be appreciated by the way 😉).

[TomasKohout]

I'm so glad that this exists. We are now facing issues with wildcard certificate and need this one to get closed soon. :-D Fingers crossed. :)

[TomasKohout]

one

[TomasKohout]

@devkid
I've tested your changes and it seems that there is some issue. Here is what I have

apiVersion: networking.istio.io/v1alpha3
kind: Gateway
metadata:
  name: test-wildcard
  namespace: istio-system
spec:
  selector:
    istio: ingressgateway
  servers:
  - port:
      number: 80
      name: http
      protocol: HTTP
    hosts:
    - '*.example.domain'
---
apiVersion: networking.istio.io/v1alpha3
kind: VirtualService
metadata:
  name: test-wildcard-virtual-service
  namespace: istio-system
spec:
  hosts:
  - 'test.example.domain'
  gateways:
  - test-wildcard
  http:
  - route:
    - destination:
        port:
          number: 12345
        host: some-host.some-namespace.svc.cluster.local

Recods in Route53 are not changed and app throw out a warning that says this:
2020-02-12T14:16:43.343606Z warn gateways.networking.istio.io "test-wildcard" not found

[alfredkrohmer]

@TomasKohout Did you adjust the RBAC permissions for external-dns to allow access to Istio Gateway and VirtualService resources? The error message in external-dns can be confusing.

[a8j8i8t8]

@devkid Somehow it's not working for me :-(
I checked out your branch LogMeIn:feature/istio-virtualservice-source and built docker image with some tag using command docker build -t sometag ..
I started external-dns with this image and below configuration.

containers:
    - name: external-dns
    image: docker-registry.ajit.nl/a.bansode/external-dns:latest
    args:
    - --source=istio-virtualservice
    - --domain-filter=dev.ajit.nl
    - --provider=aws
    - --policy=upsert-only
    - --registry=txt
    - --txt-owner-id=aprdev

I'm getting below log for my virtualservice.
No endpoints could be generated from virtualservice some-name/kubernetes-dashboard
My virtualservice is defined like below.

apiVersion: networking.istio.io/v1alpha3
kind: VirtualService
metadata:
  name: kubernetes-dashboard
spec:
  hosts:
  - kmd.dev.ajit.nl
  gateways:
  - my-gateway
  http:
  - route:
    - destination:
        host: kubernetes-dashboard
        port:
          number: 9090

Any help will be appreciated.
Cheers,
~Ajit

[TomasKohout]

@TomasKohout Did you adjust the RBAC permissions for external-dns to allow access to Istio Gateway and VirtualService resources? The error message in external-dns can be confusing.

I haven't. It's working out of the box for gateways.

[alfredkrohmer]

@TomasKohout can you share the manifest of the Service and Deployment of your ingress gateway deployment in istio-system? Do you see additional messages when you run external-dns with debug logging? (If didn't run it as such already.) The warning that you see is weird to me because it doesn't come from my code directly. With:

I haven't. It's working out of the box for gateways.

Do you mean you didn't adjust the RBAC permissions to allow access to VirtualServices? Because that's required, obviously.

@a8j8i8t8 Can you share the manifest of your Gateway and the Service and Deployment of the corresponding ingress gateway deployment in istio-system? Are VirtualService, Gateway and the ingress gateway pods located in the same namespace?

[a8j8i8t8]

@devkid I'll share it in a bit. But is it required to have VirtualService, Gateway and the ingress gateway pods located in the same namespace?

@a8j8i8t8 Can you share the manifest of your Gateway and the Service and Deployment of the corresponding ingress gateway deployment in istio-system? Are VirtualService, Gateway and the ingress gateway pods located in the same namespace?

[alfredkrohmer]

@a8j8i8t8 With the current implementation, it is required to have the Service and the pods of the ingress gateway deployment and the Gateway definition in the same namespace (most likely istio-system). The VirtualService can reside in another namespace, but if it does, you need to specify the gateway like:

apiVersion: networking.istio.io/v1alpha3
kind: VirtualService
metadata:
  name: kubernetes-dashboard
spec:
  gateways:
  - <namespace of gateway>/<name of gateway>
  # ...

Also, the hosts field of your Gateway must somehow match the hosts entries in your VirtualService. In the example from @TomasKohout, the Gateway has *.example.domain and the VirtualService has test.example.com, so this should be ok. Setting the hosts in Gateway to * should also work.

[alfredkrohmer]

I have rebased now, but I would still like to revert my changes to the gateway source before merging this. I'll let you know @njuettner, most likely next week.

[TomasKohout]

Hi, I'll have futher a look on a Monday. :-)

[alfredkrohmer]

It adds an app: istio-ingressgateway label to the Service selector, which was missing from the Gateway. It is possible that a new version of Istio adds this label, and it wasn't present in the version @devkid's testing with. Once I had my selectors in line, everything worked perfectly.

Hmmmm, I don't remember exactly. Now that you mention it, it might be that I added this manually on my end as well. That would be an explanation on why it wasn't working for others. I was using version 1.4.0 but according to git blame the selectors haven't really been changed since a while. @a8j8i8t8 @TomasKohout could you verify if this fixes it for you?

Do you folks / maintainers have an opinion on this change of label selector behavior? Currently the mapping Gateway → Service (before and still with this PR) is kind of guesswork because these two are not really related.

• Old behavior: it uses the selector of the Gateway to match labels of a Service; this works by chance because the default istio deployment comes with these matching; if one would mess with / create more (unrelated) services in the istio-system namespace with matching selectors, this could go very wrong
• New behavior: it uses the selector of the Gateway to find a Service with the same selector; this apparently doesn't work with the default istio deployment but would be slightly "more correct" than the old approach; there are cases were this might fail to select a Service it should select (e.g. Service selector more restrictive than Gateway selector, like we have seen it) but it also would prevent to select a Service it must not select (e.g. Gateway selector more restrictive than Service selector)

Yet another option would be to get all pods that match the Gateway selector, verify that all share the exact same label set, then find a service whose selector would select this label set. If there are pods with different label sets or more than one service found, error out and don't to anything. In this case, an explicit target annotation on the Gateway would be required. Thoughts on this?

As for the (new) restriction that now a Gateway resource can only refer to workloads in the same namespace, this was acknowledged as a bug by istio: istio/istio#19970 (comment) and I assume this will be fixed shortly. Hence I would like to keep this change. But in case this gets released, there needs to be large warning about this breaking change.

[elsesiy]

@devkid I added the following snippets to identify matching gateway-service selectors which works fine:

// gatewaySelectorMatchesService ensures that all labels of the gateway are present on the load balancer service
func gatewaySelectorMatchesService(gwSelector, lbSelector map[string]string) bool {
	for k, v := range gwSelector {
		if lbl, ok := lbSelector[k]; ok && lbl == v {
			continue
		} else {
			return false
		}
	}

	return true
}

The targetsFromGatewayConfig would subsequently use this instead when looping through the services:

if !gatewaySelectorMatchesService(gateway.Selector, service.Spec.Selector) {
	continue
}

In our case we deployed istio using helm so the gateway has the selector istio=xyzgatway and the service has app=istio, release=istio, istio=xyzgateway. That's why the initial PR didn't work for us.

[alfredkrohmer]

I assume this could work. It would only match services whose selector is at least as restrictive as the gateway selector, therefor this would ensure that no unwanted pods / services are selected by external-dns.

[elsesiy]

Yep, I was going to open a PR based on your work as this was stale and we needed this but I'm happy if you incorporate this in here 👍

I also added tests for it and did some other minor changes, let me know if I should send you the patch for it.

[alfredkrohmer]

@elsesiy I incorporated your snipped 👍 Could you open a PR to merge your changes into my branch?

[ophelan]

I've been following this PR and find one scenario it doesn't address - An Istio VirtualService using an ALB provisioned by the aws-alb-ingress-controller. In this case, the Service is generally a NodePort and thus doesn't track the LB endpoint (and if provisioned as a LoadBalancer the external-ip never resolves); only the Ingress itself has these attributes.

Is this a use case that should be targeted here? I'm currently addressing this with a modified version of the previous release; haven't yet attempted to rebase the targetsFromGatewayConfig changes yet.

[alfredkrohmer]

@ophelan We have essentially the same setup. You would need to create a DNS name for your ingress and can then put an external-dns.alpha.kubernetes.io/target annotation on the Gateway with the DNS name of your ingress as the value. This way the DNS names of your Virtual services will be CNAMEs (or in AWS optionally alias records) pointing to the DNS name of your ingress (which in turn would point to the ALB DNS name).

What we could also implement: adding an annotation to the gateway to refer to a specific service or ingress and extract the LB status from them (instead of all the guesswork).

[alfredkrohmer]

@ophelan You could check out if this would be useful for you (feedback wanted): https://github.com/LogMeIn/external-dns/compare/feature/istio-virtualservice-source...LogMeIn:feature/istio-virtualservice-source-with-target-from?expand=1

[alfredkrohmer]

@a8j8i8t8 @TomasKohout could you please check if the fixes on this branch work for your setup now?

[TomasKohout]

Hi,

I'm sorry for late reply, but I don't have a capacity to do that. :(

[ophelan]

@devkid I had a chance to give your gateway annotation patch a try. In NewIstioVirtualServiceSource, the ingressInformer initialization process is missing the cache population wait, but after adding that it appears to work perfectly for my simple test cases. Will play with a bit more extensively tomorrow.

[elsesiy]

@ophelan @devkid Is there anything missing now? I'm happy to help out if needed so we can get this merged soon, thanks!

[k8s-ci-robot]

@devkid: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[tariq1890]

@devkid Would you be able to rebase this? external-dns now uses istio/client-go. This hopefully should simplify things

[thomasv314]

@devkid @elsesiy would just like to mention we've been running this in multiple accounts/workloads over the past 1.5 months and it's working great.

If I can do anything to help get this in master please let me know!

[tariq1890]

Hey all. I've created a rebased version of this PR #1607

We are now using a much newer version of kubernetes and istio client-go (as opposed to dependening on the main istio repo)

[steerben]

Would love to have support for virtual services. Testet it and looks good already. Hopefully it's merged soon.