Start GRPC server using native go stack, cleanup auth #4867
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
liamawhite
reviewed
Apr 11, 2018
| @@ -15,10 +15,13 @@ | |||
| package bootstrap | |||
There was a problem hiding this comment.
Maybe this file should just be called config? Its already in the bootstrap package so is currently stuttering.
There was a problem hiding this comment.
Also this seems like a weird directory to have pilot (agent) code? Shouldn't this be somewhere like pilot/pkg/bootstrap/agent?
There was a problem hiding this comment.
Rename: I agree, but not a priority now. When we deprecate v1 we'll refactor/split this.
I'm trying to remove 'agent' from pilot - the code may be merged with node_agent (from security),
and it doesn't have many dependencies on pilot (or even envoy - technically can start any process with a template).
liamawhite
reviewed
Apr 11, 2018
pkg/bootstrap/bootstrap_config.go
Outdated
| Certificates: []tls.Certificate{chainCert}, | ||
| RootCAs: caCertPool, | ||
| ServerName: PilotSAN, | ||
| // Original ServerName was fmt.Sprintf("%v.svc", serverName), but not sure |
There was a problem hiding this comment.
That evaluated to istio-pilot.istio-system.svc, which is what you have in your pilotsan variable.
There was a problem hiding this comment.
Only in some cases - raw VMs use 'istio-pilot', and for multi-cluster/zVPN the DNS of pilot may be something else.
We may need to refine this for the case pilot is running in a different namespace.
There is a PR to make cert generation more flexible - so pilot can have multiple names in the cert.
liamawhite
reviewed
Apr 11, 2018
pkg/bootstrap/bootstrap_config.go
Outdated
| // PilotSAN is the DNS name included in the pilot certificate. It may not match the | ||
| // actual address. Pilot, IstioCA and Sidecar have special certificate that include both | ||
| // SPIFEE and DNS. | ||
| PilotSAN = "istio-pilot.istio-system.svc" |
There was a problem hiding this comment.
I assume this is not going to work in environments not named Kube? That was my assumption when I wrote it originally. We might have to do some work later to get it to work with other SRs.
There was a problem hiding this comment.
Could you stick a TODO there so its a bit more obvious that it may be an issue down the line?
There was a problem hiding this comment.
Why wouldn't it work ? That's the name in the cert - it should work anywhere.
Maybe a better option is to fix the ca to not use '.svc' in the name, it may be confusing.
|
Made some comments. Mostly just some of my thoughts, nothing particularly blocking. |
rshriram
approved these changes
Apr 11, 2018
There was a problem hiding this comment.
Turn on control plane auth in helm and let’s see if tests pass.
cc @ZackButcher or anybody else familiar with the gRPC server specific things.
My only concern is the arbitrary 5s sleep and retry. May be turn it into a retry twice and die for this PR. Later, we could set bootstrap deadlines such that pilot fails if it couldn’t start server after X seconds in total.
Approving subject to fixing 5s loop and v1alpha3 passing.
|
|
||
| // PilotCertDir is the default location for mTLS certificates used by pilot | ||
| // Visible for tests - at runtime can be set by PILOT_CERT_DIR environment variable. | ||
| PilotCertDir = "/etc/certs/" |
There was a problem hiding this comment.
Why not use IstioCert... variable that has this ?
There was a problem hiding this comment.
In model ? It's a constant - need to override it for tests. And that's the location for workload
certs - pilot may use a different directory ( for example if it is exposed for zvpn and uses real certs ).
The delay is based on what we saw in tests - it takes a while for certs to show up.
| certDir+model.KeyFilename) | ||
| // certs not ready yet. | ||
| if err != nil { | ||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
This seems arbitrary. May be panic and fail or simply retry twice and fail
There was a problem hiding this comment.
In the flaky tests we've seen >1 min delay until the cert shows up. I can make it configurable ?
There was a problem hiding this comment.
Sure.. with a bound - thats all I am asking.. else, some installation might be perpetually spinning waiting for certs, without giving any proper message to user. A panic after 4 minutes is easily noticeable..
There was a problem hiding this comment.
Right now it's 50 seconds - but I don't think panic is right unless controlPlaneAuthPolicy is on.
|
I'll check if we can make some changes to how the pilot cert is generated, need to figure out what to do |
Codecov Report
@@ Coverage Diff @@
## master #4867 +/- ##
=======================================
- Coverage 74% 74% -<1%
=======================================
Files 310 310
Lines 25920 26115 +195
=======================================
+ Hits 19010 19095 +85
- Misses 6138 6246 +108
- Partials 772 774 +2
Continue to review full report at Codecov.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
wattli
reviewed
Apr 17, 2018
| if len(nameDomain) == 2 { | ||
| override, ok := webhooks[nameDomain[0]] | ||
| if ok { | ||
| override.CustomDomains = append(override.CustomDomains, nameDomain[1]) |
There was a problem hiding this comment.
The variable 'override' is not used.
There was a problem hiding this comment.
override.CustomDomains is modified: if we already have an entry, we add an additional domain to it.
| @@ -274,6 +280,23 @@ func runCA() { | |||
| Namespace: opts.istioCaStorageNamespace, | |||
There was a problem hiding this comment.
And since you are already here, consolidate this webhookServiceNames/webhookServiceAccounts with the customerDNSNames?
There was a problem hiding this comment.
Separate PR maybe ? Didn't want to add risks to 0.8 by touching code that is used. We can cleanup in 0.9.
|
PTAL - I think the tests are finally passing ! |
rshriram
approved these changes
Apr 19, 2018
|
Why do we need a secure GRPC port when we have envoy in front of pilot ? |
|
Several reasons:
- we can use the 'native' http2 stack (which is more up to date and in
theory better) - but it only works with https
- so we can remove the envoy in front of pilot :-) - once 0.9 is out we'll
not need it for v1, and it doesn't help for v2
since telemetry is different, envoy can't look inside the stream ( will use
prom directly or direct mixer calls ).
- it allow easier testing for the https port and the AZ retrieval ( the PR
also includes certs and tests for that)
Not sure what is the order...
…On Thu, Apr 19, 2018 at 5:58 PM, Laurent Demailly ***@***.***> wrote:
Why do we need a secure GRPC port when we have envoy in front of pilot ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4867 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAFI6m_fy6NVDipMdKOt4qbM9J6MqyyHks5tqTK1gaJpZM4TPPlb>
.
|
|
That sounds contrary to our aspirational goal of self-applying istio
telemetry. Why can't we have both internal metrics and istio-provided
metrics? Pilot is not a particularly complicated service, so as a user, I
would be concerned if there's some problem with envoy in front of it.
On Thu, Apr 19, 2018 at 6:09 PM Costin Manolache <notifications@github.com>
wrote:
… Several reasons:
- we can use the 'native' http2 stack (which is more up to date and in
theory better) - but it only works with https
- so we can remove the envoy in front of pilot :-) - once 0.9 is out we'll
not need it for v1, and it doesn't help for v2
since telemetry is different, envoy can't look inside the stream ( will use
prom directly or direct mixer calls ).
- it allow easier testing for the https port and the AZ retrieval ( the PR
also includes certs and tests for that)
Not sure what is the order...
On Thu, Apr 19, 2018 at 5:58 PM, Laurent Demailly <
***@***.***>
wrote:
> Why do we need a secure GRPC port when we have envoy in front of pilot ?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#4867 (comment)>, or
mute
> the thread
> <
https://github.com/notifications/unsubscribe-auth/AAFI6m_fy6NVDipMdKOt4qbM9J6MqyyHks5tqTK1gaJpZM4TPPlb
>
> .
>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#4867 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AJGIxk10Kb7XzRYwwEVDFaGY3gN-MOPjks5tqTVigaJpZM4TPPlb>
.
|
|
Part of our mission is to make GRPC work well as a first class supported protocol; I don't see why envoy-pilot GRPC calls would not work well / or what that says to our users. |
ozevren
added a commit
that referenced
this pull request
Apr 23, 2018
* Do not add jwt and authn filter for TCP listner type (they are http filters) (#5034) * Improve virtual service validation (#5027) Automatic merge from submit-queue. Improve virtual service validation Adds more tests to virtual service validation and fixes a few issues. This PR improves the coverage but more test cases need to be added. * istioctl: fix default namespace handling for (de)register commands (#4493) * istioctl: fix default namespace handling for (de)register commands PersistentPreRun is only invoked for the leaf subcommand in the command chain and parent PersistentPreRun are skipped. In case of (de)register's PersistentPreRun, this skipped default namespace handling. Fortunatly, (de)register was only calling getRealKubeConfig which was already handled by the parent command. * fix lint errors * Removed duplicate core v1 import. (#4957) Automatic merge from submit-queue. Removed duplicate core v1 import. * update the --grpc-host-identities=istio-ca to preserve the default behavior. (#4972) Automatic merge from submit-queue. update the --grpc-host-identities=istio-ca to preserve the default behavior. We still need to set default value of --grpc-host-identities=istio-ca to support mesh expansion customer. * Support multiple route rules versions in Bookinfo e2e tests (#4896) * move route-rule-reviews-90-10 and route-rule-reviews-80-20 with all other routing rules otherwise, complex logic will be required to test them on v1alpha3 and v1alpha2 * added v1alpha3 versions of route-rule-reviews-80-20/90-10 * add TestFlags to e2e test framework * add reprocessRule * split demo_test.go into demo_test.go and main_test.go * refactor: extract getPreprocessedRulePath * add TestFlags definition and initialization * remove adding default rules from setUpDefaultRouting * remove deleting default rules on testConfig cleanup * remove adding default routing rule * print the migration rate in TestVersionMigration * add setting default rules in TestVersionMigration * ignore an error in deleting a rule that was deleted before ignore the error that will happen since the fifty rule redefines "reviews-default" rule and is deleted * add default rules to test version routing and test fault * add handling of rules config versions * refactor: extract allRules variable * add reviewsDestinationRule for v1Alpha3 * Skip -> Skipf * Remove forCA from CsrRequest. (#5018) Automatic merge from submit-queue. Remove ForCA from CsrRequest. ForCA is not used by Citadel any more. Instead, Citadel decides whether to sign certificate for workload or CA from the flag `sign-ca-certs`: https://github.com/istio/istio/blob/master/security/cmd/istio_ca/main.go#L218 * Use pod IP instead of host IP (#4988) Automatic merge from submit-queue. Correct IP for pilot debug tool. * Remove vendor files from .gitignore (#5048) * update Go control plane (#5053) Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com> * updated istio/api repo version (#5050) * updated istio/api repo version * code review comments * Add myself to install/OWNERS (#5046) Automatic merge from submit-queue. Add myself to install/OWNERS * cleanup unused files (#5042) Automatic merge from submit-queue. cleanup unused files istio-pilot-e2e-v1alpha3 is not run anymore. istio-pilot-e2e-envoyv2-v1alpha3 replaced that test * Don't build anything in the init phase (#5035) Automatic merge from submit-queue. Don't build anything in the init phase make init shouldn't actually build any binary. That was introduced in #4773. * [Part2] Added `ImagePullSecrets` to `SidecarInjectionSpec` (#5002) Automatic merge from submit-queue. [Part2] Added `ImagePullSecrets` to `SidecarInjectionSpec` This PR updated the test cases related to SidecarInjectionSpec. Fixed #4870 /cc @ayj @yusuoh @linsun * Mixer filter fixes (#5012) * Add proxy instances to plugin input params * Fix yet another slice handling issue * Update mixer cluster names * Fix mixer cluster addresses * Fix mixer cluster addresses #2 * Put registry and secrets into a nodeagent package. (#5054) Automatic merge from submit-queue. Put registry and secrets into a nodeagent package. Two sub-package under nodeagent/ - `registry/`, is to handle the workload pod creation, deletion, etc, contains interaction with flexvolumedriver. - `secrets/`, is to handle the secrets management, containing envoy SDS API interaction. * Fix pilot gRPC port in mTLS mode (#4998) * Use 15011 for pilot gRPC port when mTLS enabled. * Update bootstrap test. * Add tls context to bootstrap v1 (ingress using this). * Update bootstrap golden data. * Add a prow test for bookinfo v1alpha3 route rules (#5049) Automatic merge from submit-queue. Add a prow test for bookinfo v1alpha3 route rules * Sets up Pilot and remote cluster for multicluster (#4997) Automatic merge from submit-queue. Sets up Pilot and remote cluster for multicluster This change creates a secret and configmap in order to start pilot in mulitcluster mode. It also creates the necessary resources on the remote cluster to deploy applications there. * Update helm chart version to 0.8.0 (#5030) * simple e2e test: fix error output and exit condition (#5060) * [pilot] Some minor cleanup of bootstrap code (#5071) Automatic merge from submit-queue. [pilot] Some minor cleanup of bootstrap code * Start GRPC server using native go stack, cleanup auth (#4867) * Refactoring the init code for grpc. * Merge cleanup * Add the secure grpc port (also serving https) * Revert cleanup * Refactor and fix AZ retrieval, add test * Change default since the tls-via-envoy uses that, remove verbose log * Allow custom DNS names for pilot * Panic if certs not found an policy requires it * Setting the add to emtpty will disable the mtls listener * Revert vendor again * Fix test failure * Typo * Make sure previous logs are dumped, it seems pilot may crash * Manual fix of fmt -c * Better fix for 'prev log' * Lint errors * routes no longer working * Mount certs to pilot pod * Lint fix and revert pod logs, it's used for something else * Format * Add missing transitive dependency envoyproxy/go-control-plane (#5076) Automatic merge from submit-queue. Add missing transitive dependency envoyproxy/go-control-plane Not entirely sure how these were missed previously. Regardless, running `dep ensure` adds them to vendor (no changes to our lock file since they're transitive). * Remove EUC validation code (#5080) Automatic merge from submit-queue. Remove EUC validation code Issue: #4744 * renamed e2e-bookInfoTests-v1alpha3.sh -> e2e-bookInfoTests-envoyv2-v1alpha3.sh (#5087) Automatic merge from submit-queue. rename e2e-bookInfoTests-v1alpha3.sh -> e2e-bookInfoTests-envoyv2-v1alpha3.sh * Revert "[pilot] Some minor cleanup of bootstrap code (#5071)" (#5090) This reverts commit d5fa2b7. Conflicts: pilot/pkg/bootstrap/server.go * Enable RBAC (#5082) * Use zipkin 2.6.0 in istio. (#4726) * Modernize istio-remote helm chart (#5083) Automatic merge from submit-queue. Modernize istio-remote helm chart Modifies endpoints and services to match master This PR further changes the Makefile target to use istio-${service}.istio-system This PR further adds in the CA service and deploys it by default * Remove gcloud docker calls (#5092) Automatic merge from submit-queue. Remove gcloud docker calls Fixes #4797 * Add per proxy pilot querying (#5096) * Add per proxy pilot querying This is the final chunk of functionality for the base proxy-config command. It adds the ability to query Pilot for a specific proxy as well as the full mesh support. * Linting... * Correct the reference file path in galley (#5000) * Add secret controller for multicluster (#5017) * Vendor changes adding Informers and Listers * Secret Controller code * Linter detected issues * Vendor update related sha change * Adding final bits to the controller * Fixing controller startup code * Adding required RBAC rules to watch for secrets * Refactor Cluster Store initialization place * Fixing Unit test failure * Fixing Unit test failure * Addressing comments part #1 * Fixing Unit test * Switching to different type of Informer * Add create k8s_cr.Cluster object * Fixing if statement * Fixing lint error * Cosmetic changes * Fixing lint error * Tests reproing 503s (was 404s) during routerule apply - with t.Skip() until we have fix (#1041) Reproduces 3 classes of bugs. with t.Skip() for now/until they are fixed. * Multicluster deployment of Apps on remote (#5099) Automatic merge from submit-queue. Multicluster deployment of Apps on remote This change deploys the remote applications in a multicluster test. It alps fixes a couple issues with the remote.yaml and a typo from a prior PR. It makes some fixes to match up with PR5083 * Use iptables TPROXY instead of REDIRECT for inbound traffic (#4654) * Pilot: Support running Envoy with CAP_NET_ADMIN to support TPROXY Add iptables as a dependency to the istio.deb package. Signed-off-by: Romain Lenglet <romain@covalent.io> * Pilot: Support iptables TPROXY instead of REDIRECT for inbound traffic Add iproute2 as dependency to the istio.deb package and the proxy_init Docker image. Add a "-m" command-line flag to istio-ipstables.sh to select the inbound traffic interception mode ("REDIRECT" or "TPROXY"). Fix the usage doc for the other command-line options. Signed-off-by: Romain Lenglet <romain@covalent.io> * Fix deb/* Makefile targets Signed-off-by: Romain Lenglet <romain@covalent.io> * Pilot: Configure transparent proxy redirection from proxy config Configure the redirection mode (redirect vs. tproxy) in ProxyConfig. Pass the redirection mode to Pilot in the Node's metadata, in environment variable ISTIO_META_INTERCEPTION_MODE. Fix Envoy bootstrap to remove the ISTIO_META_ prefix from metadata key names, instead of ISTIO_META. Signed-off-by: Romain Lenglet <romain@covalent.io> * Pilot: Parse Node Metadata and store it in Proxy Signed-off-by: Romain Lenglet <romain@covalent.io> * Update pilot/pkg/kube/inject tests Fix pilot/pkg/kube/inject tests to use the new templates. Add unit tests for the ProxyConfig.InterceptionMode. Signed-off-by: Romain Lenglet <romain@covalent.io> * Pilot: Define sidecar.istio.io/interceptionMode annotation Define sidecar.istio.io/interceptionMode to override the ProxyConfig.InterceptionMode mesh-wide setting. Signed-off-by: Romain Lenglet <romain@covalent.io> * added missing external service definition (#5094) * Fix the node agent E2E test, manually tested. (#5063) Automatic merge from submit-queue. Fix the node agent E2E test, manually tested. Fixed several things to get it work: - Update outdated flags value. - In `start_app.sh`, retain the node agent process log, instead just run it in background. - Make sure the initial root and cert is loaded. How to test this: - `make docker && make docker.push` - `go test -v istio.io/istio/security/tests/integration/nodeAgentTest --tag $(git log -1 --format="%H") --hub gcr.io/<your-project> -kube-config ~/.kube/config --skip_cleanup` We should also figure out how to not run `apt-get` install for every test run. Will follow up with key cert generation issue and then ensure it's stability before re-enabling it. cc @wattli * Bug fix in stackdriver adapter. (#5026) Automatic merge from submit-queue. stackdriver adapter bug fixes and clean up Several bug fixes and cleanups in stackdriver adapter: - add duration type into distribution processing, which is used for latency metrics. - remove a duplicated layer of loop. - remove metric kind override since now adapter could deal with not only custom metrics. - remove a log line which overwhelms mixer logs. * Only query the specific trace using the provided x-client-trace-id tag (#5066) Automatic merge from submit-queue. Only query the specific trace using the provided x-client-trace-id tag This change should make the trace query more efficient as it should only retrieve the single required trace instance. cc @kyessenov @nmittler Signed-off-by: Gary Brown <gary@brownuk.com> * integrate circle ci with testgrid (#4649) Automatic merge from submit-queue. Integrate circle CI with Testgrid The integration will start with mixer and simple e2e tests. Will expand to other jobs in subsequent PRs. Results are now available on https://k8s-testgrid.appspot.com/istio#circleci-e2e-mixer https://k8s-testgrid.appspot.com/istio#circleci-e2e-simple Those are test results from this PR exclusively. After this is merged, the results will be more interesting. The `ci_to_gubernator` binary is built from istio/test-infra#769. Feel free to also take a look. * Fixing postsubmit (#5104) * Implement Citadel prometheus monitoring feature. (#5015) Automatic merge from submit-queue. Implement Citadel prometheus monitoring feature. This monitoring feature exposes a service (port 9093) about Citadel status to prometheus. Yaml files are changed in #5072. Tests will be added shortly. * let stackdriver adapter figure out project metadata by itself (#5057) Automatic merge from submit-queue. let stackdriver adapter figure out project metadata by itself This will make stackdriver adapter config generalized and usable for different projects. * Fix labelNames getting filled in stackdriver logging. (#5058) Automatic merge from submit-queue. Fix labelNames getting filled in stackdriver logging. Added LabelNames in testdata/stackdriver.yaml file Also, fixed that if label was not string, it was getting filled as null. * Don't attempt to qualify the wildcard domain. (#5106) * Don't attempt to qualify the wildcard domain. * fmt * pilot-agent: add a flag to disable internal telemetry (#5101) Automatic merge from submit-queue. pilot-agent: add a flag to disable internal telemetry Enabled by default, but allows to turn off telemetry. Signed-off-by: Kuat Yessenov <kuat@google.com> * Add tcp proxy support for multi-cluster environment (#4694) Automatic merge from submit-queue. tcp proxy support in a multi-cluster environment. Services need to maintain per cluster service IPs. For sidecars running in a cluster, cluster-specific service IPs should be used to build destination_ip_list for envoy listeners. * Add missing namespace to citadel template (#5108) Automatic merge from submit-queue. Add missing namespace to citadel template * prune old version resources that no longer exist (#5107) Automatic merge from submit-queue. prune old version resources that no longer exist * [vendor-change] CloudWatch Mixer adapter (#4617) Automatic merge from submit-queue. [vendor-change] CloudWatch Mixer adapter Adding an adapter to send metrics to cloudwatch * Enable Ingress/Egress gateways in Helm for bookinfo demos (#5120) Automatic merge from submit-queue. Enable Ingress/Egress gateways in Helm for bookinfo demos * Consume labeled multicluster secrets on startup (#5117) Automatic merge from submit-queue. Consume labeled multicluster secrets on startup This patch when run against istio.yaml or istio-auth.yaml runs in the new config mode using only labels rather than configmaps. The configmap functionality can be removed in 0.9. * Add a linter check to make sure types.go are generated. (#5110) Automatic merge from submit-queue. Add a linter check to make sure types.go are generated. addresses #4418 * Remove outdated manifests from install/kubernetes (#4882) * Remove orig_ manifests * Remove istio-mixer-validator and istio-mixer-with-health-check manifests * Remove unwanted manifests before archiving * Remove istio-sidecar-injector.yaml from install/README.md * Remove *one-namespace*.yaml from install/README.md * Make helm-generated manifests overwrite updateVersion_orig.sh manifests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note that v1/ directory and watcher are going away in 0.9 (as soon as the new SDS agent is
ready and we no longer need to watch certs and restart). AZ retrieval will be combined with
getting additional metadata from pilot, and will eventually use the secure GRPC port once v1 is
retired.