-
Notifications
You must be signed in to change notification settings - Fork 91
Conversation
0b6dd68
to
5821103
Compare
Codecov Report
@@ Coverage Diff @@
## master #1041 +/- ##
==========================================
+ Coverage 69.65% 70.91% +1.26%
==========================================
Files 47 18 -29
Lines 5529 2228 -3301
==========================================
- Hits 3851 1580 -2271
+ Misses 1481 571 -910
+ Partials 197 77 -120 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me. I left bunch of comments, mostly minor and to help with understanding the code.
@@ -0,0 +1,3 @@ | |||
FROM scratch | |||
ADD sidecar-initializer /usr/local/bin/ | |||
ENTRYPOINT ["/usr/local/bin/sidecar-initializer"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want some TLS certs here (assuming we need to talk to something other than kube API)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current version only ever interacts with the k8s apiserver via patch/list/watch. It uses the in-cluster config similar to pilot discovery service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then it's fine. Running out of cluster would not use docker anyways.
cmd/sidecar-initializer/main.go
Outdated
|
||
root.PersistentFlags().StringVar(&flags.kubeconfig, "kubeconfig", "", | ||
"Use a Kubernetes configuration file instead of in-cluster configuration") | ||
root.PersistentFlags().StringVar(&flags.meshconfig, "meshConfig", "/etc/istio/config/mesh", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I call it "meshconfig" in other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discovery and Istioctl use "meshConfig". The agent uses "meshconfig". I'm happy to make this consistent with either - let me know :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's rename both to meshconfig to be consistent with kubeconfig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to meshconfig
for the initializer command. I can change the others in a follow-up PR.
cmd/sidecar-initializer/main.go
Outdated
} | ||
|
||
root.PersistentFlags().StringVar(&flags.hub, "hub", "docker.io/istio", "Docker hub") | ||
root.PersistentFlags().StringVar(&flags.tag, "tag", "0.1", "Docker tag") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make 0.2 default since this is unlikely to work with 0.1.
cmd/sidecar-initializer/main.go
Outdated
"Use a Kubernetes configuration file instead of in-cluster configuration") | ||
root.PersistentFlags().StringVar(&flags.meshconfig, "meshConfig", "/etc/istio/config/mesh", | ||
fmt.Sprintf("File name for Istio mesh configuration")) | ||
root.PersistentFlags().DurationVar(&flags.resyncPeriod, "resync", time.Duration(0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is 0 resync reliable enough? maybe make it a few minutes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. I'll increase to 30 seconds just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for cluster-wide, probably 5 minutes is enough time not to overwhelm the config server.
cmd/sidecar-initializer/main.go
Outdated
root.PersistentFlags().StringVar(&flags.hub, "hub", "docker.io/istio", "Docker hub") | ||
root.PersistentFlags().StringVar(&flags.tag, "tag", "0.1", "Docker tag") | ||
root.PersistentFlags().StringVar(&flags.namespace, "namespace", | ||
v1.NamespaceDefault, "Namespace managed by initializer") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should "all" be default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went back and forth on the default here ."all" is a reasonable default for cluster-wide installations of Istio. It causes problems for namespace isolated installs. For internal testing we override it anyway so maybe its reasonable for cluster-wide installs to leave this as "all" to avoid broken deployments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use "all". "default" is not really default in the k8s context.
if pending := objectMeta.GetInitializers().Pending; len(pending) == 1 { | ||
objectMeta.Initializers = nil | ||
} else { | ||
objectMeta.Initializers.Pending = append(pending[:0], pending[1:]...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is pending[:0]
just a nil? some other trick?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was carried over from Kelsey's initializer tutorial. It's a type'd nil
which avoids reallocating the underlying array (see https://github.com/golang/go/wiki/SliceTricks#filtering-without-allocating)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool
platform/kube/inject/initializer.go
Outdated
} | ||
out := o.(*v1beta1.ReplicaSet) | ||
|
||
if err = i.modifyResource(&out.ObjectMeta, &out.Spec.Template.ObjectMeta, &out.Spec.Template.Spec); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could try code generation to get around this repetitive code problem (like what I had to do for CRDs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I tried a few of different ways to reduce duplication, but wasn't super happy with any of the results. I gave up and focused on getting everything integrated and tested.
platform/kube/inject/initializer.go
Outdated
return err | ||
} | ||
_, err = i.clientset.AppsV1beta1().Deployments(in.Namespace). | ||
Patch(in.Name, types.StrategicMergePatchType, patchBytes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain the choice of Patch vs Put? Is it to avoid serializing concurrent changes to the config? Is the patch library smart enough to do the right thing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initializers should already be serialized (via ordered pending list). Patch()
was carried over from Kelsey's initializer tutorial. He switched from PUT to PATCH here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool. I trust that Kelsey knows what's he doing with PATCH.
platform/kube/inject/inject.go
Outdated
} | ||
} | ||
|
||
if !required { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be moved up
|
||
// TODO - add version check for sidecar upgrade | ||
|
||
return !ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the idea is that if the status annotation is present then there should be no injection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
/test pilot-e2e-smoketest |
/test pilot-e2e-smoketest |
/test all |
… into sidecar-initializer-for-review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. few nits..
@@ -0,0 +1,24 @@ | |||
load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing copyright?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't been adding copyright to the BUILD files.
$ find -name BUILD | xargs grep Copyright | wc -l
0
cmd/sidecar-initializer/main.go
Outdated
switch inject.InjectionPolicy(flags.policy) { | ||
case inject.InjectionPolicyOff, inject.InjectionPolicyOptIn, inject.InjectionPolicyOptOut: | ||
default: | ||
return fmt.Errorf("unknown namespace injection policy: %v", flags.policy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. unknown injection policy. (omit the namespace)
meshconfig string | ||
hub string | ||
tag string | ||
namespace string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this enable initializer for a given namespace? If so, won't we need one initializer per namespace? Is there a possibility to watch some config map or something to pick up namespaces enabled for Istio?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initializer is enabled with the InitializerConfiguration resource. InitializerConfiguration is cluster scoped and there is no way through that to restrict which namespace(s) require initialization - its all or nothing. Some proposals to fix this problem are documented here but nothing is designed/implemented.
This namespace
flag determines which namespace the initializer deployment manages. This is useful (required?) in our shared per-namespace e2e tests. In this case we would need one initializer deployment per namespace. Once cluster-wide Istio is the standard this can default to v1.NameSpaceAll
and the single initializer deployment would be responsible for the entire cluster.
|
||
var ignoredNamespaces = []string{ | ||
"kube-system", | ||
"kube-public", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably istio-system as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. We may need to remove for istio-on-istio depending on how the proxy is deployed.
|
||
// InitializerOptions stores the configurable options for an initializer | ||
type InitializerOptions struct { | ||
// TODO - pull from ConfigMap? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #1062 to track this issue.
platform/kube/inject/initializer.go
Outdated
} | ||
|
||
for _, kind := range kinds { | ||
kind := kind // capture the current value of `kind` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this doing? is there a cleaner way to write this line?
func (i *Initializer) hasIstioInitializerNext(object metav1.Object) bool { | ||
glog.V(2).Infof("ObjectMeta initializer info %v/%v policy:%q status:%q %v", | ||
object.GetNamespace(), object.GetName(), | ||
object.GetAnnotations()[istioSidecarAnnotationPolicyKey], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will i be able to skip sidcear using annotations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: it would be great to customize the sidecar tag (or full path) using annotations. For example to pick a debug image of sidecar for a job that needs debugging (ideally the full path of the sidecar should be specified ). Or a job may use a specialized sidecar. Or many other use cases...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, policy.sidecar.istio.io: "policy.sidecar.istio.io/force-off"
annotation can be used for skip sidecar injection. See here for an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if resourcePolicy != istioSidecarAnnotationPolicyValueForceOff { | ||
required = true | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
cmd/sidecar-initializer/main.go
Outdated
resyncPeriod time.Duration | ||
}{} | ||
|
||
root := &cobra.Command{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/root/rootCmd
cmd/sidecar-initializer/main.go
Outdated
return root | ||
} | ||
|
||
func main() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just nit, is it possible to update this file a bit to keep consistent with other files as following format?
var(
variables here
)
func init() {
cmd.PersistentFlags init
}
func main() {
if err := rootCmd.Execute(); err != nil {
glog.Error(err)
os.Exit(-1)
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer this approach which limits the use of global variables.
// Namespace is the Kubernetes namespace the initializer is | ||
// responsible for managing. The initializer will manage injection | ||
// for all namespaces if the value of v1.NamespaceAll is | ||
// specified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be moved to above line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the entire namespace comment should be moved above the struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just mean the word of specified
can be moved to L65
platform/kube/inject/initializer.go
Outdated
} | ||
|
||
for _, kind := range kinds { | ||
kind := kind // capture the current value of `kind` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shadow will make people confused, +1 to rewrite.
containers: | ||
- name: sidecar-initializer | ||
image: {{.Hub}}/sidecar_initializer:{{.Tag}} | ||
imagePullPolicy: Always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer that we use ifNotPresent
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. I think we fixed this in non initializer case?
- statefulsets | ||
- jobs | ||
- daemonsets | ||
# - replicasets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kill those two lines if not needed
test/integration/infra.go
Outdated
// clean-up. | ||
if err := deploy("initializer-config.yaml.tmpl"); err != nil { | ||
// only log if error is AlreadyExists | ||
glog.Infof("sidecar.initializer.istio.io already exists: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to mention already exists
here? I think the err
should already include this?
namespace: kube-system | ||
spec: | ||
replicas: 7 | ||
strategy: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why include some empty values here? Can we remove this? Ditto for others.
I also noticed that some old files also include such kind of empty values, is it really useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's go JSON thingy that generates these empty ones
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove those empty line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we cannot remove these lines. testdata/*.injected
files are direct output of yaml.Marshal
which outputs default values for all fields not marked with omitempty
JSON annotation e,g. strategy
and resource
. Removing them from testdata/*.injected
would require doing the same for the real injected resources and either require a custom YAML marshaller, updated the k8s type definitions, or using regex to strip out known empty fields. Including the default values here doesn't seem to cause any problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks @ayj
{ | ||
"jobs", | ||
i.clientset.BatchV1().RESTClient(), | ||
&batchv1.Job{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about CronJob
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This list of supported kinds mirrors what kube-inject currently supports. Ideally we could generalize to all pod types (see line 149), but that wasn't working yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, but shall we file an issue to trace CronJob
for now?
|
||
// Tag is the docker image tag used for the injected sidecar proxy | ||
// and init images. | ||
Tag string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to also customize the image names ( or use a full string, as used in docker ). Istio design allows for
multiple implementations of sidecars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. We went back and forth on these options with kube-inject and settled on hub/tag instead since users had to type it at the command line everyone. This is no longer an issue with initializer but I kept hub/tag for consistency with existing kube-inject. We could fix this as part of #1062.
- containerPort: 80 | ||
name: http | ||
resources: {} | ||
- args: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feature request: allow user to provide a template config - they may want other -v, cpu/mem requirements, etc.
(same for initializer - but probably just the args need to be customized).
If resource constraints are enabled, it's tricky to schedule jobs if you can't control the injected containers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1062 tracks this issue.
This PR adds the Kubernetes initializer for transparent proxy injection. The initializer configuration is currently disabled to avoid breaking other test instances without the corresponding initializer deployment. This feature will be incrementally enabled after the initial set of changes are merged and all test instances have a default initializer running with policy=OFF.
Fixes #335