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

Can't inject on resources with the conduit.io/plane label. #201

Closed
olix0r opened this issue Jan 24, 2018 · 8 comments
Closed

Can't inject on resources with the conduit.io/plane label. #201

olix0r opened this issue Jan 24, 2018 · 8 comments
Assignees
Labels

Comments

@olix0r
Copy link
Member

olix0r commented Jan 24, 2018

Running a command like:

:; target/cli/macos/conduit install -n inception -v $(bin/root-tag) |target/cli/macos/conduit inject -v $(bin/root-tag) - |kubectl apply -f -

I'd expect us to add the proxy to all conduit pods in the inception namespace. Unfortunately, we use a single label, conduit.io/plane to indicate whether the node is in the control plane.

It shouldn't be a requirement that a pod is only in one plane.

Furthermore, we should be able to run inject on pods that already have been injected (i.e. to update the proxy configuration).

I propose moving these labels to annotations so that they are not mutually exclusive.

Something like conduit.io/data-plane conduit.io/control-plane?

@siggy siggy self-assigned this Jan 26, 2018
@siggy
Copy link
Member

siggy commented Jan 26, 2018

Proposed changes

Summary

conduit.io/plane: control => conduit.io/controller-component: web
conduit.io/controller: conduit => conduit.io/controller-ns: conduit
conduit.io/plane: data => (remove, redundant with conduit.io/controller-ns)

Before

conduit.io/plane: control: app is a member of the control plane
conduit.io/plane: data: app has an injected data plane
conduit.io/controller: conduit: app is connected to a controller deployed in the conduit namespace

conduit controller

annotations:
  conduit.io/created-by: conduit/cli git-7399df83
labels:
  app: controller
  conduit.io/plane: control

injected app

annotations:
  conduit.io/created-by: conduit/cli git-7399df83
  conduit.io/proxy-version: git-7399df83
labels:
  app: emoji-svc
  conduit.io/controller: conduit
  conduit.io/plane: data

After

conduit.io/controller-component: web: app is the web component of the control plane
conduit.io/controller-ns: conduit: app has an injected data plane, and is connected to a controller deployed in the conduit namespace

conduit controller

annotations:
  conduit.io/created-by: conduit/cli git-7399df83
labels:
  app: web
  conduit.io/controller-component: web

injected app

annotations:
  conduit.io/created-by: conduit/cli git-7399df83
  conduit.io/proxy-version: git-7399df83
labels:
  app: emoji-svc
  conduit.io/controller-ns: conduit

@olix0r
Copy link
Member Author

olix0r commented Jan 26, 2018

I think conduit.io/mesh might be slightly clearer as conduit.io/controller-ns or something similar that explicitly indicates what the value is. Mesh is a bit of an underdefined term...

The member thing is a little unfortunate -- that we have a label with only one value. I wonder if we would benefit from using labels like conduit.io/controller-component: web conduit.io/controller-component: prometheus conduit.io/controller-component: controller, etc.

This enables label selections like conduit.io/controller-component to simply determine which pods are acting as a part of a controller, as well as things like conduit.io/controller-component != web` to enumerate non-web components, etc.

TIOLI.

generally seems fine to me.

@siggy
Copy link
Member

siggy commented Jan 26, 2018

Proposal updated with:

  • conduit.io/controller-component
  • conduit.io/controller-ns

@briansmith
Copy link
Contributor

It shouldn't be a requirement that a pod is only in one plane.

It isn't clear to me what you are trying to accomplish with the "inception" example. Could you explain more what you're trying to do?

Furthermore, we should be able to run inject on pods that already have been injected (i.e. to update the proxy configuration).

I agree.

I propose moving these labels to annotations so that they are not mutually exclusive.

Something like conduit.io/data-plane conduit.io/control-plane?

We need these things to be labels because the Kubernetes API is good at selecting things based on labels and bad at selecting things based on any other criteria. For example, it is easy and efficient to find everything with the "conduit.io/control-plane" label. I don't know that it is easy and efficient to find everything with a similar annotation.

I think we do want to have the Conduit proxy injected into the control plane pods, however I don't think conduit inject should do that. Instead conduit install and/or other self-maintenance tasks in the control plane should manage the control plane's proxy configuration.

@briansmith
Copy link
Contributor

annotations:

  conduit.io/created-by: conduit/cli git-7399df83
labels:
  app: web
  conduit.io/controller-component: web

injected app

annotations:
  conduit.io/created-by: conduit/cli git-7399df83
  conduit.io/proxy-version: git-7399df83
labels:
  app: emoji-svc
  conduit.io/controller-ns: conduit

I think this new scheme is fine. However, I would like to prevent conduit inject on control plane components unless/until we have a clearer plan for it. I think we should have the Conduit proxy injected into some or all of the control plane pods, but whether that happens should be controlled by Conduit and not be something decided by the end-user.

(To be crystal clear, the app labels aren't actually injected by Conduit, right?)

@siggy
Copy link
Member

siggy commented Jan 27, 2018

@briansmith Agree on all points.

It isn't clear to me what you are trying to accomplish with the "inception" example. Could you explain more what you're trying to do?

This is really just a proof of concept. Agree we can leverage the proxy between the control plane components, but it should not be left to the user to define that configuration.

We need these things to be labels because the Kubernetes API is good at selecting things based on labels and bad at selecting things based on any other criteria.

Yup, we're sticking with labels. My proposal above is what we're planning to implement.

(To be crystal clear, the app labels aren't actually injected by Conduit, right?)

Correct, I left the app label in there to help with context, it's not injected.

@briansmith
Copy link
Contributor

OK, perfect.

siggy added a commit that referenced this issue Jan 29, 2018
The conduit.io/* k8s labels and annotations we're redundant in some
cases, and not flexible enough in others.

This change modifies the labels in the following ways:
`conduit.io/plane: control` => `conduit.io/controller-component: web`
`conduit.io/controller: conduit` => `conduit.io/controller-ns: conduit`
`conduit.io/plane: data` => (remove, redundant with `conduit.io/controller-ns`)
It also centralizes all k8s labels and annotations into
pkg/k8s/labels.go, and adds tests for the install command.

Part of #201

wip

wip

wip

wip
siggy added a commit that referenced this issue Jan 29, 2018
The conduit.io/* k8s labels and annotations we're redundant in some
cases, and not flexible enough in others.

This change modifies the labels in the following ways:
`conduit.io/plane: control` => `conduit.io/controller-component: web`
`conduit.io/controller: conduit` => `conduit.io/controller-ns: conduit`
`conduit.io/plane: data` => (remove, redundant with `conduit.io/controller-ns`)
It also centralizes all k8s labels and annotations into
pkg/k8s/labels.go, and adds tests for the install command.

Part of #201

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
siggy added a commit that referenced this issue Feb 1, 2018
The conduit.io/* k8s labels and annotations we're redundant in some
cases, and not flexible enough in others.

This change modifies the labels in the following ways:
`conduit.io/plane: control` => `conduit.io/controller-component: web`
`conduit.io/controller: conduit` => `conduit.io/controller-ns: conduit`
`conduit.io/plane: data` => (remove, redundant with `conduit.io/controller-ns`)
It also centralizes all k8s labels and annotations into
pkg/k8s/labels.go, and adds tests for the install command.

Part of #201

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
siggy added a commit that referenced this issue Feb 1, 2018
The conduit.io/* k8s labels and annotations we're redundant in some
cases, and not flexible enough in others.

This change modifies the labels in the following ways:
`conduit.io/plane: control` => `conduit.io/controller-component: web`
`conduit.io/controller: conduit` => `conduit.io/controller-ns: conduit`
`conduit.io/plane: data` => (remove, redundant with `conduit.io/controller-ns`)
It also centralizes all k8s labels and annotations into
pkg/k8s/labels.go, and adds tests for the install command.

Part of #201

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
siggy added a commit that referenced this issue Feb 1, 2018
The conduit.io/* k8s labels and annotations we're redundant in some
cases, and not flexible enough in others.

This change modifies the labels in the following ways:
`conduit.io/plane: control` => `conduit.io/controller-component: web`
`conduit.io/controller: conduit` => `conduit.io/controller-ns: conduit`
`conduit.io/plane: data` => (remove, redundant with `conduit.io/controller-ns`)
It also centralizes all k8s labels and annotations into
pkg/k8s/labels.go, and adds tests for the install command.

Part of #201

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
siggy added a commit that referenced this issue Feb 1, 2018
The conduit.io/* k8s labels and annotations we're redundant in some
cases, and not flexible enough in others.

This change modifies the labels in the following ways:
`conduit.io/plane: control` => `conduit.io/controller-component: web`
`conduit.io/controller: conduit` => `conduit.io/controller-ns: conduit`
`conduit.io/plane: data` => (remove, redundant with `conduit.io/controller-ns`)
It also centralizes all k8s labels and annotations into
pkg/k8s/labels.go, and adds tests for the install command.

Part of #201

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
siggy added a commit that referenced this issue Feb 1, 2018
The conduit.io/* k8s labels and annotations we're redundant in some
cases, and not flexible enough in others.

This change modifies the labels in the following ways:
`conduit.io/plane: control` => `conduit.io/controller-component: web`
`conduit.io/controller: conduit` => `conduit.io/controller-ns: conduit`
`conduit.io/plane: data` => (remove, redundant with `conduit.io/controller-ns`)
It also centralizes all k8s labels and annotations into
pkg/k8s/labels.go, and adds tests for the install command.

Part of #201

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
siggy added a commit that referenced this issue Feb 1, 2018
The conduit.io/* k8s labels and annotations we're redundant in some
cases, and not flexible enough in others.

This change modifies the labels in the following ways:
`conduit.io/plane: control` => `conduit.io/controller-component: web`
`conduit.io/controller: conduit` => `conduit.io/controller-ns: conduit`
`conduit.io/plane: data` => (remove, redundant with `conduit.io/controller-ns`)
It also centralizes all k8s labels and annotations into
pkg/k8s/labels.go, and adds tests for the install command.

Part of #201

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
siggy added a commit that referenced this issue Feb 1, 2018
The conduit.io/* k8s labels and annotations we're redundant in some
cases, and not flexible enough in others.

This change modifies the labels in the following ways:
`conduit.io/plane: control` => `conduit.io/controller-component: web`
`conduit.io/controller: conduit` => `conduit.io/controller-ns: conduit`
`conduit.io/plane: data` => (remove, redundant with `conduit.io/controller-ns`)
It also centralizes all k8s labels and annotations into
pkg/k8s/labels.go, and adds tests for the install command.

Part of #201

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
@briansmith
Copy link
Contributor

There's already an another open issue, #311, for injecting the proxy into the control plane pods and this issue already had a PR landed for it, so I'm closing this.

ihcsim pushed a commit that referenced this issue Feb 27, 2019
This picks up the following commits:

* 0fe8063 replace  with  (#2370) (#201)
* 1ea7559 Minor cleanup in the config tests (#188)
* d0ef56b Update *ring* to 0.14.6 (#197)
* c54377f fs-watch: Use a properly sized buffer for inotify events (#195)
* 23e02a6 Update Router to wait for inner poll_ready before calling inner call
* 2de8e9b Update metrics quickcheck to 0.8, and hyper to 0.12.24
* d1bbd4b make: Optionally include debug symbols with builds (#193)
* 738a541 Fix compilation warnings in fs-watch (#192)
* 6cc7558 Apply rustfmt (#191)

Signed-off-by: Ivan Sim <ivan@buoyant.io>
khappucino pushed a commit to Nordstrom/linkerd2 that referenced this issue Mar 5, 2019
Currently, linkerd2-proxy uses and implements `Error::cause`
for error handling.

This branch removes the use of the deprecated
`Error::cause` method and implements and uses the
`Error::source` method, as [recommended by the
documentation](https://doc.rust-lang.org/std/error/trait.Error.html#method.cause). To implement `Error::source`, there are
static lifetimes added to the `Error` traits. However, this
doesn't remove the [use of `cause2`](https://github.com/DebugSteven/linkerd2-proxy/blob/231cb67b1a2178627bbb304055d408cee1007fd9/src/proxy/http/glue.rs#L226) because
`hyper::Error` doesn't provide a `source` yet:
https://docs.rs/hyper/0.12.24/src/hyper/error.rs.html#289-333.
A separate issue could be opened to use `source` for
`hyper::Error` when it becomes available.

Fixes: linkerd#2370

Signed-off-by: J Haigh <DebugSteven@gmail.com>
@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

5 participants