Update istio-remote helm chart to support automatic sidecar injection#6366
Update istio-remote helm chart to support automatic sidecar injection#6366istio-testing merged 4 commits intoistio:masterfrom
Conversation
|
@tiswanso I migrated our internal Helm chart into the istio-remote directory. This is pretty much exactly what I deployed and the sidecar injector just worked. As I mentioned in the working group meeting I was unable to get a full end to end test due to #6313 but that's unrelated to multi-cluster. We're going to create a couple clusters without custom domains and I'll keep working at testing this. I removed the endpoint and service templates in favor of using the remote endpoints directly. I could see setting up |
Codecov Report
@@ Coverage Diff @@
## master #6366 +/- ##
=======================================
+ Coverage 71% 71% +1%
=======================================
Files 354 354
Lines 30666 30740 +74
=======================================
+ Hits 21503 21619 +116
+ Misses 8314 8274 -40
+ Partials 849 847 -2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Does the two cluster can communicate with each other if using different root cert?
There was a problem hiding this comment.
@gyliu513 this looks like a copy and paste from the main helm chart. I think that is fine for now.
There was a problem hiding this comment.
yeah, what Steve said. The security and sidecar injector charts are exact copies of the subcharts in install/kubernetes/helm/istio
There was a problem hiding this comment.
But my question is does mTLS works cross clusters for such case?
There was a problem hiding this comment.
@gyliu513 -- AFAIK, yes, as long as each cluster's root cert was signed by the same CA.
There was a problem hiding this comment.
@gyliu513 I prefer not to carry deltas in the istio-remote chart. The update model should be a simple copy and paste. that said, if you think security belongs as a keyword, might submit that to the main istio helm chart.
There was a problem hiding this comment.
It is a straight copy/paste. I'd personally prefer to use helm dependency but that's not a pattern that is currently in use for the project. I could change it to symlinks if we think t's a better approach to avoid drift but I generally don't like them in git repos.
There was a problem hiding this comment.
@sdake @gyliu513 --what do you guys think? IMO, relative path symlinks are ok and easier to review--explicitly ensuring "there's no change here".
EDIT: I'd prefer helm dependency as well but @sdake mentioned some practical problems. Not sure if we could/should focus on tackling those problems instead of borking up the organization of the charts, though.
There was a problem hiding this comment.
I was a bit confused, seems here the rbac logic in your PR only handling the serviceAccount name?
Same issue for the local chart. @ymesika as you initiated the original PR for local chart, any comments for this?
There was a problem hiding this comment.
I don't think its helpful to fix bugs in the main chart by reviewing istio-remote :) It may make more sense to just fix those problems independently and use a copy and paste as was done here (and was done in the past for citadel).
There was a problem hiding this comment.
Do we need to add permission for gateway, destinationrules, virtualservices?
There was a problem hiding this comment.
No because those resources will only exist in the primary control plane cluster.
There was a problem hiding this comment.
yes, we have only one control panel.
There was a problem hiding this comment.
This is from the main chart as well. If the namespace is required it should be in{{ .Values.global.proxy.envoyStatsd.host }}
There was a problem hiding this comment.
Please add namespace back for this, as master already fixed this.
There was a problem hiding this comment.
If removing this, then how to let the local istio connect to remote istio?
BTW: I also created a PR to add more checking for this at #6393
There was a problem hiding this comment.
agreed, I don't understand how this works properly. @Rigdon could you expand on how one deploys Istio in this setup?
There was a problem hiding this comment.
The proxy containers in the remote cluster will use the remote endpoints directly as this is written. We could add some ExternalName services so that they are referencing local service names but I couldn't think of any immediate value in that.
There was a problem hiding this comment.
I can go ahead and update the README with details if we think this is generally the right direction.
There was a problem hiding this comment.
The README.md should not exist - it was removed once and replaced with a link to istio.io. No sense updating README.md, although istio.io will need any updates that are relevant to changes made in the charts.
There was a problem hiding this comment.
@sdake @Rigdon @gyliu513 -- does removing this mandate using auto-sidecar injection? (or pretty much mandate it?)
With the selectorless service + endpoint setup on the remote, we're able to make the manual injection workflow use the exact same kube-inject output for apps as if they were deployed on the cluster with the Istio control-plane. IMO, this is fairly nice.
However, I think a similar workflow could still work with this change if the istioctl kube-inject is run against a remote cluster with the --meshConfigMapName (or maybe --injectConfigMapName) option pointing to the istio configmap in this change. I hope to get time to play with that scenario.
There was a problem hiding this comment.
Yeah I guess I was hoping (I haven't been using manual injection at all) that istioctl kube-inject would honor the meshConfig in the target cluster. If that's not the case then adding the ExternalName type service may be a better option.
There was a problem hiding this comment.
@Rigdon @tiswanso I think the outstanding question that needs answered prior to merging this PR is whether or not manual injection works with it. All of the e2e tests for multicluster depend on manual injection, and while i'm not opposed to changing those to automatic injection, we don't want to break the test cases with this merge.
Further we do need manual injection to work - atleast for 1.0 and until automatic injection has less tread on the tires.
Cheers
-steve
There was a problem hiding this comment.
namespace objects do not work properly in helm. Please remove this section.
There was a problem hiding this comment.
values.yaml is a special file in that it exports an API to the user. As such, it should have as minimum a footprint as is necessary to get the job done. This has a bunch of unrelated stuff in it - would need to be trimmed prior to merge.
There was a problem hiding this comment.
I'll go through and review the variables again. I removed all of the sections related to Istio components that won't be installed on the remote clusters.
There was a problem hiding this comment.
Kubernetes applications won't work in this model unless there is some magic I don't understand. Currently we specify the pilotendpoint so that the sidecars inside kubernetes can find pilot and receive the mesh calculation.
Could you add more detail as to how the deployment works in this situation?
There was a problem hiding this comment.
I use remotePilotAddress as the discoveryAddress here in the mesh config (along with the other remoteX addresses. The proxy sidecars will then use those adresses instead of local default service names.
There was a problem hiding this comment.
Sounds like an improvement. Mind I test prior to merge? cc @baodongli @tiswanso
There was a problem hiding this comment.
@Rigdon are you using proyv1, or v2? v1 is deprecated, and IIUC will disappear in the 1.0 release. We should be marching towards that model if possible.
There was a problem hiding this comment.
The default in both charts now is the proxyv2 image, though as @tiswanso pointed out the sidecars are still configured by default to use the old http ports rather than the newer grpc ports. I tried to keep as much the same as possible between the two charts.
There was a problem hiding this comment.
@sdake -- I think we want to fix this in the main charts ASAP. From your slack msg, I'm guessing you're looking at it. Do you know if we have an issue already for tracking the latest xDS config stuff the helm charts?
@baodongli ^^
|
@sdake I removed the namespace and any variables in the values.yaml that aren't used by the security and injector subcharts |
There was a problem hiding this comment.
This service account isn't created in any chart that I can see. Did you add a new file to create this service account but it didn't get added to changeset?
I see now: istio-remote/templates/serviceaccount.yaml has the wrong service-account name
There was a problem hiding this comment.
you want istio-control-plane-service-account instead of istio-multi
There was a problem hiding this comment.
I changed it the other way. Kept the istio-multi SA name to match the docs you'd written and fixed the role binding instead.
There was a problem hiding this comment.
We need to fix this section for envoyv1 xDS v. envoyv2 xDS (grpc). ports 15005 and 15007 are the envoy v1 (legacy) pilot endpoints. The envoy v2 (grpc) endpoints are 15010 and 15011.
There was a problem hiding this comment.
yeah... I noticed that. Didn't want to deviate from the main chart though.
There was a problem hiding this comment.
Cool... good point. Not sure if there's an issue already for this.
There was a problem hiding this comment.
But my question is does mTLS works cross clusters for such case?
There was a problem hiding this comment.
yes, we have only one control panel.
There was a problem hiding this comment.
Should not here be remote statsd?
There was a problem hiding this comment.
the existing statsd configuration in values.yaml is adequate. the statsd collector could be local or remote and either is supported by the current configuration options.
There was a problem hiding this comment.
So how does the metrics be send to local control panel prometheus if using local statsd?
|
@Rigdon -- hey, would you mind if we move the service-account stuff out into another PR to maybe get it in faster? I might end up needing it to cleanup the multicluster e2e tests. |
1027c89 to
ba3be8c
Compare
There was a problem hiding this comment.
the existing statsd configuration in values.yaml is adequate. the statsd collector could be local or remote and either is supported by the current configuration options.
|
I updated this with all of the additional feedback. The big things being the switch to symlinks for the security and injector charts and I updated the readme to point at the preliminary docs page. I'll also pull the service account parts into a separate PR so that we can get them merged for Tim. I'll leave them here for now too in case this gets merged quickly and fix the conflict Monday if not.
|
5661348 to
cd72a31
Compare
|
The commands that appear to be failing in both the lint and e2e-galley tests are working fine in Helm version 2.8.2. e2e-galleyLintIt looks like an issue with the symlinks. |
|
As I mentioned above it works with the version of Helm I'm using. I expect
that it's that the istio project is using an older version of Helm or the
symlinks aren't being cloned correctly. I agree with @sdake that we should
move on without the symlinks for now. Hopefully we can just use helm
dependency in the near future.
…On Mon, Jun 25, 2018, 7:54 PM Guang Ya Liu ***@***.***> wrote:
@Rigdon <https://github.com/Rigdon> it is weird that the symlinks failed,
if this is the case, I think we may need to open an issue in helm community
and have some discussion there?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6366 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABcyuNRzA43jcTb2F0iBmqVXKIxj9CGOks5uAXg5gaJpZM4UsYlZ>
.
|
|
@Rigdon can you rebase so that I can do some test for this PR? |
ec2435f to
e1ba0ca
Compare
gyliu513
left a comment
There was a problem hiding this comment.
I want to test it locally, but after the istio.yaml file is generated by make generate_yaml, the deployments cannot get images to deploy.
This seems because the CI was not passed, so the image was not created.
There was a problem hiding this comment.
missing {{ end }} for {{- if $.Values.cleanUpOldCA }}
4feddc6 to
7a2a0a2
Compare
|
/retest |
|
/lgtm Nice work Ryan! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gyliu513, Rigdon, sdake The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WIP, this needs further testing.
This removes the local Istio service endpoints in favor of addressing remote endpoints directly and provides a basic setup for the Citadel and SIdecar Injectors. I will update the README with instructions if we agree on this approach and it works for others.