Add build rules and instructions for enabling Istio sidecar injection#278
Add build rules and instructions for enabling Istio sidecar injection#278ian-mi merged 14 commits intoknative:masterfrom
Conversation
Sidecar injection should be enabled for ela pods and disabled for autoscaler pods.
There was a problem hiding this comment.
I think we should make this as part of "everything" and not a separate step. Otherwise, once we rip out nginx, without sidecar injection, traffic will not be properly handled.
There was a problem hiding this comment.
The issue is that this should only happen once for the lifetime of the cluster and not during every everything.apply for example. One solution might be to create a new build rule for initial setup only where we can add this. WDYT?
There was a problem hiding this comment.
I like that idea. We will need other things of this nature in the future and we can add all one-time cluster initialization in there.
There was a problem hiding this comment.
This will enable injection for all pods in the default namespace, including the autoscale pod. Is there a way to enable injection only for ela generated pods?
There was a problem hiding this comment.
Ah never mind. Looking below, seems like you are explicitly disabling injection for autoscale.
There was a problem hiding this comment.
This will enable injection for builds too. Can we opt pods into this instead?
There was a problem hiding this comment.
I haven't found any documentation about opt-in behavior. The istio-inject configmap has a policy field which might do what we want but perhaps builds should be explicitly opted-out or run in a separate namespace. WDYT?
There was a problem hiding this comment.
Because we already have some code that injects side cars, what's the benefit for using istio here, instead of injecting the istio sidecar in the controller? Seems odd that we have one way of injecting some sidecars than others.
There was a problem hiding this comment.
I think the benefit would be that the istio-proxy sidecar spec can be owned by istio and be mostly transparent to us. Right now our build rules tightly couple us to an istio version but ideally elafros could be installed on any cluster with istio + automatic sidecar injection and use the same istio-proxy sidecar that is injected for other services within the cluster.
There was a problem hiding this comment.
My one concern is that I can see there being different capabilities that get lit up based on the type of revision. This seems like something that could be configurable. Right now configuration/revision is something that is a self standing unit and can be used as a building block and while I think it's indeed nice to rely on Istio for this injection I can also see that this might be tuneable for different kinds of workloads. Currently it's very hardcoded what k8s resources revision creates, but this should change. Thoughts?
There was a problem hiding this comment.
I think that ideally we would push for the capabilities we need to be supported by istio and configurable through CRDs such as RouteRules or DestinationRules rather than by modifying the sidecar spec. Note that we're also not locked into this decision as we can disable automatic injection on a per-revision basis if needed if, for example, we want to transition to something like istio/istio#3816.
mattmoor
left a comment
mattmoor
left a comment
There was a problem hiding this comment.
A few comments inline.
I'm very impressed with your Bazel-fu! :)
There was a problem hiding this comment.
This should have -c _CLUSTER_NAME where the cluster name is passed through from the workspace.
There was a problem hiding this comment.
I've copied the command used to extract the user override for the cluster role binding. It might be nice to perform extraction of these variables in another repository rule at some point.
There was a problem hiding this comment.
Can we name this cluster_ca_bundle for more clarity?
There was a problem hiding this comment.
You can drop the kind, this is becoming vestigial
There was a problem hiding this comment.
This will enable injection for builds too. Can we opt pods into this instead?
There was a problem hiding this comment.
This assumes the current kubectl context is where you want it vs. an explicit cluster argument. You should at least make the docs note this (or how to check that their cluster variable $K8S_CLUSTER_OVERRIDE matches the current context kubectl config current-context)
There was a problem hiding this comment.
Looking inside of this script, this looks like it would be non-trivial to incorporate into our k8s_object rules, but I think it's doable through some custom logic (and I'd like to keep our setup to :everything.apply). Can you open an issue and assign it to me?
There was a problem hiding this comment.
FYI, here's an issue describing kind of what I had in mind: bazelbuild/rules_k8s#88
google-prow-robot
added
the
size/L
There was a problem hiding this comment.
Sorry I missed this before, I'd meant: "Can we change the name or the rule to cluster_ca_bundle?"
There was a problem hiding this comment.
Changed the rule as well.
There was a problem hiding this comment.
Oh, I see what you meant earlier. I didn't realize _CLUSTER was insufficient. :(
There was a problem hiding this comment.
nit: -n vs. --cluster
I'd use -c or --namespace :)
There was a problem hiding this comment.
I ended up using --cluster since kubectl doesn't appear to have a short version for this. I've switched to --namespace to match.
There was a problem hiding this comment.
I think my only remaining major problem is that this will break Build going in as-is since those Pods are not annotated: knative/build#73
I think we should add the opt-out for Build pods, but is there a more surgical way we should recommend that folks opt into this until we can get an updated Build into Elafros?
There was a problem hiding this comment.
One option would be to include an istio-inject configmap with policy: disabled. I've confirmed that the sidecar.istio.io/inject annotation will override this policy effectively making all namespaces opt-in vs. opt-out. As long as we always set this annotation elafros should work with the inject-policy enabled or disabled.
There was a problem hiding this comment.
I've gone ahead an implemented this using an expand_template rule. It is kind of a hack but allows us to use the upstream template.
There was a problem hiding this comment.
@mattmoor This is ready for another look.
There was a problem hiding this comment.
Thanks, if you want to TAL at: knative/build#74 I can try to merge Build into the Elafros repo tomorrow.
This will allow istio sidecars to be injected for elafros pods without changing the default behavior of the rest of the default namespace.
…#278) * Upgrade istio to v0.6.0 * Add build rules for setting up istio's automatic sidecar injector. * Remove fluentd sidecar. * Set annotations to explicitly enable/disable sidecar injection. Sidecar injection should be enabled for ela pods and disabled for autoscaler pods. * Document steps for enabling Istio sidecar injection. * Respect the cluster override when fetching CA bundle. * Rename ca_bundle rule to cluster_ca_bundle. * Remove unnecessary kind from istio-sidecar-injector-configmap. * Use --namespace instead of -n in ca_bundle.bzl * Change istio build target to disable istio sidecar injection. This will allow istio sidecars to be injected for elafros pods without changing the default behavior of the rest of the default namespace. * Rename ca_bundle rule to cluster_ca_bundle.
5 participants
This requires removing the fluentd sidecar which currently requires connectivity with services outside of the mesh. mdemirhan is looking at adding different fluentd sidecar to replace this.
Please also note that a few extra setup steps are required to enable automatic sidecar injection.