KEP-2943: Add initial draft on Network Policy Status #2947
Conversation
k8s-ci-robot
added
sig/network
k8s-ci-robot
added
the
kind/kep
|
|
||
| ``` | ||
| <<[UNRESOLVED optional short context or usernames ]>> | ||
| We need to define if .status.PolicyStatus[].Status will have its own type with |
There was a problem hiding this comment.
Will the properly added network policy have a "GOOD" status like the parsed by default.. Or the idea is to reserve this for errors only.
There was a problem hiding this comment.
Could be good to have the possible status list as an enumeration with fixed values.
There was a problem hiding this comment.
I added some comments based on the things I wish people had told me about status when I first started writing status for Kube objects.
I would also recommend though, that unless namespacing the status by CNI plugin is vital, the Conditions should live at the root (will make the status much easier to parse by tooling).
| * Add the `NetworkPolicyStatus` struct as following: | ||
| ```go | ||
| // NetworkPolicyStatus describe the current state of the NetworkPolicy. | ||
| type NetworkPolicyStatus struct { |
There was a problem hiding this comment.
This reads like it's expected that a NetworkPolicy will be implemented by multiple CNIs (because the status is namespaced by CNI name)? Is that a requirement or a common use case?
There was a problem hiding this comment.
I actually removed this and turned into a Conditions field.
Maybe we should add in the conditions field a reporter for that (@thockin this is not common, but as a Network Policy can be consumed by multiple providers, should we add the Reporter of a condition as a field?)
There was a problem hiding this comment.
I don't think we need to add reporter at this point, see my comment below.
| type CNIPolicyStatus struct { | ||
| // CNI defines the CNI name | ||
| CNI string | ||
|
|
||
| // Status defines the status string (parsed, error, etc) | ||
| Status string | ||
|
|
||
| // Description defines the description of the Status (why it's on that status) | ||
| Description string | ||
| } |
There was a problem hiding this comment.
I'd recommend this use a metav1.Conditions array instead of just status and description fields, because:
- Conditions have the ObservedGeneration and LastTransitionTime fields that are essential for checking that the version of the status you're seeing matches the version of the object (that is, that things are not still being updated).
- Lets there be a an expected main summary condition like
AcceptedorValidthat, when itsstatusis true, indicates that everything was parsed correctly. When thestatusis false, then the Reason and Message field can be used to indicate the exact problem (like I would imagine Description is used here). - This also lets extra Conditions be easily added to the spec later (I would guess there might also be a use for a desired-true
ReadyCondition that lets you know when the rules have been completely programmed into the underlying CNI.
I asked the question above, but in the case that the status does need to be namespaced by CNI name, then in that case, this would look like:
type CNIPolicyStatus struct {
// CNI defines the CNI name that this status is written by.
CNI string
// Conditions holds an array of metav1,Conditions that describe the state of the NetworkPolicy.
// When a CNI or other NetworkPolicy controller implementation parses this object,
// it must ensure that the `Accepted` Condition is updated with the parsing outcome.
// The Reason and Description must indicate the cause of the failure, if there was one.
// (the Conditions field should also have a default that adds the `Accepted` Condition
// with a `status` of `Unknown`.)
Conditions []metav1.Conditon
}(edit: I tried a suggestion, but the diff was not very useful here.)
Ideally, the update will also include Reason constants for the common Condition reasons (like EverythingOK, FailedParsing or something).
The idea here is that this is a spec that needs to be implemented by lots of CNI plugins, so the more clear the behavior is, the less chance there is of drift between implementations.
There was a problem hiding this comment.
I second this recommendation; we could add an UnsupportedFeature NetworkPolicyConditionType as a new condition type which can make use of Message and Reason fields to provide more information on what the CNI is doing with regards to this.
type NetworkPolicyConditionType string
const (
NetworkPolicyReady NetworkPolicyConditionType = "Ready"
UnsupportedFeature NetworkPolicyConditionType = "UnsupportedFeature"
)
type NetworkPolicyCondition struct {
Type NetworkPolicyConditionType `json:"type,omitempty"`
// Status of the condition, one of True, False, Unknown.
Status v1.ConditionStatus `json:"status,omitempty"`
// +optional
// Last time the condition transited from one status to another.
LastTransitionTime metav1.Time `json:"lastTransitionTime,omitempty"`
// +optional
// A human readable message indicating details about the transition.
Message string `json:"message,omitempty"`
// +optional
// Unique, one-word, CamelCase reason for the condition's last transition.
Reason string `json:"reason,omitempty"`
}
type NetworkPolicyStatus struct {
Conditions []NetworkPolicyCondition `json:"conditions,omitempty"`
}
There was a problem hiding this comment.
I'm actually a -1 on making custom Condition types, I think unless you're adding something substantial to the metav1.Condition type, it's much better to use it directly. In particular, the ObservedGeneration field is absolutely vital for ensuring that the Condition isn't stale. Regardless, they'll deserialize the same, but code using the field defs directly will find it easier to use the metav1.Condition than a custom type. (I recommend this because I did it in Contour's HTTPProxy, and it was a mistake.)
I'd encourage supplying string constants for the Type, Reason and Message fields instead.
There was a problem hiding this comment.
I'm a -1 on custom Condition Types as well. I've updated the KEP to contain Conditions, I think we are on the same page here in this case
There was a problem hiding this comment.
Also, TIL that Conditions are actually a meta type. Let me fix here in the KEP
| ```go | ||
| type CNIPolicyStatus struct { | ||
| // CNI defines the CNI name | ||
| CNI string |
There was a problem hiding this comment.
CNI isn't involved in the enforcement of the network policy API, despite the fact that many network plugins also include a network policy implementation and some policy implementations rely on being involved in container networking.
I would recommend avoiding any naming that focuses on implementation details like this in general.
There was a problem hiding this comment.
PolicyControllerStatus?
|
@rikatz my proposal is in #2947 (comment) |
|
|
||
| ## Motivation | ||
|
|
||
| While implementing new features in Network Policy, we've realized that some CNIs were |
There was a problem hiding this comment.
It sounds like these scenarios are for providers that only partially implement the network policy API, or have some restrictions / limitations in the objects they will accept from the API.
If that is the case, should we be handling this via some form of validation so that users get immediate feedback rather than needing to seek it out?
If I create an "invalid" policy for my cluster, I'd like to know about it at the time I attempt to create it, rather than a few weeks down the road when something unexpected happens.
There was a problem hiding this comment.
I agree with you Casey, but in this case that should be some common Validation Webhook right?
Also, just reminding that this Status field was mostly a PRR request to provide some 'feature usage' feedback
There was a problem hiding this comment.
Yeah, I didn't mean to imply a solution. I was just suggesting that these use-cases sound better suited to validation as far as user-experience goes.
Could either be a webhook, some sort of capability negotiation, etc. Haven't thought that far into it!
There was a problem hiding this comment.
webhook would be the responsibility of individual network policy providers and not something that will be part of k/k.. right? but i agree that would be most appropriate, but not something we can solve with a KEP
There was a problem hiding this comment.
webhook would be the responsibility of individual network policy providers
Yes, but populating the conditions field of a NetworkPolicy status is also ultimately up to the network policy provider to actually implement.
| - Impact of its degraded performance or high-error rates on the feature: | ||
| --> | ||
|
|
||
| ### Scalability |
There was a problem hiding this comment.
There is a scalability concern. All nodes are watching for Network Policy events in order to perform their enforcement. If the status field is updated by a cluster-wide controller, or some other entity, it means that the number of events sent for each Network Policy created will be two: one for the creation and a second for the update of the Network Policy status.
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
I guess #2709 (comment) is a trigger for this KEP and I'm 100% supportive for this change (although I also just skimmed it). Re why not events - events are best-effort and ephemeral (default 1h TTL). When I launched my network-policy and then went to to 1h meeting, I would like to be able to learn why my network-policy doesn't work and I won't be able to do that from the event, because it was already GC-ed. But I would be able to figure this out from the status. |
|
@rikatz - will you be able to get back to this work? |
|
Hi folks, being completely honest with you: I WANT to keep the work on this, I'm not sure I CAN as I'm pretty low on bandwidth (pun intended) those last few days and don't wanna block anyone because of this. I've pinged other folks on sig-net-policy-api (@astoycos I'm looking at you!) to check if someone can help me on this, otherwise I can take care of the kep as soon as it doesn't block other Keps :) |
|
@wojtek-t I will re-read all the comments during this and next week and see what I can do. Will try to attend sig net netpol meeting as well and discuss with the group how they want to prioritize this, and how I can get some help. I know the majority of the group is focused on cluster scoped netpol, so maybe even from that work we can get something that can be reused here. @thockin what's your feeling about @wojtek-t comments on Events? I mean, I agree that the Events TTL may be frustrating, but OTOH events may bring some more "pattern" than some open status right? (if we use status, we will probably end up again on that problem of "we need a new status field for feature XPTO" in a future) |
@rikatz Totally understandable, I'm in the same boat. We had an out of band meeting with Tim last week and thankfully I don't think this is really blocking us at the moment with new network policy objects moving forward. I think with CNP at least we are going to have a standard CNP status field that is simply a In terms of this KEP (i.e adding status to existing netPol) that may be a bit more difficult since it is already a mature API, unless we follow the same vane an allow CNI's to simply populate the |
There was a problem hiding this comment.
OK, I remove my objections in general, though there are speciofic bits of feedback here I agree with.
- Conditions
- Define a standard "FeatureNotSupported" condition (or similar)
- Maybe an "Accepted" condition for well-behaved modern implementations to set
- Don't say "CNI"
Question: do we need to support multiple sets of conditions (e.g. per controller) ? Is multiple implementations of NetPol a thing? In theory it could be (e.g. iptables and in a cloud firewall)...
|
It sounds like "not for v1.24" right? |
k8s-ci-robot
added
size/XXL
There was a problem hiding this comment.
@aanm from "Non goals":
* Provide a feedback to users on whether a Network Policy was applied on all the cluster nodesI think that, from my perspective the field would be there and the providers can use the way they want, so if Cilium wants to use them to provide informations like:
- This kind of rule was blocked by the admin (like, users trying to implement a policy with CIDR 0.0.0.0/0 and this being blocked)
- The provider cannot parse "SCTP" rules
- Users passed an invalid CIDR
Are valid cases, whether
- Policy could not be applied in node XXXX
is a decision from the netpol provider to implement or not
@rikatz thanks for pointing this out! I read this proposal a couple of months back and I remember that the scalability was a non-issue because of the non-goal: "Provide a feedback to users on whether a Network Policy was applied on all the cluster nodes" but I miss this today. LGTM then
|
Just to echo and emphasize:
Any model that requires "all nodes" to check in is doomed to fail at scale. :) |
k8s-ci-robot
removed
the
lifecycle/stale
|
/lifecycle active |
k8s-ci-robot
added
the
lifecycle/active
| Should Network Policy Status cover multiple network policy provider conditions? | ||
| If so, how can this be solved? Prefixing the Type field with the implementation | ||
| name as `myprovider.com/PartialFailure?` | ||
|
|
||
| Also, what kind of polatiry should we use in a default condition Type? Positive polarity | ||
| like "Accepted=bool" or negative polarity like PartialFailure=true? | ||
|
|
||
| See the original discussion in https://github.com/kubernetes/enhancements/pull/2947#discussion_r796974848 |
There was a problem hiding this comment.
In Gateway API, we've settled on this, (based on my personal opinions as much as anything else):
- a small number (ideally one) of positive-polarity rollup conditions (
Acceptedwould be the go here), but I could see some value in bothAccepted, meaning the config is okay, andReady, meaning the responsible controller has rolled the config out everywhere it needs to be. The idea for these is to provide a low-noise signal that either "everything is OK, no worries", or "something is wrong, you should check more". - If necessary, negative-polarity conditions that indicate the presence of a specific error (
IPNotAllowed, or similar error conditions.) The Node error conditions are the exemplar here, I think. Also note that providing a good Reason and Message on anAcceptedcondition may be sufficient, and that these negative-polarity conditions should probably start out namespaced to the owning controller until there's some agreement about what's useful.
Having a Ready Condition as well as an Accepted one allows a user to easily distinguish between "My config is busted" and "my config hasn't been deployed yet", without requiring all nodes to update the object. I'd guess that it should be relatively straightforward for the policy controller to have some mechanism for knowing which nodes have the config version, even if that's just polling.
For any required Conditions, I strongly recommend providing string constants for Reason values you want to be well known, and document the cases you expect them to be used as clearly as possible.
However, having these non-namespaced summary Conditions required will make it difficult to impossible for multiple controllers to coordinate. If multiple controllers are really required, all Conditions should be namespaced with the name of the owning controller, and the API definitions can only suggest Type suffixes. This can be done just using the existing Type field (since it's kind of expected that it may contain domain-prefixed strings).
I'd agree with a comment from @aojea that questioned the need to support multiple network policy providers, personally, but my knowledge in this area is pretty out-of-date, so take that with a whole pile of salt.
There was a problem hiding this comment.
I'd agree with a comment from @aojea that questioned the need to support multiple network policy providers, personally, but my knowledge in this area is pretty out-of-date, so take that with a whole pile of salt.
+1 to this one, but again, I'm far from being an expert here.
My main question is - do we have to address that for Alpha? While I think we should resolve that for beta, given that alpha is disabled by default, we can probably start with a single provider support only and reevaluate for beta.
@thockin - thoughts?
There was a problem hiding this comment.
I think, IMO that in this case and as Tim stated before, we can deal with that on the implementation moment rather than right now. We are targeting alpha, so we can change as much as we want right? :)
Thanks folks for your thoughts on this!
|
|
||
| ###### How can a rollout or rollback fail? Can it impact already running workloads? | ||
|
|
||
| A rollout won't cause any problem, as this is a new field. |
There was a problem hiding this comment.
Technically, one can imagine a crashing conversions in kube-apiserver or sth like that...
There was a problem hiding this comment.
sorry @wojtek-t can you explain a bit more on this. It's the only part I'm missing to adjust. What's the scenario? Conversions between old objects not supporting status and new ones?
There was a problem hiding this comment.
Yeah. Or more generically, the registry strategy -related code in kube-apiserver. The risk is low, but non-zero.
There was a problem hiding this comment.
I think the answer is to ACK "There is low, but non-zero, risk that registering a status strategy could have bugs that cause problems even when the gate is disabled."
There was a problem hiding this comment.
Yeah - I would just change to something like:
"Unexpected crashes of kube-apiserver. Running workloads will not be affected".
There was a problem hiding this comment.
@thockin @rikatz @jayunit100 Speaking on behalf of the Antrea project, there is no concern on our side when it comes to implementing this. From a user perspective, a validation webhook is better IMO, but I guess making it part of the API is a better incentive for CNI providers to implement it as opposed to “you should have a validation webhook if you don’t support all features”.
I understand the rationale for prefixing the Type field with a provider namespace, but 1) it seems unlikely to me that there will be multiple controller for NP implementation, 2) I feel like it hurts the ability of users to consume this Status as opposed to having a fixed / unique condition Type (e.g. Accepted) that can be checked to determine whether the NP can be implemented by the provider.
|
+1 |
Yep, I had the same concerns originally @aanm, but am OK with the non-goal as stated. Have pushed back on the "know when my policy in fully implemented" requirement in the past as well for this same reason - it doesn't scale well. My main concern with this at the moment is that I can't see us getting a lot of value from it. Take the endPort example - Calico vA.B.C has support for it, but Calico vX.Y.Z existed prior to the endPort field being added to the API. It simply won't know about the field and won't be able to report back to the API that the policy isn't implemented correctly, and it never will be able to because the code is already released and in the wild. Seems like this only works for when a network policy provider explicitly writes code to check for a specific policy formulation it doesn't allow, which is certainly better than nothing but might provide a false sense of security to users who see "Accepted" on a policy only to find out that the version they are running is just old. TL;DR: /lgtm, has some limitations but better than nothing and worth playing with in alpha. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@wojtek-t As the only missing part is for when targetting beta releases and I don't wanna block on this, your call, should I:
|
|
|
||
| ###### Does enabling the feature change any default behavior? | ||
|
|
||
| No |
There was a problem hiding this comment.
I just realized that it actually should be "yes", because after enabling the feature gate network policy providers will start setting some status.
There was a problem hiding this comment.
They will just if implemented on their side. This is why I thought this was no. The change of a "default behavior" for me would be something like "enabling the feature gate will make all TCP connections deny something".
But let me change here :)
|
|
||
| ###### How can a rollout or rollback fail? Can it impact already running workloads? | ||
|
|
||
| A rollout won't cause any problem, as this is a new field. |
There was a problem hiding this comment.
Yeah - I would just change to something like:
"Unexpected crashes of kube-apiserver. Running workloads will not be affected".
k8s-ci-robot
removed
the
lgtm
|
This is more than enough for PRR for Alpha. We can always tune it further in Beta. /lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aanm, rikatz, thockin, wojtek-t 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
|
It seems that everyone above was supportive, so cancelling hold. /hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
I don't think we need to sweat beta criteria at alpha time. |
Sure - I was just adding comments to them as I was also adding to alpha ones. But I mentioned offline to @rikatz that beta ones wasn't blocking for alpha. |
One-line PR description: Add initial draft on Network Policy Status for discussion
Issue link: NetworkPolicy Status - Features and parsing #2943
Other comments:
This is a follow up on the requirements of Network Policy Status in NetworkPolicy port range #2079 review
/sig network
/assign @thockin @caseydavenport