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

cni: Synchroneous pod label retrieval on CNI add #6299

Merged
merged 1 commit into from Nov 28, 2018

Conversation

@tgraf
Copy link
Member

tgraf commented Nov 27, 2018

Move away from asynchronous networking plumbing for CNI. This mode has been introduced to optimize the pod scheduling delay but it is causing some troubles when people perform benchmarks or rely on immediate DNS resolution success.

Leverage the CNI_ARGS to retrieve the pod name and namespace and issue a call
to the apiserver to retrieve the pod labels. This allows to retrieve the pod
labels and allocate the identity synchronously on the endpoint creation API
call.

This makes the CNI endpoint creation code divert from the CMM plugin. The CMM
plugin continues to rely on the workloads API to retrieve the metadata.

Consequences

  • The init identity will be skipped when an endpoint is managed by CNI
  • An additional identity change can occur if any of the additional container labels are security relevant.

Possible follow-ups

  • This work allows potentially disabling workload association altogether when running in CNI only mode

This change is Reviewable

@tgraf tgraf added this to the 1.4-feature milestone Nov 27, 2018

@tgraf tgraf added this to In Progress in 1.4 via automation Nov 27, 2018

@tgraf tgraf requested review from cilium/agent as code owners Nov 27, 2018

@tgraf

This comment has been minimized.

Copy link
Member Author

tgraf commented Nov 27, 2018

test-me-please

cni: Synchroneous pod label retrieval on CNI add
Leverage the CNI_ARGS to retrieve the pod name and namespace and issue a call
to the apiserver to retrieve the pod labels. This allows to retrieve the pod
labels and allocate the identity synchronously on the endpoint creation API
call.

This makes the CNI endpoint creation code divert from the CMM plugin. The CMM
plugin continues to rely on the workloads API to retrieve the metadata.

Signed-off-by: Thomas Graf <thomas@cilium.io>

@tgraf tgraf force-pushed the pr/tgraf/cni-args branch from 120d8d5 to 31a6b4f Nov 27, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 27, 2018

Coverage Status

Coverage decreased (-0.005%) to 43.478% when pulling 31a6b4f on pr/tgraf/cni-args into 1ad9445 on master.

@tgraf

This comment has been minimized.

Copy link
Member Author

tgraf commented Nov 27, 2018

test-me-please

@tgraf

This comment has been minimized.

Copy link
Member Author

tgraf commented Nov 27, 2018

Nominating for backport to 1.3 and 1.2 as this fixes a lot of init connectivity problems.

@tgraf tgraf added this to Proposed in 1.3.1 via automation Nov 27, 2018

@tgraf tgraf added this to Proposed in 1.2.6 via automation Nov 27, 2018

@jrajahalme
Copy link
Contributor

jrajahalme left a comment

LGTM

@jrajahalme

This comment has been minimized.

Copy link
Contributor

jrajahalme commented Nov 28, 2018

About potential for additional label changes mentioned in the description: With this change there should be at most as many (security relevant) label changes as before, but usually less?

@tgraf tgraf merged commit 65fe98c into master Nov 28, 2018

4 checks passed

Cilium-Ginkgo-Tests Build finished.
Details
Hound 3 violations found.
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.005%) to 43.478%
Details

1.4 automation moved this from In Progress to Done Nov 28, 2018

@tgraf tgraf deleted the pr/tgraf/cni-args branch Nov 28, 2018

@tgraf

This comment has been minimized.

Copy link
Member Author

tgraf commented Nov 28, 2018

About potential for additional label changes mentioned in the description: With this change there should be at most as many (security relevant) label changes as before, but usually less?

Correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment