Explicitly explain "fail closed". #252
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @fasaxc! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @fasaxc. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
✅ Deploy Preview for kubernetes-sigs-network-policy-api ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| // consumer observes none of its fields are set, they must assume an unknown | ||
| // option has been specified and fail closed. | ||
| // Exactly one of the fields must be set for a given peer. If a consumer | ||
| // observes none of its fields are set, they must assume an unknown option has |
There was a problem hiding this comment.
I am a bit confused by this comment, "unknown option"... do you mean other values outside of the 5 allowed peers? Should yaml creation fail ? or does it get ignored internall? could you please show an example of the fail scenario you mean?
There was a problem hiding this comment.
My understanding is that we want to make sure that we guard against upgrading the CRD without upgrading the implementation. If you upgrade to a new CRD version and it has a new field in the struct, we want the old implementation to spot that the struct appears empty and then to infer that an unsupported field must be present.
That design decision was made before I joined the group; I'm not sure how realistic it is given that the implementations are bundling the CRDs (so as you say, CRD validation should kick in)? But it seemed to be a design goal for the struct.
There was a problem hiding this comment.
Hm... the thinking here was based on experience with NetworkPolicy where you have to deal with the possibility of the API and the implementation being out-of-sync.
It's possible that this doesn't really make sense in the ANP / CRD model...
There was a problem hiding this comment.
I have done some research, and here is what I found:
The AdminNetworkPolicySubject and AdminNetworkPolicyPeer types are explicitly
designed to allow for future extensibility with a focus on the addition of new types of
selectors. Specifically it will allow for failing closed in the event an implementation does
not implement a defined selector. For example, If a new type (ServiceAccounts) was
added to the AdminNetworkPolicyPeer struct, and an implementation had not yet
implemented support for such a selector, an ANP using the new selector would have no
effect since the implementation would simply see an empty AdminNetworkPolicyPeer
object.
I am not quite sure, if "ANP using the new selector would have no effect" actually means that "an empty AdminNetworkPolicyPeer" should have no effect. But I think "deny all" was not a part of the plan.
There was a problem hiding this comment.
But if a user tried to create an ANP with a ServiceAccounts selector against the current v1alpha1 CRD, it would fail, because CRD validation would not see that field in the object (since it's not part of the CRD), and so then the MinProperties=1 on AdminNetworkPolicyEgressPeer would be false.
The only way there could be a problem would be if the ANP implementation installed a version of the CRD that includes ServiceAccounts, but the ANP code hadn't been updated to know about it.
I guess perhaps this could happen though, if you had updated the CRDs because you wanted to implement a different new feature, but hadn't also implemented this new feature yet?
So in that case the advice is correct, but maybe confusingly worded. We should say something like "if an implementation does not implement all of the peer types in the version of the CRD that it installs, then it needs to fail closed when one of the unimplemented peer types is used".
There was a problem hiding this comment.
what do you mean here by "fail closed"? I think we should not use this phrase as it is confusing.
There was a problem hiding this comment.
I mean, the new text Shaun already wrote. We would just be explaining better that that advice is only needed in the case where you have intentionally shipped a version of the CRD that contains features that you don't support.
There was a problem hiding this comment.
we have discussed on the last sig meeting another potential solution: ignore "empty" rule OR the whole ANP and report an error condition in the status.
One concern about denying all for an unknown peer type, is that it may break important connections. Best case is just some crashlooping pods, worst case is the whole cluster will be dead if it denies e.g. kube-components ingress, like for apiserver.
There was a problem hiding this comment.
I do think we need to do something to alert the user. Blocking traffic is extreme but it's the only tool we have until / unless we add a status or similar. I see this PR as just tidying up the wording that was already there (it already said "fail closed", I just wanted to make sure we wrote down the expectation).
Feels like adding a status would be a bigger change needing an NPEP.
There was a problem hiding this comment.
agree, the only concern I have is that changing poorly defined "fail closed" to a well-defined status is less of a breaking changing than changing now (if this PR merges) well-defined "deny all" to some status in the future.
| // | ||
| // For "Allow" rules, "fail closed" means: "treat the rule as matching no | ||
| // traffic". For "Deny" and "Pass" rules, "fail closed" means: "treat the rule | ||
| // as a 'Deny all' rule". |
There was a problem hiding this comment.
I'm happy with the current wording here. We might want to explain the logic behind this in a little more detail somewhere, but the API documentation isn't really the right place for that.
/approve
anyone else want to lgtm?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, fasaxc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Spell out the meaning of "fail closed" for the various rule actions. Tweak description to account for not all fields being pointers.