Skip to content

SNI routing from sidecar to gateway with virtual services#6402

Merged
rshriram merged 44 commits intoistio:masterfrom
ijsnellf:basic_sni_routing
Jul 3, 2018
Merged

SNI routing from sidecar to gateway with virtual services#6402
rshriram merged 44 commits intoistio:masterfrom
ijsnellf:basic_sni_routing

Conversation

@ijsnellf
Copy link
Copy Markdown
Contributor

@ijsnellf ijsnellf commented Jun 19, 2018 •
edited by rshriram
Loading

This PR adds basic SNI routing with matching of sourceLabels and mesh but no L4 attributes. Virtual service hosts must exist in the registry and the match port and destination ports must match.

The aim is to be able to support forcing traffic through the egress gateway using SNI.

apiVersion: networking.istio.io/v1alpha3
kind: ServiceEntry
metadata:
  name: wikipedia-sni
spec:
  hosts:
  - www.wikipedia.org
  ports:
    - number: 443
      protocol: TLS
      name: tls-port
  resolution: DNS
---
apiVersion: networking.istio.io/v1alpha3
kind: Gateway
metadata:
  name: istio-egressgateway
spec:
  selector:
    # DO NOT CHANGE THESE LABELS
    # The egressgateway is defined in install/kubernetes/helm/istio/values.yaml
    # with these labels
    istio: egressgateway
  servers:
  - port:
      number: 443
      name: tls
      protocol: tls
    hosts:
    - "www.wikipedia.org"
    tls:
      mode: PASSTHROUGH
---
apiVersion: networking.istio.io/v1alpha3
kind: VirtualService
metadata:
  name: force-a-to-wiki-through-egress-gateway
spec:
  hosts:
  - www.wikipedia.org
  tls:
  - match:
    - sourceLabels:
        app: a
      gateways:
      - mesh
      sniHosts:
      - "www.wikipedia.org"
    route:
    - destination:
        # use the shortname so the test doesn't depend on istio-system namespace
        host: istio-egressgateway
        port:
          number: 443
  - match:
    - gateways:
      - istio-egressgateway
      sniHosts:
      - "www.wikipedia.org"
    route:
    - destination:
        host: www.wikipedia.org
        port:
          number: 443

@istio-testing istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. approved labels Jun 19, 2018
@rshriram rshriram changed the title WIP: Basic SNI routing with virtual services WIP: Basic SNI routing from sidecar to gateway with virtual services Jun 19, 2018
@rshriram rshriram changed the title WIP: Basic SNI routing from sidecar to gateway with virtual services WIP: SNI routing from sidecar to gateway with virtual services Jun 19, 2018
GregHanson
GregHanson reviewed Jun 19, 2018
View reviewed changes
pilot/pkg/networking/core/v1alpha3/tcp.go Outdated
@@ -0,0 +1,128 @@
package v1alpha3
Copy link
Copy Markdown
Member

@GregHanson GregHanson Jun 19, 2018

Choose a reason for hiding this comment

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

missing copyright

@louiscryan louiscryan self-requested a review June 19, 2018 17:37
@louiscryan
Copy link
Copy Markdown
Contributor

louiscryan commented Jun 19, 2018 •
edited
Loading

I don't think we should overload protocol specifications in VirtS like this.

Looking at the sample you can't actually differentiate between routing TCP traffic and routing tcp-over-tls-via-SNI traffic. This lack of specificity is likely to be dangerous.

I'd rather we create a separate stanza for 'tcp-tls' in VirtualService to explicitly handle this protocol form. We're going to need to create stanzas for other protocols going forward so this shouldn't be such a big deal.

This would also line up with

#6280

@vadimeisenbergibm @rshriram

@ijsnellf
Copy link
Copy Markdown
Contributor Author

ijsnellf commented Jun 19, 2018

Refactoring to use istio/api#546...

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 20, 2018 •
edited
Loading

Codecov Report

Merging #6402 into master will decrease coverage by 1%.
The diff coverage is 8%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #6402    +/-   ##
=======================================
- Coverage      71%     71%   -<1%     
=======================================
  Files         367     369     +2     
  Lines       31956   32001    +45     
=======================================
- Hits        22575   22513    -62     
- Misses       8457    8574   +117     
+ Partials      924     914    -10
Impacted Files Coverage Δ
pilot/pkg/networking/core/v1alpha3/gateway.go 0% <ø> (ø) ⬆️
pilot/pkg/networking/core/v1alpha3/tls.go 0% <0%> (ø)
...ilot/pkg/networking/plugin/authn/authentication.go 74% <0%> (ø) ⬇️
...ilot/pkg/networking/core/v1alpha3/networkfilter.go 0% <0%> (ø) ⬆️
pilot/pkg/model/service.go 77% <0%> (ø) ⬆️
pilot/pkg/model/validation.go 86% <19%> (-3%) ⬇️
pilot/pkg/networking/core/v1alpha3/listener.go 1% <4%> (+1%) ⬆️
...olarwinds/internal/papertrail/papertrail_logger.go 59% <0%> (-21%) ⬇️
pilot/pkg/model/jwks_resolver.go 64% <0%> (-4%) ⬇️
mixer/adapter/signalfx/metrics.go 41% <0%> (-2%) ⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62d0e30...ab70852. Read the comment docs.

@vadimeisenbergibm
Copy link
Copy Markdown
Contributor

vadimeisenbergibm commented Jun 23, 2018

@ijsnellf I created a PR to document HTTPS-egress-gateway istio/istio.io#1579, see the Direct HTTPS traffic through an egress gateway.

From what I see:

  1. sni_domains is not set for the filterChains of the application pod. So the TCP filter directs all the traffic that arrives to the port 443, to the host of the ServiceEntry.
  2. Redirection to the egress gateway works (again without sni_domains in the filterChains).
  3. For the gateway pod, no tcp proxy to route to the cluster of the destination is defined. The cluster is defined.

fix authN plugin overwriting TLS context
eada09d 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@PiotrSikora
Copy link
Copy Markdown
Contributor

PiotrSikora commented Jul 3, 2018

Could you include / paste xDS generated with this change?

@rshriram
Copy link
Copy Markdown
Member

rshriram commented Jul 3, 2018

which mode? with SNI or with just plain tcp proxy?

@PiotrSikora
Copy link
Copy Markdown
Contributor

PiotrSikora commented Jul 3, 2018

@rshriram both would be great ;)

@rshriram
Copy link
Copy Markdown
Member

rshriram commented Jul 3, 2018 

 {
     "version_info": "2018-07-03 13:43:05.9141201 +0000 UTC m=+93.376140292",
     "listener": {
      "name": "0.0.0.0_19090",
      "address": {
       "socket_address": {
        "address": "0.0.0.0",
        "port_value": 19090
       }
      },
      "filter_chains": [
       {
        "filters": [
         {
          "name": "mixer",
          "config": {
           "transport": {
            "check_cluster": "outbound|9091||istio-policy.istio-system.svc.cluster.local",
            "network_fail_policy": {
             "policy": "FAIL_CLOSE"
            },
            "report_cluster": "outbound|9091||istio-telemetry.istio-system.svc.cluster.local",
            "attributes_for_mixer_proxy": {
             "attributes": {
              "source.uid": {
               "string_value": "kubernetes://a-675595579f-sdjxx.istio-system"
              }
             }
            }
           },
           "disable_check_calls": true,
           "mixer_attributes": {
            "attributes": {
             "destination.service": {
              "string_value": "headless.istio-system.svc.cluster.local"
             },
             "source.uid": {
              "string_value": "kubernetes://a-675595579f-sdjxx.istio-system"
             },
             "source.namespace": {
              "string_value": "istio-system"
             },
             "destination.service.host": {
              "string_value": "headless.istio-system.svc.cluster.local"
             },
             "context.reporter.kind": {
              "string_value": "outbound"
             },
             "context.reporter.uid": {
              "string_value": "kubernetes://a-675595579f-sdjxx.istio-system"
             }
            }
           }
          }
         },{
          "name": "envoy.tcp_proxy",
          "config": {
           "stat_prefix": "outbound|19090||headless.istio-system.svc.cluster.local",
           "cluster": "outbound|19090||headless.istio-system.svc.cluster.local"
          }
         }
        ]
       }
      ],
      "deprecated_v1": {
       "bind_to_port": false
      }
     },

TCP proxy

more tests - route via egress gateway
1eca080 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
costinm
costinm reviewed Jul 3, 2018
View reviewed changes
pilot/pkg/networking/core/v1alpha3/gateway.go Outdated
// listeners
if len(opts.filterChainOpts) == 0 {
log.Warnf("buildGatewayListeners: No filter chain options for gateway (possibly missing virtual services)")
return []*xdsapi.Listener{}, nil
Copy link
Copy Markdown
Contributor

@costinm costinm Jul 3, 2018

Choose a reason for hiding this comment

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

Metric !!!!

costinm
costinm reviewed Jul 3, 2018
View reviewed changes
pilot/pkg/networking/core/v1alpha3/gateway.go Outdated
if err != nil {
log.Errora("Failed to get services from registry")
return []*filterChainOpts{}
return nil
Copy link
Copy Markdown
Contributor

@costinm costinm Jul 3, 2018

Choose a reason for hiding this comment

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

Metric as well - errors are not visible to users, metrics may show on dashboard.

costinm
costinm reviewed Jul 3, 2018
View reviewed changes
pilot/pkg/networking/core/v1alpha3/gateway.go Outdated
rdsName := model.GatewayRDSRouteName(servers[0])
routeCfg := configgen.buildGatewayInboundHTTPRouteConfig(env, node, nameToServiceMap, gatewayNames, servers, rdsName)
if routeCfg == nil {
log.Debugf("omitting HTTP listeners for port %d filter chain due to no routes", servers[0].Port)
Copy link
Copy Markdown
Contributor

@costinm costinm Jul 3, 2018

Choose a reason for hiding this comment

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

Same.

@PiotrSikora
Copy link
Copy Markdown
Contributor

PiotrSikora commented Jul 3, 2018

@rshriram no filter chain match?

yaml fixes
2cf6cd6 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@rshriram
Copy link
Copy Markdown
Member

rshriram commented Jul 3, 2018

Filter chain match for TCP proxy is useless, as it only supports SNI. Will get you SNI routing in a few moments..

Shriram Rajagopalan added 2 commits July 3, 2018 17:13
initialize prom variables
ff3ab93 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
split tests
61161a0 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@rshriram
Copy link
Copy Markdown
Member

rshriram commented Jul 3, 2018

SNI routing via egress gateway

"dynamic_active_listeners": [
    {
     "version_info": "2018-07-03 21:36:23.514137053 +0000 UTC m=+1081.777744952",
     "listener": {
      "name": "0.0.0.0_443",
      "address": {
       "socket_address": {
        "address": "0.0.0.0",
        "port_value": 443
       }
      },
      "filter_chains": [
       {
        "filter_chain_match": {
         "server_names": [
          "www.google.com"
         ]
        },
        "filters": [
         {
          "name": "mixer",
          "config": {
           "transport": {
            "network_fail_policy": {
             "policy": "FAIL_CLOSE"
            },
            "report_cluster": "outbound|9091||istio-telemetry.istio-system.svc.cluster.local",
            "check_cluster": "outbound|9091||istio-policy.istio-system.svc.cluster.local"
           },
           "mixer_attributes": {
            "attributes": {
             "source.namespace": {
              "string_value": "istio-system"
             },
             "context.reporter.uid": {
              "string_value": "kubernetes://istio-egressgateway-c8894555f-l44gw.istio-system"
             },
             "source.uid": {
              "string_value": "kubernetes://istio-egressgateway-c8894555f-l44gw.istio-system"
             },
             "context.reporter.kind": {
              "string_value": "outbound"
             }
            }
           }
          }
         },
         {
          "name": "envoy.tcp_proxy",
          "config": {
           "stat_prefix": "outbound|443||www.google.com",
           "cluster": "outbound|443||www.google.com"
          }
         }

@rshriram
Copy link
Copy Markdown
Member

rshriram commented Jul 3, 2018

SNI routing via gateway, sidecar config

"listener": {
      "name": "0.0.0.0_443",
      "address": {
       "socket_address": {
        "address": "0.0.0.0",
        "port_value": 443
       }
      },
      "filter_chains": [
       {
        "filter_chain_match": {
         "server_names": [
          "www.google.com"
         ]
        },
        "filters": [
         {
          "name": "mixer",
          "config": {
           "transport": {
            "check_cluster": "outbound|9091||istio-policy.istio-system.svc.cluster.local",
            "network_fail_policy": {
             "policy": "FAIL_CLOSE"
            },
            "report_cluster": "outbound|9091||istio-telemetry.istio-system.svc.cluster.local"
           },
           "disable_check_calls": true,
           "mixer_attributes": {
            "attributes": {
             "context.reporter.kind": {
              "string_value": "outbound"
             },
             "context.reporter.uid": {
              "string_value": "kubernetes://a-7c8ff555d8-khgvg.istio-system"
             },
             "source.uid": {
              "string_value": "kubernetes://a-7c8ff555d8-khgvg.istio-system"
             },
             "destination.service": {
              "string_value": "www.google.com"
             },
             "source.namespace": {
              "string_value": "istio-system"
             },
             "destination.service.host": {
              "string_value": "www.google.com"
             },
             "destination.service.namespace": {
              "string_value": "istio-system"
             },
             "destination.service.name": {
              "string_value": "www.google.com"
             }
            }
           }
          }
       },
         {
          "name": "envoy.tcp_proxy",
          "config": {
           "value": {
            "stat_prefix": "outbound|443||istio-egressgateway.istio-system.svc.cluster.local",
            "route_config": {
             "routes": [
              {
               "cluster": "outbound|443||istio-egressgateway.istio-system.svc.cluster.local"
              }
             ]
            }
           },
           "deprecated_v1": true
          }
         }
        ]
       },```

@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented Jul 3, 2018 •
edited
Loading

@ijsnellf: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/istio-pilot-e2e.sh fe9c151 link /test istio-pilot-e2e
prow/e2e-bookInfoTests.sh fe9c151 link /test e2e-bookInfo
Details

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. I understand the commands that are listed here.

Shriram Rajagopalan added 3 commits July 3, 2018 18:04
final test fix hopefully
e596ec9 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
Merge branch 'master' of https://github.com/istio/istio into basic_sn…
d1a1957 
…i_routing
revert gateway tweaks
ab70852 
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@rshriram
Copy link
Copy Markdown
Member

rshriram commented Jul 3, 2018

merging.. All tests passed. CLA bot stuck

@rshriram rshriram merged commit 560c9f9 into istio:master Jul 3, 2018
@rshriram rshriram mentioned this pull request Jul 3, 2018
Closed
rshriram added a commit that referenced this pull request Jul 4, 2018
@rshriram
Cherry picking bug fixes from master into release-1.0 (#6834)
704a275 
* fix bug so that destination.service.** attributes are collected (#6801)

* remove unnecessary generated attributes finding. (#6785)

* modify docker template files for proxyv2 (#6790)

* Long-running testing improvements (#6800)

* Add values for config map settings, including access log.
More docs.

* Updates and improvements for the stress-testing configs.

* Add values for config map settings, including access log. (#6797)

* Add values for config map settings, including access log.
More docs.

* Updates and improvements for the stress-testing configs.

* Address review comments

* Merged wrong files

* Add the setup helm file - this change now depend on the previous PR.

* Sync with remote, remove accidentally added files.

* Another accidental file

* SNI routing from sidecar to gateway with virtual services (#6402)

* quick sni matching 1st pass with no refactoring of existing code

* use shriram's api sha

* quick pass at using tls block

* add some validation

* copyright

* fix lint + remove deadcode

* rename protocol tcp_tls -> tls

* update back to istio/api master

* remove accidentally added test file

* add tls block to gateway logic

* add todos

* basic sni wildcard implementation

* add tcp, fix problems with rbac, matching

* better tcp + tls validation

* address code review comments

* remove out of date comment

* update comments

* fix compile error

* use tcp proxy in tcp routing

* add tcp routing e2e test

* add forgotten vs config file + update description of test

* Comments, bug fixes

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* cleanup gateway tcp test

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* moving networking test yamls

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* tcp/tls tests

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* yaml fixes

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* fix file switcheroo

* port matches

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* fix authN plugin overwriting TLS context

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* more tests - route via egress gateway

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* yaml fixes

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* initialize prom variables

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* split tests

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* final test fix hopefully

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* revert gateway tweaks

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@ijsnellf ijsnellf deleted the basic_sni_routing branch July 5, 2018 20:57
istio-testing pushed a commit that referenced this pull request Jul 7, 2018
@quanjielin @istio-testing
merge latest master into collab-gcp-identity (#6889)
118a828 
* Introducing mcpc, a command-line  mcp client tool. (#6715)

* Set default requests cpu resource to all deployments (#6229)

* Set default requests cpu resource to all deployments

* Description added

* Requested CPU decreased to meet test env node capacity

* Setting requested cpu to 10m as most k8s platforms are defaulting to 0m

* Merge master remains

* Apply the global resources to the new gateways

* Updated goldens to cpu 10m

* Clean up and rename the global default resources

* Redundant Redis servers were removed + defer in a loop is not recommended. (#6768)

* Fix upgrade. (#6792)

* Fix upgrade.

* Format

* Move back the fail close - not clear if we have time to get mixer client in this build, we can remove in next

* Add configuration to allow transformation of durations to integers. (#5416)

The fluentd handler currently transforms durations to their string
representation for log messages. This commit adds a configuration to
override this default behaviour and tranform durations to integers of
unit millisecond.

* Add workload and service dashboard (#6789)

* Add workload and service dashboard

* fix typo

* Skip service dash e2e test for now

* Add rushed, missed workload dash

* Touch up

* Commenting out test until can replicate locally and fix

* simplify fmt script (#6798)

* simplify fmt script

- goimports is a superset of gofmt
- shouldn't need to remove spaces
- now utilizing -l flag to tell us if there is a diff

* always update gofmt to latest

* spacing that is now considered incorrect

* support hash based routing for soft session affinity (#6742)

* support sticky sessions

* fmt :(

* update servicediscovery fakes

* bump istio/api

* use updated api for ring hash

* support session affinity for sidecar

* update destination rule yaml to reflect new api

* ttl is a string

* unset sdsUdsPath in configmap (#6799)

* fix bug so that destination.service.** attributes are collected (#6801)

* remove unnecessary generated attributes finding. (#6785)

* modify docker template files for proxyv2 (#6790)

* Long-running testing improvements (#6800)

* Add values for config map settings, including access log.
More docs.

* Updates and improvements for the stress-testing configs.

* Add values for config map settings, including access log. (#6797)

* Add values for config map settings, including access log.
More docs.

* Updates and improvements for the stress-testing configs.

* Address review comments

* Merged wrong files

* Add the setup helm file - this change now depend on the previous PR.

* Sync with remote, remove accidentally added files.

* Another accidental file

* SNI routing from sidecar to gateway with virtual services (#6402)

* quick sni matching 1st pass with no refactoring of existing code

* use shriram's api sha

* quick pass at using tls block

* add some validation

* copyright

* fix lint + remove deadcode

* rename protocol tcp_tls -> tls

* update back to istio/api master

* remove accidentally added test file

* add tls block to gateway logic

* add todos

* basic sni wildcard implementation

* add tcp, fix problems with rbac, matching

* better tcp + tls validation

* address code review comments

* remove out of date comment

* update comments

* fix compile error

* use tcp proxy in tcp routing

* add tcp routing e2e test

* add forgotten vs config file + update description of test

* Comments, bug fixes

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* cleanup gateway tcp test

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* moving networking test yamls

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* tcp/tls tests

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* yaml fixes

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* fix file switcheroo

* port matches

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* fix authN plugin overwriting TLS context

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* more tests - route via egress gateway

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* yaml fixes

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* initialize prom variables

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* split tests

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* final test fix hopefully

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* revert gateway tweaks

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>

* Add myself back to OWNERS, sort the list alphabetically. (#6830)

* Upload cloudfoundry test result to GCS (#6829)

* Fix typos in command-line output.
This was referenced Jul 13, 2018
Merged
Closed
@cmluciano cmluciano mentioned this pull request Jul 14, 2018
Closed
@nmittler nmittler mentioned this pull request Jul 20, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@costinm costinm costinm left review comments

@ZackButcher ZackButcher ZackButcher approved these changes

@vadimeisenbergibm vadimeisenbergibm Awaiting requested review from vadimeisenbergibm

@louiscryan louiscryan Awaiting requested review from louiscryan

+2 more reviewers

@rshriram rshriram rshriram approved these changes

@GregHanson GregHanson GregHanson left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@costinm costinm

@ZackButcher ZackButcher

Labels

cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA.

Projects

None yet

Milestone

1.0 - launched

Development

Successfully merging this pull request may close these issues.

10 participants

@ijsnellf @louiscryan @vadimeisenbergibm @costinm @googlebot @rshriram @istio-testing @PiotrSikora @ZackButcher @GregHanson