Add CNP IngressDeny and EgressDeny rules validation #36598
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
b6000a4 to
6fa6c7d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
6fa6c7d to
7a679a5
Compare
In preparation of the subsequent commits, rework the errors usage in rules sanitization. Specifically: - fix some linting errors (capitalized error strings, punctuation in error strings, useless usage of error formatting and so on) - use errors.Join to propagate errUnsupportedICMPWithToPorts alongside other possible validation errors - fix tests to rely on require.ErrorIs when checking for a specific error Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
Extract IngressCommonRule sanitization into its own method. This will be useful in a subsequent commit, where the sanitization for IngressDeny rules will be added. Since IngressCommonRule is embedded into both Ingress and IngressDeny rules, the new method can be used while sanitizing both rule types. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
Extract EgressCommonRule sanitization into its own method. This will be useful in a subsequent commit, where the sanitization for EgressDeny rules will be added. Since EgressCommonRule is embedded into both Egress and EgressDeny rules, the new method can be used while sanitizing both rule types. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
Add sanitization for IngressDeny rules. The sanitization applies the same logic used for Ingress rules, dropping the parts that are not relevant for the IngressDeny ones (e.g: IngressDeny has no L7 rules). Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
Add sanitization for EgressDeny rules. The sanitization applies the same logic used for Egress rules, dropping the parts that are not relevant for the EgressDeny ones (e.g: EgressDeny has no L7 rules). Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
Add helper methods to {Egress,EgressDeny,EgressCommon}Rule to get the
list of L3 members and the list of L3 members that are not compatible
with ToPorts rules.
This simplifies the extension and reuse of the sanitization logic in
EgressCommonRule to EgressRule and EgressDenyRule.
Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
Extend the current unit tests to take into account the sanitization of both IngressDeny and EgressDeny rules. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
7a679a5 to
6fbcc8a
Compare
|
/test |
|
@pippolo84 this PR would fix a bug I'm seeing in v1.16, do you think it should be backported? |
Since this can help to fix a bug, I'd say it qualifies for backporting. I'm (mildly) concerned that adding an additional layer of validation into v1.16 might break existing policies, but given that those policies wouldn't have worked as intended anyway (see the segfault you observed) I think backporting is the way to go. Let me take care of that 👍 |
45 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add sanitization for IngressDeny and EgressDeny rules in CNP/CCNP.
The sanitization logic follows the same rules already applied for Ingress and Egress rules, except for the parts that are not supported in their "Deny" counterparts (e.g: no L7 rules support in IngressDeny/EgressDeny, hence no L7 rules validation).
Note to reviewers: please review commit by commit