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

canary: service with multiple ports gets routed to first port declared #263

Closed
fcantournet opened this issue Jul 30, 2019 · 9 comments
Closed
Labels
kind/bug Something isn't working

Comments

@fcantournet
Copy link
Contributor

fcantournet commented Jul 30, 2019

TL;DR: canary makes my grpc traffic on $service.$namespace go to the http port in a service with 2 ports.

I'm using the Canary CRD with a service that exposes 2 ports, one for http and one for grpc :

apiVersion: v1
kind: Service
metadata:
  name: places
  namespace: trip
  ownerReferences:
  - apiVersion: flagger.app/v1alpha3
    blockOwnerDeletion: true
    controller: true
    kind: Canary
    name: places
    uid: 30d633f1-ad67-11e9-a8c3-4201c0a8800a
spec:
  clusterIP: 10.250.134.101
  ports:
  - name: http
    port: 8080
    protocol: TCP
    targetPort: 8080
  - name: grpc
    port: 6565
    protocol: TCP
    targetPort: 6565
  selector:
    app: places-primary
  sessionAffinity: None
  type: ClusterIP

The Canary CRD :

apiVersion: flagger.app/v1alpha3
kind: Canary
metadata:
  annotations:
    flux.weave.works/antecedent: trip:helmrelease/places
  creationTimestamp: "2019-07-23T16:30:32Z"
  generation: 1
  labels:
    app: places
  name: places
  namespace: trip
  resourceVersion: "66111554"
  selfLink: /apis/flagger.app/v1alpha3/namespaces/trip/canaries/places
  uid: 30d633f1-ad67-11e9-a8c3-4201c0a8800a
spec:
  autoscalerRef:
    apiVersion: autoscaling/v2beta1
    kind: HorizontalPodAutoscaler
    name: places
  canaryAnalysis:
    interval: 5s
    maxWeight: 50
    metrics:
    - interval: 1m
      name: request-success-rate
      threshold: 99
    - interval: 1m
      name: request-duration
      threshold: 500
    stepWeight: 10
    threshold: 10
    webhooks:
    - metadata:
        cmd: test places --cleanup
        type: helm
      name: helm test
      timeout: 3m
      type: pre-rollout
      url: http://flagger-helmtester.kube-system/
  progressDeadlineSeconds: 60
  service:
    port: 8080
    portDiscovery: true
    trafficPolicy:
      tls:
        mode: DISABLE
  skipAnalysis: true
  targetRef:
    apiVersion: apps/v1
    kind: Deployment
    name: places

Following the documentation in the FAQ I have set portDiscovery to true.

If I query place-primary.trip:6565 it works as expect and I get the GRPC endpoint (and respectively for 8080 and http)

However if I query places.trip:6565 I end up on the http 8080 port of the pods (instead of the grpc one).

istioctl pc route $(k get pods -l app=synapse-trip -o jsonpath='{.items[0].metadata.name}') --name 6565 -o json

            {
                "name": "places.trip.svc.cluster.local:6565",
                "domains": [
                    "places.trip.svc.cluster.local",
                    "places.trip.svc.cluster.local:6565",
                    "places",
                    "places:6565",
                    "places.trip.svc.cluster",
                    "places.trip.svc.cluster:6565",
                    "places.trip.svc",
                    "places.trip.svc:6565",
                    "places.trip",
                    "places.trip:6565",
                    "10.250.134.101",
                    "10.250.134.101:6565"
                ],
                "routes": [
                    {
                        "match": {
                            "prefix": "/"
                        },
                        "route": {
                            "cluster": "outbound|8080||places-primary.trip.svc.cluster.local",
                            "timeout": "0s",
                            "retryPolicy": {
                                "retryOn": "connect-failure,refused-stream,unavailable,cancelled,resource-exhausted,retriable-status-codes",
                                "numRetries": 2,
                                "retryHostPredicate": [
                                    {
                                        "name": "envoy.retry_host_predicates.previous_hosts"
                                    }
                                ],
                                "hostSelectionRetryMaxAttempts": "3",
                                "retriableStatusCodes": [
                                    503
                                ]
                            },
                            "maxGrpcTimeout": "0s"
                        },
                        "metadata": {
                            "filterMetadata": {
                                "istio": {
                                    "config": "/apis/networking/v1alpha3/namespaces/trip/virtual-service/places"
                                }
                            }
                        },
                        "decorator": {
                            "operation": "places-primary.trip.svc.cluster.local:8080/*"
                        },
                        "perFilterConfig": {
                            "mixer": {
                                "disable_check_calls": true,
                                "forward_attributes": {
                                    "attributes": {
                                        "destination.service.host": {
                                            "string_value": "places-primary.trip.svc.cluster.local"
                                        },
                                        "destination.service.name": {
                                            "string_value": "places-primary"
                                        },
                                        "destination.service.namespace": {
                                            "string_value": "trip"
                                        },
                                        "destination.service.uid": {
                                            "string_value": "istio://trip/services/places-primary"
                                        }
                                    }
                                },
                                "mixer_attributes": {
                                    "attributes": {
                                        "destination.service.host": {
                                            "string_value": "places-primary.trip.svc.cluster.local"
                                        },
                                        "destination.service.name": {
                                            "string_value": "places-primary"
                                        },
                                        "destination.service.namespace": {
                                            "string_value": "trip"
                                        },
                                        "destination.service.uid": {
                                            "string_value": "istio://trip/services/places-primary"
                                        }
                                    }
                                }
                            }
                        }
                    }
                ]
            },

As you can see the route in envoy ends up routing grpc traffic on port 6565 to the 8080 cluster for some reason.
I suspect this is a bug.

I'm using version 0.17.0 of flagger and istio 1.1.11

@stefanprodan
Copy link
Member

Hi @fcantournet
Can you post the virtual service generated by Flagger?

kubectl -n trip get virtualservice/places -oyaml

@fcantournet
Copy link
Contributor Author

Sure ! (also edited the initial post for version)

apiVersion: networking.istio.io/v1alpha3
kind: VirtualService
metadata:
  creationTimestamp: "2019-07-23T16:30:36Z"
  generation: 1
  name: places
  namespace: trip
  ownerReferences:
  - apiVersion: flagger.app/v1alpha3
    blockOwnerDeletion: true
    controller: true
    kind: Canary
    name: places
    uid: 30d633f1-ad67-11e9-a8c3-4201c0a8800a
  resourceVersion: "65133890"
  selfLink: /apis/networking.istio.io/v1alpha3/namespaces/trip/virtualservices/places
  uid: 32faef14-ad67-11e9-8566-4201c0a8800b
spec:
  gateways:
  - mesh
  hosts:
  - places
  http:
  - route:
    - destination:
        host: places-primary
        port:
          number: 8080
      weight: 100
    - destination:
        host: places-canary
        port:
          number: 8080
      weight: 0

@stefanprodan
Copy link
Member

stefanprodan commented Jul 30, 2019

Hmm looks like Istio takes all ports from the ClusterIP definition and maps them to the destination port number when the ClusterIP name matches a virtual service name... The places-canary and places-primary will have both ports and routes set correctly. My intention was to make only a port available with the virtual service but seems that Istio will map all of them...

@fcantournet
Copy link
Contributor Author

To be 100% certain I understand and the intended behavior matches what I want to do (regardless of current potentiel bug) : the intended way is to have clients of places target places.trip:6565 and get routed as the places owners configure it ( and not target explicitely places-primary.trip:6565 as a client)

Correct ?

I suspect the problem to be here :
https://github.com/weaveworks/flagger/blob/677ee8d63967e57c8de9d1556efff4b92449d292//pkg/router/istio.go

The virtual service actually needs match rules for each port and matching destinations for each port apparently.

@stefanprodan
Copy link
Member

Can you please do the following test: scale Flagger to zero and edit the virtual service:

spec:
  gateways:
  - mesh
  hosts:
  - places
  http:
  - route:
    - destination:
        host: places-primary
        port:
          number: 8080
      weight: 100
    - destination:
        host: places-canary
        port:
          number: 8080
      weight: 0
  - route:
    - destination:
        host: places-primary
        port:
          number: 6565
      weight: 100
    - destination:
        host: places-canary
        port:
          number: 6565
      weight: 0

@fcantournet
Copy link
Contributor Author

Yeah I'll do that test as soon as possible and get back to you 👍

@fcantournet
Copy link
Contributor Author

fcantournet commented Jul 30, 2019

Ok So I tried with your yaml and it didn't work.

Then I added a match rule with the port and it works as expected now :

  gateways:
  - mesh
  hosts:
  - places
  http:
  - match:
    - port: 8080
    route:
    - destination:
        host: places-primary
        port:
          number: 8080
      weight: 100
    - destination:
        host: places-canary
        port:
          number: 8080
      weight: 0
  - match:
    - port: 6565
    route:
    - destination:
        host: places-primary
        port:
          number: 6565
      weight: 100
    - destination:
        host: places-canary
        port:
          number: 6565
      weight: 0

@stefanprodan
Copy link
Member

Hmm ok this complicates things as users can add match conditions to the canary spec. Flagger would need to carefully merge those.

Thanks @fcantournet for the report 👍

@stefanprodan stefanprodan added the kind/bug Something isn't working label Jul 30, 2019
fcantournet added a commit to fcantournet/flagger that referenced this issue Aug 1, 2019
This fixes issue fluxcd#263

We actually don't need to specify any ports in the VirtualService
and DestinationRules.
Istio will create clusters/listerners for each named port we have declared in
the kubernetes services and the router can be shared as it operates only on L7 criterias

Also contains a tiny clean-up of imports
fcantournet added a commit to fcantournet/flagger that referenced this issue Aug 2, 2019
This fixes issue fluxcd#263

We actually don't need to specify any ports in the VirtualService
and DestinationRules.
Istio will create clusters/listerners for each named port we have declared in
the kubernetes services and the router can be shared as it operates only on L7 criterias

Also contains a tiny clean-up of imports
@stefanprodan
Copy link
Member

Fixed by #267 release in v0.18.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants