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

Add ability to re-inject resources that were already injected #1970

Closed
kamushadenes opened this issue Dec 10, 2018 · 9 comments · Fixed by #2089
Closed

Add ability to re-inject resources that were already injected #1970

kamushadenes opened this issue Dec 10, 2018 · 9 comments · Fixed by #2089
Assignees

Comments

@kamushadenes
Copy link

kamushadenes commented Dec 10, 2018

Bug Report

What is the issue?

linkerd inject is not upgrading the proxy sidecars from 2.0.0 to 2.1.0

How can it be reproduced?

Logs, error output, etc

...

hostNetwork: pods do not use host networking...............................[ok]
sidecar: pods do not have a proxy or initContainer already injected........[warn] -- known sidecar detected in deployment/some-service                                                                                                 
supported: at least one resource injected..................................[warn] -- no supported objects found
udp: pod specs do not include UDP ports....................................[ok]

Summary: 0 of 1 YAML document(s) injected

...

All containers stay at 2.0.0:

...

      linkerd.io/proxy-version: stable-2.0.0
      linkerd.io/proxy-version: stable-2.0.0
      linkerd.io/proxy-version: stable-2.0.0
      linkerd.io/proxy-version: stable-2.0.0
      linkerd.io/proxy-version: stable-2.0.0
      linkerd.io/proxy-version: stable-2.0.0
      linkerd.io/proxy-version: stable-2.0.0
      linkerd.io/proxy-version: stable-2.0.0
      linkerd.io/proxy-version: stable-2.0.0
      linkerd.io/proxy-version: stable-2.0.0
      linkerd.io/proxy-version: stable-2.0.0
      linkerd.io/proxy-version: stable-2.0.0
      linkerd.io/proxy-version: stable-2.0.0
      linkerd.io/proxy-version: stable-2.0.0

...

linkerd check output

kubernetes-api: can initialize the client..................................[ok]
kubernetes-api: can query the Kubernetes API...............................[ok]
kubernetes-api: is running the minimum Kubernetes API version..............[ok]
linkerd-api: control plane namespace exists................................[ok]
linkerd-api: control plane pods are ready..................................[ok]
linkerd-api: can initialize the client.....................................[ok]
linkerd-api: can query the control plane API...............................[ok]
linkerd-api[kubernetes]: control plane can talk to Kubernetes..............[ok]
linkerd-api[prometheus]: control plane can talk to Prometheus..............[ok]
linkerd-api: no invalid service profiles...................................[ok]
linkerd-version: can determine the latest version..........................[ok]
linkerd-version: cli is up-to-date.........................................[ok]
linkerd-version: control plane is up-to-date...............................[ok]

Status check results are [ok]

Environment

  • Kubernetes Version: 1.12.3
  • Cluster Environment: Rancher 2.1.3
  • Host OS: RancherOS 1.4.2
  • Linkerd version: 2.1.0

Possible solution

Maybe sed s'/stable-2.0.0/stable-2.1.0/g' ?

Additional context

linkerd check --proxy output

kubernetes-api: can initialize the client..................................[ok]
kubernetes-api: can query the Kubernetes API...............................[ok]
kubernetes-api: is running the minimum Kubernetes API version..............[ok]
linkerd-api: control plane namespace exists................................[ok]
linkerd-api: control plane pods are ready..................................[ok]
linkerd-api: can initialize the client.....................................[ok]
linkerd-api: can query the control plane API...............................[ok]
linkerd-api[kubernetes]: control plane can talk to Kubernetes..............[ok]
linkerd-api[prometheus]: control plane can talk to Prometheus..............[ok]
linkerd-api: no invalid service profiles...................................[ok]
linkerd-data-plane: data plane proxies are ready...........................[retry] -- The "linkerd/web-568678896c-p4pgn" pod is not running                                                                                                   
linkerd-data-plane: data plane proxies are ready...........................[retry] -- The "linkerd/web-568678896c-mdphg" pod is not running 
...
@klingerf
Copy link
Contributor

Hey @kamushadenes, thanks for reporting this. The issue that you're running into is described by this pre-inject warning:

sidecar: pods do not have a proxy or initContainer already injected........[warn] -- known sidecar detected in deployment/some-service                                                                                                 

It looks like you're trying to inject a YAML spec that already includes a linkerd-proxy sidecar, so inject skips it rather than overwriting. The fix is to use your original, un-injected YAML spec, inject with linkerd 2.1, and apply.

That said, it does seem like this would be a common use case for upgrading. Something like:

kubectl get deploy/web -oyaml | linkerd inject - | kubectl apply -f -

We should consider modifying the inject behavior to allow it to mutate resources that have already been injected. That might be a good thing to tackle as part of #1748.

@kamushadenes
Copy link
Author

@klingerf thanks for the quick response.

Manually setting the image to stable-2.1.0 should work?

@klingerf
Copy link
Contributor

That would typically work, but there was a breaking name change between 2.0 and 2.1, unfortunately. See the 2.1 upgrade notice here for more info: https://linkerd.io/2/upgrade/index.html#upgrade-notice-stable-2-1-0

@klingerf
Copy link
Contributor

klingerf commented Dec 17, 2018

Let's use this issue to track adding support for re-injecting resources that have already been injected with linkerd. On a related note, we could consider adding support for linkerd uninject.

@klingerf klingerf changed the title linkerd inject not upgrading data plane from 2.0.0 to 2.1.0 Add ability to re-inject resources that were already injected Dec 19, 2018
@alpeb alpeb self-assigned this Jan 7, 2019
@alpeb
Copy link
Member

alpeb commented Jan 7, 2019

How would folks feel if linkerd inject would always return a JSON patch, to be piped into kubectl patch? This would cater for both injecting for the first time and for updates. It would also be in line with the patch that the mutating webhook needs to produce, thus providing a starting point for the unification effort (#1748).

@grampelberg
Copy link
Contributor

@alpeb could you explain a little bit more? I'm having a tough time visualizing the use cases and interaction. In particular, I'm wondering how the output looks (the auditability of inject is one of its main advantages) and the difference between initial usage (linkerd inject -f foo.yaml) and later usage (kubectl get -o yaml | linkerd inject).

@alpeb
Copy link
Member

alpeb commented Jan 7, 2019

@grampelberg both initial and later usage would remain the same, but replacing apply with patch:

linkerd inject -f foo.yaml | kubectl patch -f -
kubectl get -o yaml | linkerd inject | kubectl patch -f -

Currently in the later usage case you need to provide the uninjected yaml. With what I'm suggesting that wouldn't be the case anymore; you would be able to update to a new version an already injected deployment.
As for looks, linkerd inject would return something like this:

[
    {
        "op": "add",
        "path": "/spec/template/metadata/annotations",
        "value": {
            "linkerd.io/created-by": "linkerd/cli stable-2.1.0",
            "linkerd.io/proxy-version": "stable-2.1.0"
        },
    },
    {
        "op": "add",
        "path": "/spec/template/spec/initContainers",
        "value": {
            "args": [
                "--incoming-proxy-port",
                "4143",
                "--outgoing-proxy-port",
                "4140",
                "--proxy-uid",
                "2102",
                "--inbound-ports-to-ignore",
                "4190,4191"
            ],
            "image": "gcr.io/linkerd-io/proxy-init:stable-2.1.0",
            "imagePullPolicy": "IfNotPresent",
            "name": "linkerd-init",
            "resources": {},
            "securityContext": {
                "capabilities": {
                    "add": [
                        "NET_ADMIN"
                    ]
                },
                "privileged": false
            },
            "terminationMessagePath": "/dev/termination-log",
            "terminationMessagePolicy": "FallbackToLogsOnError"
        }
    },
    {
        "op": "add",
        "path": "/spec/template/spec/containers",
        "value": {
            "env": [
                {
                    "name": "LINKERD2_PROXY_LOG",
                    "value": "warn,linkerd2_proxy=info"
                },
                {
                    "name": "LINKERD2_PROXY_BIND_TIMEOUT",
                    "value": "10s"
                },
                {
                    "name": "LINKERD2_PROXY_CONTROL_URL",
                    "value": "tcp://linkerd-proxy-api.linkerd.svc.cluster.local:8086"
                },
                {
                    "name": "LINKERD2_PROXY_CONTROL_LISTENER",
                    "value": "tcp://0.0.0.0:4190"
                },
                {
                    "name": "LINKERD2_PROXY_METRICS_LISTENER",
                    "value": "tcp://0.0.0.0:4191"
                },
                {
                    "name": "LINKERD2_PROXY_OUTBOUND_LISTENER",
                    "value": "tcp://127.0.0.1:4140"
                },
                {
                    "name": "LINKERD2_PROXY_INBOUND_LISTENER",
                    "value": "tcp://0.0.0.0:4143"
                },
                {
                    "name": "LINKERD2_PROXY_DESTINATION_PROFILE_SUFFIXES",
                    "value": "."
                },
                {
                    "name": "LINKERD2_PROXY_POD_NAMESPACE",
                    "valueFrom": {
                        "fieldRef": {
                            "apiVersion": "v1",
                            "fieldPath": "metadata.namespace"
                        }
                    }
                }
            ],
            "image": "gcr.io/linkerd-io/proxy:stable-2.1.0",
            "imagePullPolicy": "IfNotPresent",
            "livenessProbe": {
                "failureThreshold": 3,
                "httpGet": {
                    "path": "/metrics",
                    "port": 4191,
                    "scheme": "HTTP"
                },
                "initialDelaySeconds": 10,
                "periodSeconds": 10,
                "successThreshold": 1,
                "timeoutSeconds": 1
            },
            "name": "linkerd-proxy",
            "ports": [
                {
                    "containerPort": 4143,
                    "name": "linkerd-proxy",
                    "protocol": "TCP"
                },
                {
                    "containerPort": 4191,
                    "name": "linkerd-metrics",
                    "protocol": "TCP"
                }
            ],
            "readinessProbe": {
                "failureThreshold": 3,
                "httpGet": {
                    "path": "/metrics",
                    "port": 4191,
                    "scheme": "HTTP"
                },
                "initialDelaySeconds": 10,
                "periodSeconds": 10,
                "successThreshold": 1,
                "timeoutSeconds": 1
            },
            "resources": {},
            "securityContext": {
                "runAsUser": 2102
            },
            "terminationMessagePath": "/dev/termination-log",
            "terminationMessagePolicy": "FallbackToLogsOnError"
        }
    }
]

@grampelberg
Copy link
Contributor

It'd be nice to have a user friendly view of that, maybe diff based? The other problem is that patch doesn't take -f - for the actual path =/ -f is supposed to be the way it locates what to apply the patch to.

@alpeb
Copy link
Member

alpeb commented Jan 8, 2019

Thanks for the feedback @grampelberg. I understand now why kubectl patch is not an option. I'll open a PR for the new linkerd uninject, that can be leveraged by linkerd inject as a preliminary step 👍

alpeb added a commit that referenced this issue Jan 9, 2019
uninject.go iterates through the resources annotations, labels,
initContainers and Containers, removing what we know was injected by
linkerd.

The biggest part of this commit is the refactoring of inject.go, to make
it more generic and reusable by uninject. Some common parts were moved
into inject_util.go but others common parts remained in inject.go to
ease review and will then later get moved (marked with a comment).

The entry-point function `transformInput` (formerly called
`runInjectCmd`) receives as a parameter the function holding the logic
specific for injection (`injectResource`) or uninjection
(`uninjectResource`).

`injectResource` was split into the `parse` (reused by uninject) and
`injectResource` methods on the new `resourceConfig` receiver. That
receiver holds all the info required for the injection/uninjection.

The idea is that in a following PR this functionality will get reused by
`linkerd inject` to uninject as as preliminary step to injection, as a
solution to #1970.

This was tested successfully on emojivoto with:

```
1) inject:
kubectl get -n emojivoto deployment -o yaml | bin/linkerd inject - |
kubectl apply -f -
2) uninject:
kubectl get -n emojivoto deployment -o yaml | bin/linkerd uninject - |
kubectl apply -f -
```

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Jan 14, 2019
Add `linkerd uninject` command

uninject.go iterates through the resources annotations, labels,
initContainers and Containers, removing what we know was injected by
linkerd.

The biggest part of this commit is the refactoring of inject.go, to make
it more generic and reusable by uninject.

The idea is that in a following PR this functionality will get reused by
`linkerd inject` to uninject as as preliminary step to injection, as a
solution to #1970.

This was tested successfully on emojivoto with:

```
1) inject:
kubectl get -n emojivoto deployment -o yaml | bin/linkerd inject - |
kubectl apply -f -
2) uninject:
kubectl get -n emojivoto deployment -o yaml | bin/linkerd uninject - |
kubectl apply -f -
```

Also created unit tests for uninject.go. The fixture files from the inject
tests could be reused. But as now the input files act as outputs, they
represent existing resources and required these changes (that didn't
affect inject):
  - Rearranged fields in alphabetical order.
  - Added fields that are only relevant for existing resources (e.g.
    creationTimestamp and status.replicas in StatefulSets)

Signed-off-by: Alejandro Pedraza <[email protected]>
alpeb added a commit that referenced this issue Jan 15, 2019
Fixes #1970

The fixture `inject_emojivoto_already_injected.input.yml` is no longer
rejected, so I created the corresponding golden file.

Note that we'll still forbid injection over resources already injected
with third party meshes (Istio, Contour), so now we have
`HasExisting3rdPartySidecars()` to detect that.

Signed-off-by: Alejandro Pedraza <[email protected]>
klingerf pushed a commit that referenced this issue Jan 17, 2019
* When injecting, perform an uninject as a first step

Fixes #1970

The fixture `inject_emojivoto_already_injected.input.yml` is no longer
rejected, so I created the corresponding golden file.

Note that we'll still forbid injection over resources already injected
with third party meshes (Istio, Contour), so now we have
`HasExisting3rdPartySidecars()` to detect that.

* Generalize HasExistingSidecars() to cater for both the auto-injector and
* Convert `linkerd uninject` result format to the one used in `linkerd inject`.
* More updates to the uninject reports. Revert changes to the HasExistingSidecars func.

Signed-off-by: Alejandro Pedraza <[email protected]>
@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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants