Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use same code path for CLI injection and admission controller injection #1748

Closed
klingerf opened this issue Oct 9, 2018 · 7 comments
Closed
Assignees
Labels

Comments

@klingerf
Copy link
Contributor

klingerf commented Oct 9, 2018

Once #1714 merges, it will be possible to install linkerd with an admission controller that automatically injects new deployments. The admission controller uses a YAML template from template.go to produce the injected sidecar configuration, whereas the CLI inject subcommand builds its configuration using Kubernetes objects. The net result is the same, but these two implementations could drift over time. We should standardize injection to use a single code path. cc @ihcsim.

@alenkacz
Copy link
Contributor

@klingerf which of the two options for we want to lean to? Any preferences?

@ihcsim
Copy link
Contributor

ihcsim commented Nov 12, 2018

I think the proxy and proxy-init container specs generated in cli/cmd/inject.go should be used to populate the config map in template.go during install, instead of hard-coding it as seen in template.go.

@klingerf
Copy link
Contributor Author

That's right -- when linkerd is installed with --proxy-auto-inject enabled, the install YAML includes a proxy-injector-sidecar-config ConfigMap. Right now, the contents of that ConfigMap are templated to mirror the output of running linkerd inject on a pod spec, but any changes to inject also need to be reflected in the template. It would be better to produce the template using the inject code itself.

Taking this one step further, however, I think an additional improvement could be to stop storing auto-inject YAML in the ConfigMap altogether, and instead store an object containing inject options, which we could then use to produce the YAML at auto-inject time. That would allow us to further standardize with the standard inject code path, by sharing this struct:

linkerd2/cli/cmd/root.go

Lines 133 to 153 in 38dfc53

type proxyConfigOptions struct {
linkerdVersion string
proxyImage string
initImage string
dockerRegistry string
imagePullPolicy string
inboundPort uint
outboundPort uint
ignoreInboundPorts []uint
ignoreOutboundPorts []uint
proxyUID int64
proxyLogLevel string
proxyBindTimeout string
proxyAPIPort uint
proxyControlPort uint
proxyMetricsPort uint
proxyCpuRequest string
proxyMemoryRequest string
proxyOutboundCapacity map[string]uint
tls string
}

This would be a pretty sizable change, however.

@olix0r
Copy link
Member

olix0r commented Dec 19, 2018

What do folks think about putting the entire inject logic into the public-api service? such that the inject command and the webhook can make an API call that takes yaml/json and receives yaml/json. This is especially useful if cluster-wide policy is set on the controller that needs to influence how injection is done. Relates to #1999

@klingerf
Copy link
Contributor Author

@olix0r 👍 That sounds like a great idea to me. As part of this, I'd like for all of the auto-inject defaults to be passed as flags to the public-api service at install time, so that we don't need to store them in a separate configmap that's read by the proxy-injector service.

It's also the case the the proxy-injector has to supply its modifications in JSON Patch format. So it might make sense for the public-api endpoint to accept a format param, which dictates whether to return the injected object or the patch. Otherwise the proxy-injector will need to compare the injected YAML to the original YAML to create the patch.

@ihcsim
Copy link
Contributor

ihcsim commented Dec 19, 2018

This means the CLI inject command will call the public-api service directly, right? Does the CLI need to authenticate with the service, or is the public-service (as its name implies) public?

@olix0r
Copy link
Member

olix0r commented Dec 19, 2018

@ihcsim i haven't thought deeply about implications around authorization; but in the short-term, this API is read-only and doesn't betray any private information. this may change depending on how TLS evolves.

@alpeb alpeb self-assigned this Feb 18, 2019
alpeb added a commit that referenced this issue Feb 20, 2019
- Created the pkg/inject package to hold the new injection shared lib.
- Moved patch.go there.
- Added the ability to add pod labels and annotations without having to
specify the already existing ones. For that purpose, added `patch.addPodLabelsRoot()` and `addPodAnnotationsRoot()`, and repurposed `addPodLabels()` into `addPodLabel()`, and `addPodAnnotations()` into  `addPodAnnotation()`
- Removed `addDeploymentLabels()` because I couldn't find the need for
it. At least `linkerd inject` wasn't setting those labels. Please
confirm in case I'm wrong.

Still to-do: have `patch_test.go` depend on fixtures separate from the
ones under `controller/proxy-injector`

Ref #1748

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Feb 21, 2019
- Created the pkg/inject package to hold the new injection shared lib.
- Moved patch.go there.
- Added the ability to add pod labels and annotations without having to
specify the already existing ones. For that purpose, added `patch.addPodLabelsRoot()` and `addPodAnnotationsRoot()`, and repurposed `addPodLabels()` into `addPodLabel()`, and `addPodAnnotations()` into  `addPodAnnotation()`
- Removed `addDeploymentLabels()` because I couldn't find the need for
it. At least `linkerd inject` wasn't setting those labels. Please
confirm in case I'm wrong.

Still to-do: have `patch_test.go` depend on fixtures separate from the
ones under `controller/proxy-injector`

Ref #1748

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Feb 21, 2019
- Extracted from `/cli/cmd/inject.go` and `/cli/cmd/inject_util.go`
the core methods doing the workload parsing and injection, and moved them into
`/pkg/inject/inject.go`. The CLI files should now deal only with
strictly CLI concerns, and applying the json patch returned by the new
lib.
- Proceeded analogously with `/cli/cmd/uninject.go` and
`/pkg/inject/uninject.go`.
- The `InjectReport` struct and helping methods were moved into
`/pkg/inject/report.go`

The only network call `/pkg/inject/inject.go` does is inside `shouldInject()` where it
retrieves the namespace annotations from kubernetes. I think we need to
review how that client is created in there.

Caveats:

- Currently passing aroung GlobalConfig and ProxyConfig and using their
GRPC methods directly, until we come up with an abstraction for all that.
- I removed support for parsing yamls with List containing multiple workloads
because it was not an easy refactor given the recursion. I'll re-add
that later on.
- Todo: fix all the tests.
- I've made a note to remove later on the bad v1 import alias that
VSCode is adding.

Ref #1748 and #2289

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Feb 21, 2019
For testing purposes the GlobalConfig and ProxyConfig are currently
hard-coded.

Ref #1748 and #2289

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Feb 22, 2019
- Created the pkg/inject package to hold the new injection shared lib.
- Moved patch.go there.
- Added the ability to add pod labels and annotations without having to
specify the already existing ones. For that purpose, added `patch.addPodLabelsRoot()` and `addPodAnnotationsRoot()`, and repurposed `addPodLabels()` into `addPodLabel()`, and `addPodAnnotations()` into  `addPodAnnotation()`
- Removed `addDeploymentLabels()` because I couldn't find the need for
it. At least `linkerd inject` wasn't setting those labels. Please
confirm in case I'm wrong.

Still to-do: have `patch_test.go` depend on fixtures separate from the
ones under `controller/proxy-injector`

Ref #1748

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Feb 22, 2019
- Extracted from `/cli/cmd/inject.go` and `/cli/cmd/inject_util.go`
the core methods doing the workload parsing and injection, and moved them into
`/pkg/inject/inject.go`. The CLI files should now deal only with
strictly CLI concerns, and applying the json patch returned by the new
lib.
- Proceeded analogously with `/cli/cmd/uninject.go` and
`/pkg/inject/uninject.go`.
- The `InjectReport` struct and helping methods were moved into
`/pkg/inject/report.go`

The only network call `/pkg/inject/inject.go` does is inside `shouldInject()` where it
retrieves the namespace annotations from kubernetes. I think we need to
review how that client is created in there.

Caveats:

- Currently passing aroung GlobalConfig and ProxyConfig and using their
GRPC methods directly, until we come up with an abstraction for all that.
- I removed support for parsing yamls with List containing multiple workloads
because it was not an easy refactor given the recursion. I'll re-add
that later on.
- Todo: fix all the tests.
- I've made a note to remove later on the bad v1 import alias that
VSCode is adding.

Ref #1748 and #2289

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Feb 22, 2019
For testing purposes the GlobalConfig and ProxyConfig are currently
hard-coded.

Ref #1748 and #2289

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Feb 25, 2019
- Created the pkg/inject package to hold the new injection shared lib.
- Moved patch.go there.
- Added the ability to add pod labels and annotations without having to
specify the already existing ones. For that purpose, added `patch.addPodLabelsRoot()` and `addPodAnnotationsRoot()`, and repurposed `addPodLabels()` into `addPodLabel()`, and `addPodAnnotations()` into  `addPodAnnotation()`
- Removed `addDeploymentLabels()` because I couldn't find the need for
it. At least `linkerd inject` wasn't setting those labels. Please
confirm in case I'm wrong.

Still to-do: have `patch_test.go` depend on fixtures separate from the
ones under `controller/proxy-injector`

Ref #1748

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Feb 25, 2019
- Extracted from `/cli/cmd/inject.go` and `/cli/cmd/inject_util.go`
the core methods doing the workload parsing and injection, and moved them into
`/pkg/inject/inject.go`. The CLI files should now deal only with
strictly CLI concerns, and applying the json patch returned by the new
lib.
- Proceeded analogously with `/cli/cmd/uninject.go` and
`/pkg/inject/uninject.go`.
- The `InjectReport` struct and helping methods were moved into
`/pkg/inject/report.go`

The only network call `/pkg/inject/inject.go` does is inside `shouldInject()` where it
retrieves the namespace annotations from kubernetes. I think we need to
review how that client is created in there.

Caveats:

- Currently passing aroung GlobalConfig and ProxyConfig and using their
GRPC methods directly, until we come up with an abstraction for all that.
- I removed support for parsing yamls with List containing multiple workloads
because it was not an easy refactor given the recursion. I'll re-add
that later on.
- Todo: fix all the tests.
- I've made a note to remove later on the bad v1 import alias that
VSCode is adding.

Ref #1748 and #2289

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Feb 25, 2019
For testing purposes the GlobalConfig and ProxyConfig are currently
hard-coded.

Ref #1748 and #2289

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Feb 26, 2019
- Created the pkg/inject package to hold the new injection shared lib.
- Moved patch.go there.
- Added the ability to add pod labels and annotations without having to
specify the already existing ones. For that purpose, added `patch.addPodLabelsRoot()` and `addPodAnnotationsRoot()`, and repurposed `addPodLabels()` into `addPodLabel()`, and `addPodAnnotations()` into  `addPodAnnotation()`
- Removed `addDeploymentLabels()` because I couldn't find the need for
it. At least `linkerd inject` wasn't setting those labels. Please
confirm in case I'm wrong.

Still to-do: have `patch_test.go` depend on fixtures separate from the
ones under `controller/proxy-injector`

Ref #1748

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Feb 26, 2019
- Extracted from `/cli/cmd/inject.go` and `/cli/cmd/inject_util.go`
the core methods doing the workload parsing and injection, and moved them into
`/pkg/inject/inject.go`. The CLI files should now deal only with
strictly CLI concerns, and applying the json patch returned by the new
lib.
- Proceeded analogously with `/cli/cmd/uninject.go` and
`/pkg/inject/uninject.go`.
- The `InjectReport` struct and helping methods were moved into
`/pkg/inject/report.go`

The only network call `/pkg/inject/inject.go` does is inside `shouldInject()` where it
retrieves the namespace annotations from kubernetes. I think we need to
review how that client is created in there.

Caveats:

- Currently passing aroung GlobalConfig and ProxyConfig and using their
GRPC methods directly, until we come up with an abstraction for all that.
- I removed support for parsing yamls with List containing multiple workloads
because it was not an easy refactor given the recursion. I'll re-add
that later on.
- Todo: fix all the tests.
- I've made a note to remove later on the bad v1 import alias that
VSCode is adding.

Ref #1748 and #2289

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Feb 26, 2019
For testing purposes the GlobalConfig and ProxyConfig are currently
hard-coded.

Ref #1748 and #2289

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Feb 28, 2019
- Created the pkg/inject package to hold the new injection shared lib.
- Moved patch.go there.
- Added the ability to add pod labels and annotations without having to
specify the already existing ones. For that purpose, added `patch.addPodLabelsRoot()` and `addPodAnnotationsRoot()`, and repurposed `addPodLabels()` into `addPodLabel()`, and `addPodAnnotations()` into  `addPodAnnotation()`
- Removed `addDeploymentLabels()` because I couldn't find the need for
it. At least `linkerd inject` wasn't setting those labels. Please
confirm in case I'm wrong.

Still to-do: have `patch_test.go` depend on fixtures separate from the
ones under `controller/proxy-injector`

Ref #1748

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Feb 28, 2019
- Extracted from `/cli/cmd/inject.go` and `/cli/cmd/inject_util.go`
the core methods doing the workload parsing and injection, and moved them into
`/pkg/inject/inject.go`. The CLI files should now deal only with
strictly CLI concerns, and applying the json patch returned by the new
lib.
- Proceeded analogously with `/cli/cmd/uninject.go` and
`/pkg/inject/uninject.go`.
- The `InjectReport` struct and helping methods were moved into
`/pkg/inject/report.go`

The only network call `/pkg/inject/inject.go` does is inside `shouldInject()` where it
retrieves the namespace annotations from kubernetes. I think we need to
review how that client is created in there.

Caveats:

- Currently passing aroung GlobalConfig and ProxyConfig and using their
GRPC methods directly, until we come up with an abstraction for all that.
- I removed support for parsing yamls with List containing multiple workloads
because it was not an easy refactor given the recursion. I'll re-add
that later on.
- Todo: fix all the tests.
- I've made a note to remove later on the bad v1 import alias that
VSCode is adding.

Ref #1748 and #2289

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Feb 28, 2019
For testing purposes the GlobalConfig and ProxyConfig are currently
hard-coded.

Ref #1748 and #2289

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Mar 4, 2019
- Created the pkg/inject package to hold the new injection shared lib.
- Moved patch.go there.
- Added the ability to add pod labels and annotations without having to
specify the already existing ones. For that purpose, added `patch.addPodLabelsRoot()` and `addPodAnnotationsRoot()`, and repurposed `addPodLabels()` into `addPodLabel()`, and `addPodAnnotations()` into  `addPodAnnotation()`
- Removed `addDeploymentLabels()` because I couldn't find the need for
it. At least `linkerd inject` wasn't setting those labels. Please
confirm in case I'm wrong.

Still to-do: have `patch_test.go` depend on fixtures separate from the
ones under `controller/proxy-injector`

Ref #1748

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Mar 4, 2019
- Extracted from `/cli/cmd/inject.go` and `/cli/cmd/inject_util.go`
the core methods doing the workload parsing and injection, and moved them into
`/pkg/inject/inject.go`. The CLI files should now deal only with
strictly CLI concerns, and applying the json patch returned by the new
lib.
- Proceeded analogously with `/cli/cmd/uninject.go` and
`/pkg/inject/uninject.go`.
- The `InjectReport` struct and helping methods were moved into
`/pkg/inject/report.go`

The only network call `/pkg/inject/inject.go` does is inside `shouldInject()` where it
retrieves the namespace annotations from kubernetes. I think we need to
review how that client is created in there.

Caveats:

- Currently passing aroung GlobalConfig and ProxyConfig and using their
GRPC methods directly, until we come up with an abstraction for all that.
- I removed support for parsing yamls with List containing multiple workloads
because it was not an easy refactor given the recursion. I'll re-add
that later on.
- Todo: fix all the tests.
- I've made a note to remove later on the bad v1 import alias that
VSCode is adding.

Ref #1748 and #2289

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Mar 4, 2019
For testing purposes the GlobalConfig and ProxyConfig are currently
hard-coded.

Ref #1748 and #2289

Signed-off-by: Alejandro Pedraza <[email protected]>
@alpeb alpeb closed this as completed in ddf2e72 Mar 5, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants