Skip to content
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.

Sign up for GitHub

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

Explicitly explain "fail closed". #252

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Explicitly explain "fail closed". #252

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d7a229
Explicitly explain "fail closed".
fasaxc Oct 14, 2024
6785c21
Hint that mismatched CRD is a misconfig.
fasaxc Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions apis/v1alpha1/adminnetworkpolicy_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,9 +220,14 @@ type AdminNetworkPolicyEgressRule struct {
type AdminNetworkPolicyRuleAction string

// AdminNetworkPolicyEgressPeer defines a peer to allow traffic to.
// Exactly one of the selector pointers must be set for a given peer. If a
// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/2091-admin-network-policy#general-notes-on-the-adminnetworkpolicy-api

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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".

Copy link
Member

Choose a reason for hiding this comment

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

what do you mean here by "fail closed"? I think we should not use this phrase as it is confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

// been specified and fail closed.
//
// 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.
//
// +kubebuilder:validation:MaxProperties=1
// +kubebuilder:validation:MinProperties=1
type AdminNetworkPolicyEgressPeer struct {
Expand Down
11 changes: 8 additions & 3 deletions apis/v1alpha1/baselineadminnetworkpolicy_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,9 +195,14 @@ type BaselineAdminNetworkPolicyEgressRule struct {
type BaselineAdminNetworkPolicyRuleAction string

// BaselineAdminNetworkPolicyEgressPeer defines a peer to allow traffic to.
// Exactly one of the selector pointers must be set for a given peer. If a
// 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
// been specified and fail closed.
//
// 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.
//
// +kubebuilder:validation:MaxProperties=1
// +kubebuilder:validation:MinProperties=1
type BaselineAdminNetworkPolicyEgressPeer struct {
Expand Down
13 changes: 9 additions & 4 deletions apis/v1alpha1/shared_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,10 +120,15 @@ type PortRange struct {
End int32 `json:"end"`
}

// AdminNetworkPolicyIngressPeer defines an in-cluster peer to allow traffic from.
// Exactly one of the selector pointers must be set for a given peer. If a
// consumer observes none of its fields are set, they must assume an unknown
// option has been specified and fail closed.
// AdminNetworkPolicyIngressPeer defines a peer to allow traffic to.
// 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
// been specified and fail closed.
//
// 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.
//
// +kubebuilder:validation:MaxProperties=1
// +kubebuilder:validation:MinProperties=1
type AdminNetworkPolicyIngressPeer struct {
Expand Down
24 changes: 17 additions & 7 deletions config/crd/experimental/policy.networking.k8s.io_adminnetworkpolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,9 +213,14 @@ spec:
items:
description: |-
AdminNetworkPolicyEgressPeer defines a peer to allow traffic to.
Exactly one of the selector pointers must be set for a given peer. If a
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
been specified and fail closed.


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.
maxProperties: 1
minProperties: 1
properties:
Expand Down Expand Up @@ -583,10 +588,15 @@ spec:
Support: Core
items:
description: |-
AdminNetworkPolicyIngressPeer defines an in-cluster peer to allow traffic from.
Exactly one of the selector pointers must be set for a given peer. If a
consumer observes none of its fields are set, they must assume an unknown
option has been specified and fail closed.
AdminNetworkPolicyIngressPeer defines a peer to allow traffic to.
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
been specified and fail closed.


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.
maxProperties: 1
minProperties: 1
properties:
Expand Down
24 changes: 17 additions & 7 deletions config/crd/experimental/policy.networking.k8s.io_baselineadminnetworkpolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,9 +203,14 @@ spec:
items:
description: |-
BaselineAdminNetworkPolicyEgressPeer defines a peer to allow traffic to.
Exactly one of the selector pointers must be set for a given peer. If a
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
been specified and fail closed.


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.
maxProperties: 1
minProperties: 1
properties:
Expand Down Expand Up @@ -522,10 +527,15 @@ spec:
Support: Core
items:
description: |-
AdminNetworkPolicyIngressPeer defines an in-cluster peer to allow traffic from.
Exactly one of the selector pointers must be set for a given peer. If a
consumer observes none of its fields are set, they must assume an unknown
option has been specified and fail closed.
AdminNetworkPolicyIngressPeer defines a peer to allow traffic to.
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
been specified and fail closed.


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.
maxProperties: 1
minProperties: 1
properties:
Expand Down
24 changes: 17 additions & 7 deletions config/crd/standard/policy.networking.k8s.io_adminnetworkpolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,9 +203,14 @@ spec:
items:
description: |-
AdminNetworkPolicyEgressPeer defines a peer to allow traffic to.
Exactly one of the selector pointers must be set for a given peer. If a
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
been specified and fail closed.


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.
maxProperties: 1
minProperties: 1
properties:
Expand Down Expand Up @@ -428,10 +433,15 @@ spec:
Support: Core
items:
description: |-
AdminNetworkPolicyIngressPeer defines an in-cluster peer to allow traffic from.
Exactly one of the selector pointers must be set for a given peer. If a
consumer observes none of its fields are set, they must assume an unknown
option has been specified and fail closed.
AdminNetworkPolicyIngressPeer defines a peer to allow traffic to.
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
been specified and fail closed.


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.
maxProperties: 1
minProperties: 1
properties:
Expand Down
24 changes: 17 additions & 7 deletions config/crd/standard/policy.networking.k8s.io_baselineadminnetworkpolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,9 +193,14 @@ spec:
items:
description: |-
BaselineAdminNetworkPolicyEgressPeer defines a peer to allow traffic to.
Exactly one of the selector pointers must be set for a given peer. If a
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
been specified and fail closed.


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.
maxProperties: 1
minProperties: 1
properties:
Expand Down Expand Up @@ -414,10 +419,15 @@ spec:
Support: Core
items:
description: |-
AdminNetworkPolicyIngressPeer defines an in-cluster peer to allow traffic from.
Exactly one of the selector pointers must be set for a given peer. If a
consumer observes none of its fields are set, they must assume an unknown
option has been specified and fail closed.
AdminNetworkPolicyIngressPeer defines a peer to allow traffic to.
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
been specified and fail closed.


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.
maxProperties: 1
minProperties: 1
properties:
Expand Down