Skip to content

policy/api: Support unmanaged entity in policies#12474

Merged
aanm merged 1 commit intomasterfrom
pr/pchaigno/unmanaged-entity
Jul 10, 2020
Merged

policy/api: Support unmanaged entity in policies#12474
aanm merged 1 commit intomasterfrom
pr/pchaigno/unmanaged-entity

Conversation

@pchaigno
Copy link
Copy Markdown
Member

@pchaigno pchaigno commented Jul 9, 2020

0a9685e introduced the reserved:unmanaged identity, but did not introduce the associated entity. This commit fixes it and allows the following in policies:

- fromEntity:
  - unmanaged

The goal is to allow policies to work even before all pods are restarted and managed by Cilium. In particular, this can be an issue with host policies since the host is more likely than pods to need to talk with unmanaged pods (e.g., on GKE right after deploying Cilium).

Fixes: #5898

0a9685e introduced the reserved:unmanaged identity, but did not
introduce the associated entity. This commit fixes it and allows the
following in policies:

    - fromEntity:
      - unmanaged

The goal is to allow policies to work even before all pods are restarted
and managed by Cilium. In particular, this can be an issue with host
policies since the host is more likely than pods to need to talk with
unmanaged pods (e.g., on GKE right after deploying Cilium).

Fixes: 0a9685e ("identity: Introduce reserved:unmanaged identity")
Signed-off-by: Paul Chaignon <paul@cilium.io>
@pchaigno pchaigno added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. needs-backport/1.8 labels Jul 9, 2020
@pchaigno pchaigno requested review from a team as code owners July 9, 2020 14:18
@pchaigno
Copy link
Copy Markdown
Member Author

pchaigno commented Jul 9, 2020

test-me-please

@coveralls
Copy link
Copy Markdown

coveralls commented Jul 9, 2020

Coverage Status

Coverage increased (+0.02%) to 36.957% when pulling b7c5884 on pr/pchaigno/unmanaged-entity into 4814e1c on master.

Copy link
Copy Markdown
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with the change, but it's one more thing for users to sort through. Is there a use case you have in mind where you want to allow unmanaged but allowing cluster is too broad? Otherwise cluster should be sufficient.

@maintainer-s-little-helper maintainer-s-little-helper Bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 9, 2020
@pchaigno
Copy link
Copy Markdown
Member Author

Is there a use case you have in mind where you want to allow unmanaged but allowing cluster is too broad? Otherwise cluster should be sufficient.

I currently do frequent tests on GKE with an ingress+egress host policy that only allows the strict minimum. Whenever I forget to restart pods after deploying Cilium (pretty much every single time...), the cluster breaks in all sorts of ways. Having the unmanaged entity allows me to whitelist communications to/from unmanaged pods until they are restarted.

I'm unsure if we can expect users to run into the same scenario, but it's certainly handy to have unmanaged if they ever do. cluster would whitelist a lot more than the policy intends in this case.

it's one more thing for users to sort through

I didn't really consider that TBH. This pull request doesn't introduce any new concept; users already have to know about unmanaged pods. I'm just allowing to match on these in policies and making our entities consistent in the process: AFAICS, all our other reserved labels have corresponding entities.

@aanm aanm merged commit 5e9c427 into master Jul 10, 2020
@aanm aanm deleted the pr/pchaigno/unmanaged-entity branch July 10, 2020 13:33
@brb brb mentioned this pull request Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants