Skip to content

policy/api: add ability to select reserved:health entity from CCNP and / or CNP#11099

Closed
aanm wants to merge 1 commit intomasterfrom
pr/add-reserved-health
Closed

policy/api: add ability to select reserved:health entity from CCNP and / or CNP#11099
aanm wants to merge 1 commit intomasterfrom
pr/add-reserved-health

Conversation

@aanm
Copy link
Copy Markdown
Member

@aanm aanm commented Apr 22, 2020

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

Signed-off-by: André Martins andre@cilium.io

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
```

Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm 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 Apr 22, 2020
@aanm aanm requested a review from a team as a code owner April 22, 2020 10:05
@aanm
Copy link
Copy Markdown
Member Author

aanm commented Apr 22, 2020

test-me-please

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.02%) to 44.67% when pulling 2432996 on pr/add-reserved-health into 37abed5 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.

Simple change, low risk, LGTM.

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.

@joestringer joestringer added the needs/e2e-test This issue is not covered by existing CI tests, but should be. label Apr 22, 2020
@aanm aanm marked this pull request as draft May 1, 2020 15:15
@stale
Copy link
Copy Markdown

stale Bot commented May 31, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale Bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label May 31, 2020
@stale
Copy link
Copy Markdown

stale Bot commented Jun 14, 2020

This issue has not seen any activity since it was marked stale. Closing.

@stale stale Bot closed this Jun 14, 2020
@pchaigno pchaigno added needs-backport/1.8 and removed stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. needs/e2e-test This issue is not covered by existing CI tests, but should be. labels Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants