[CMPT-2133] [1.13] Add EnableDefaultDeny field to Network Policy #551
Conversation
This adds a new field, EnableDefaultDeny, to CiliumNetworkPolicy and CiliumClusterwideNetworkPolicy, that controls whether or not the subject endpoints of this policy should drop unselected peer traffic. By default, endpoints are in a default-allow mode. When the first policy applies to an endpoint, it flips it to a default-deny mode. This option allows disabling that behavior. If not specified, the existing behavior remains: for each direction, traffic is allowed unless at least one policy rule applies to that endpoint. If multiple policies select an endpoint, then default-deny takes precedence. It is useful in heterogeneous environments, it may not be desirable to implicitly drop all non-matching traffic. Consider, for example, the case where an administrator wishes to ensure a monitoring service can access all namespaces. If they create a cluster-wide policy allowing access from the monitoring service, it may create a deny policy where none was previously; unexpectedly dropping traffic. See: https://github.com/cilium/design-cfps/blob/main/cilium/CFP-30572-non-default-deny-policies.md This commit contains only the API changes; a subsequent commit will introduce the implementation. Signed-off-by: Casey Callendrello <cdc@isovalent.com>
Methods called by Sanitize() may alter the underlying structures. For example. selectors are aggregated and address families are upper-cased. However, top-level fields can't be written by Sanitize. In the future, we'd like to do that. So, give it a pointer receiver. Signed-off-by: Casey Callendrello <cdc@isovalent.com>
This adjust the policy rule generation to take in to account non-default-deny policies. As before, an endpoint is normally in a policy-disabled state. If any policies select this endpoint, then policy is enabled and all non-allowed traffic is dropped. If, however, an endpoint is only selected by default-allow policies, then policy is enabled, but a special wildcard allow policy is inserted. Since wildcard polcies have very low precedence, this ensures that any Deny or L7-proxy rules will still take effect. This commit also fixes tests that incorrectly failed to sanitize rules before adding to the policy repository, leading to a nil pointer exception. Production code *always* sanitizes rules before adding. Signed-off-by: Casey Callendrello <cdc@isovalent.com>
| @@ -23,7 +23,7 @@ const ( | |||
| // | |||
| // Maintainers: Run ./Documentation/check-crd-compat-table.sh for each release | |||
| // Developers: Bump patch for each change in the CRD schema. | |||
| CustomResourceDefinitionSchemaVersion = "1.26.7" | |||
| CustomResourceDefinitionSchemaVersion = "1.26.8" | |||
There was a problem hiding this comment.
Should we use a numbering scheme that is orthogonal to upstream here? E.g. "1.26.7-dd0"?
If not is there a risk that we deploy the true upstream 1.26.8 at a later date and end up with a conflict of some form?
There was a problem hiding this comment.
aren't they going to bump the version if it gets changed in main anyway? 🤔
There was a problem hiding this comment.
I actually suspect it's an oversight in the original PR that they didn't bump the CRD schema version because I don't see how the changes would get picked up otherwise.
And I'm not sure the -dd suffix would change anything here because we need this semver check to be true for the Operator to upgrade the CRDs in the cluster:
And I confirmed that this check ignores suffixes:
func (vc *VersionCheckTestSuite) TestVersion(c *C) {
ver1, _ := Version("1.26.7")
ver2, _ := Version("1.26.7-dd0")
c.Assert(ver1.LT(ver2), Equals, true)
}c.Assert(ver1.LT(ver2), Equals, true)
Expected :bool = true
Actual :bool = false
check_test.go:24
OOPS: 0 passed, 1 FAILED
--- FAIL: Test (0.00s)
So we'll have to update this check as well if we want to go this route (we'll need to make sure the check looks at clusterVersion.Pre field and that the -dd suffix is allowed in versions here)
There was a problem hiding this comment.
aren't they going to bump the version if it gets changed in main anyway?
Well that's why I wondered if our bumping might conflict with theirs.
LT() and 1.26.7 vs 1.26.7-dd0
Probably what you'd want then is to upgrade to 1.26.8-dd0 here then. But if we think this isn't necessary, that's fine too.
There was a problem hiding this comment.
Yeah I can do 1.26.8-dd0 but it won't change anything in practice because 1.26.8-dd0 is treated exactly the same as 1.26.8 in the existing logic (we'll have to change the comparison functions if we want to make it work)
There was a problem hiding this comment.
oh indeed my bad, I did not realize the version bump was not part of their change!
There was a problem hiding this comment.
I actually suspect it's an oversight in the original PR that they didn't bump the CRD schema
This is expected. IIRC they bump it only when they cut a new release to coalesce different changes into one bump.
Backport cilium#30572 for our fork