Conversation
- remove the SDS startup in pilot - the feedback is to start with SDS in agent, pilot will only sign certs. - cleanup a bit the startup - use port 8080 for http - istioctl expects it. - add the original Dockerfile - while it is getting integrated into the istio build system ( can be useful after as well, it's very easy)
|
Note - this is built on top of #18132, waiting for that to merge - the last commits are relevant to CA. |
|
/test integ-galley-k8s-tests_istio |
|
/test e2e-simpleTests_istio |
|
/test integ-security-k8s-tests_istio |
|
/test pilot-multicluster-e2e_istio |
|
/test unit-tests_istio |
|
/test integ-security-k8s-tests_istio |
|
|
||
| // Location of K8S CA root. | ||
| k8sCAPath = "./var/run/secrets/kubernetes.io/serviceaccount/ca.crt" | ||
| ) |
There was a problem hiding this comment.
Jimmy, I think we should reuse 15020 if it's already used?
|
The PR description says the PR is only tested manually. For each flow related to this PR, can you provide detailed intructions on how to test this PR manually? |
| conIDresourceNamePrefix := sdsLogPrefix(conID, resourceName) | ||
| if !s.skipToken { | ||
| if s.localJWT { | ||
| // Running in-process, no need to pass the token from envoy to agent as in-context - use the file |
There was a problem hiding this comment.
I am uncertain why the localJWT option is needed. Without the localJWT option, Envoy reads the token from a file and passes it to istio agent in the context. Withou localJWT option, istio agent reads the token from a file.
If the localJWT option is introduced to improve the performance, as the token still needs to be read, there is no performance gain.
There was a problem hiding this comment.
This is to simplify the code - there is no need for Pilot to generate a complicated config, with the file path - in each cluster/listener - when the agent has access to the file as well.
The option is only used short term, until node-agent is still in use - once it's gone there will be no passing of tokens from envoy to SDS.
There was a problem hiding this comment.
As a fun fact - if the file is not found, envoy is crashing hard.
Not passing the file from envoy is not only simpler - it also allows us the option to use Envoy GRPC library for envoy to local SDS communication, instead of using 2 grpc stacks at the same time.
There was a problem hiding this comment.
Be a good citizen -- file an issue about the crash?
There was a problem hiding this comment.
I think John did already, he found it while testing istiod.
There was a problem hiding this comment.
Can you provide more details about "using 2 grpc stacks at the same time"?
costinm
left a comment
There was a problem hiding this comment.
Re. testing: this depends on a separate PR in installer, with a new injection template and istiod updates.
The important part - that the new SDS doesn't affect existing workloads - is covered by the existing test suites.
|
/test integ-security-k8s-tests_istio |
|
/retest |
|
@costinm: The following test failed, say
DetailsInstructions 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. |
This reverts commit b9defec.
* Incremental changes: - remove the SDS startup in pilot - the feedback is to start with SDS in agent, pilot will only sign certs. - cleanup a bit the startup - use port 8080 for http - istioctl expects it. - add the original Dockerfile - while it is getting integrated into the istio build system ( can be useful after as well, it's very easy) * Add a cloudbuilder for istiod, add missing files * More cleanup, remove controversial dockerfile * Use upstream galley server, reuse kube * Make format * Fix lint * Add missing trust domain * Post merge fixes * Lint * Fix nil check for interface * Lint * Initial SDS agent and support * Improved startup config, use injection * Tested SDS with Istiod and google plugin * Format and lint * More lint fighting, cleanup * Change the template to use K8S-signed cert with local SDS * More fixes, better debug * Lint and fixes * More lint * Simpler/safer code. Istiod will not support citadel agent * Restore previous code to disable sds * Another attempt to format code and fight linter * Indent and fix sds default * Fix another crazy startup issue. The watcher is not only watching - but also kicks the first start. This code needs to be mostly replaced once SDS and istiod are enabled. Also add a simple way to patch galley options for fine tunning. Like values.yaml subset needed by injection, it's part of the transition to mesh.yaml * Add explicit option to use local JWT * format * Review feedback, renames and more comments * Test failure * Signature change in security * Fixes for crash and canary mode * Remove dep on bootstrap
Initial merging of (minimal) CA functionality from Citadel into istiod - just the GRPC cert signing and self-signed cert.
Initial merging of SDS grpc into pilot-agent - just minimal stuff to call istiod or google ca to get certs.
Tested manually - still working on running the e2e tests against this.