Skip to content

policy/api: Add reserved:health entity#12199

Merged
aanm merged 1 commit intomasterfrom
pr/add-reserved-health
Jun 29, 2020
Merged

policy/api: Add reserved:health entity#12199
aanm merged 1 commit intomasterfrom
pr/add-reserved-health

Conversation

@pchaigno
Copy link
Copy Markdown
Member

@pchaigno pchaigno commented Jun 19, 2020 •
edited
Loading

With the introduction of CiliumClusterwideNetworkPolicies a policy has
the ability to select the 'reserved:health' endpoints. If a user applies
a policy such as [0], it will block all connectivity for the health
endpoints. As this does not seem expected, since Cilium will report that
the connectivity across Cilium pods is not working, the user will need to
deploy a CCNP that allows connectivity between health endpoints. This
can be done with [1].

This commit introduces 'reserved:health' as an entity that can be
selected by CNP and / or CCNP.

[0]

---
apiVersion: cilium.io/v2
kind: CiliumClusterwideNetworkPolicy
metadata:
  name: ingress-deny-all
spec:
  endpointSelector:
    matchLabels: {}
  ingress:
  - {}
---
apiVersion: cilium.io/v2
kind: CiliumClusterwideNetworkPolicy
metadata:
  name: egress-deny-all
spec:
  endpointSelector:
    matchLabels: {}
  egress:
  - {}

[1]

apiVersion: "cilium.io/v2"
kind: CiliumClusterwideNetworkPolicy
metadata:
  name: "allow-reserved-health"
spec:
  endpointSelector:
    matchLabels:
      'reserved:health': ""
  ingress:
    - fromEntities:
      - health

Related: #11099 (which I couldn't reopen)
Signed-off-by: André Martins andre@cilium.io

@pchaigno pchaigno added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.7 labels Jun 19, 2020
@pchaigno pchaigno requested a review from aanm June 19, 2020 09:07
aanm
aanm approved these changes Jun 19, 2020 •
edited
Loading
View reviewed changes
@aanm
Copy link
Copy Markdown
Member

aanm commented Jun 19, 2020

cc @joestringer

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 19, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.006%) to 36.941% when pulling afa3711 on pr/add-reserved-health into 978248b on master.

@pchaigno pchaigno force-pushed the pr/add-reserved-health branch from 0eb5de1 to e3fe9b5 Compare June 19, 2020 12:08
@pchaigno pchaigno marked this pull request as ready for review June 19, 2020 12:08
@pchaigno pchaigno requested review from a team as code owners June 19, 2020 12:08
@pchaigno pchaigno force-pushed the pr/add-reserved-health branch from e3fe9b5 to f402246 Compare June 22, 2020 16:08
@pchaigno
Copy link
Copy Markdown
Member Author

pchaigno commented Jun 22, 2020

I updated the PR to also add entity health to entity cluster.

@pchaigno pchaigno force-pushed the pr/add-reserved-health branch from f402246 to 0c04d0e Compare June 22, 2020 16:10
@pchaigno pchaigno changed the title policy/api: Add ability to select reserved:health entity from CCNP policy/api: Add reserved:health entity Jun 22, 2020
@pchaigno
Copy link
Copy Markdown
Member Author

pchaigno commented Jun 22, 2020

test-me-please

@pchaigno
Copy link
Copy Markdown
Member Author

pchaigno commented Jun 23, 2020

retest-runtime

@aanm aanm requested a review from joestringer June 23, 2020 15:18
@aanm
Copy link
Copy Markdown
Member

aanm commented Jun 23, 2020

@joestringer PTAL as you had concerns in the original PR

@pchaigno
Copy link
Copy Markdown
Member Author

pchaigno commented Jun 24, 2020

@joestringer's comment from the original PR:

I do wonder if we could handle this better on the internal side. Seems a bit weird that the user is forced to think about the policy implications for infrastructure-level checks run by the daemon. Suggestion is something like excluding cilium-health endpoint from CCNP. Though we also would need to be quite careful there to ensure that any such changes don't accidentally apply to other endpoints and open an unintended hole.

I'm fine with this approach, but for consistency, I would still expect a health entity to be there.
Also, would we want to backport the whitelisting as well?

@joestringer
Copy link
Copy Markdown
Member

joestringer commented Jun 26, 2020

I would still expect a health entity to be there.

➕

Also, would we want to backport the whitelisting as well?

That would be subject to risk assessment but seems reasonable to me assuming we are confident we aren't allowing additional traffic unintentionally.

I'm fine to merge & backport this PR as-is, it provides a workaround for now but it'd be nice to follow up on the whitelisting approach so users don't have to care.

joestringer
joestringer reviewed Jun 26, 2020
View reviewed changes
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 note that there's no testing in this PR but the changes look fine.

@aanm @pchaigno
policy/api: add ability to select reserved:health entity from CCNP
afa3711 
With the introduction of CiliumClusterwideNetworkPolicies a policy has
the ability to select the 'reserved:health' endpoints. If a user applies
a policy such as [0], it will block all connectivity for the health
endpoints. As this does not seem expected, since Cilium will report that
the connectivity across Cilium pods is not working, the user will need to
deploy a CCNP that allows connectivity between health endpoints. This
can be done with [1].

This commit introduces 'reserved:health' as an entity that can be
selected by CNP and / or CCNP.

[0]
```
---
apiVersion: cilium.io/v2
kind: CiliumClusterwideNetworkPolicy
metadata:
  name: ingress-deny-all
spec:
  endpointSelector:
    matchLabels: {}
  ingress:
  - {}
---
apiVersion: cilium.io/v2
kind: CiliumClusterwideNetworkPolicy
metadata:
  name: egress-deny-all
spec:
  endpointSelector:
    matchLabels: {}
  egress:
  - {}
```

[1]
```
apiVersion: "cilium.io/v2"
kind: CiliumClusterwideNetworkPolicy
metadata:
  name: "allow-reserved-health"
spec:
  endpointSelector:
    matchLabels:
      'reserved:health': ""
  ingress:
    - fromEntities:
      - health
```

Co-authored-by: Paul Chaignon <paul@cilium.io>
Signed-off-by: André Martins <andre@cilium.io>
@pchaigno pchaigno force-pushed the pr/add-reserved-health branch from 0c04d0e to afa3711 Compare June 26, 2020 18:36
@pchaigno
Copy link
Copy Markdown
Member Author

pchaigno commented Jun 26, 2020

test-me-please

@pchaigno
Copy link
Copy Markdown
Member Author

pchaigno commented Jun 26, 2020

retest-net-next

@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 Jun 26, 2020
@aanm aanm merged commit 8390305 into master Jun 29, 2020
@aanm aanm deleted the pr/add-reserved-health branch June 29, 2020 08:56
@christarazi christarazi mentioned this pull request Jun 30, 2020
Merged
@christarazi christarazi mentioned this pull request Jun 30, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joestringer joestringer joestringer left review comments

@aanm aanm aanm approved these changes

@qmonnet qmonnet qmonnet approved these changes

+1 more reviewer

@vadorovsky vadorovsky vadorovsky approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@pchaigno @aanm @coveralls @joestringer @qmonnet @vadorovsky @christarazi