-
Notifications
You must be signed in to change notification settings - Fork 369
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.
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
Upgrade Suricata to 7.0 #6589
Upgrade Suricata to 7.0 #6589
Conversation
suricata 7.0 seems to have incomptaible changes? |
Yes, that's why I made this a draft. I'm working on it. I will probably not try to backport in the end, because of these issues. Even though Suricata 6 is EOL and won't receive security issues, I think it's ok not to backport given that all the Antrea features which rely on Suricata are Alpha. |
c73f27e
to
c8c4105
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one minor comment
if err := os.Mkdir(tenantRulesDir, 0755); err != nil && !os.IsExist(err) { | ||
klog.ErrorS(err, "Failed to create Suricata rule directory", "directory", tenantRulesDir) | ||
} | ||
// Create log directory /var/log/antrea/networkpolicy/l7engine/ for Suricata. | ||
if err := os.Mkdir(antreaSuricataLogPath, os.ModePerm); err != nil { | ||
klog.ErrorS(err, "Failed to create L7 Network Policy log directory", "Directory", antreaSuricataLogPath) | ||
if err := os.Mkdir(antreaSuricataLogPath, 0755); err != nil && !os.IsExist(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use MkdirAll
in the two places here to make them more robust and avoid ExistErr check, otherwise it would still rely on another function to create /var/log/antrea/networkpolicy
first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I made the change in a follow-up commit. PTAL.
Suricata 6.0 is now end-of-life so we upgrade to 7.0: https://forum.suricata.io/t/suricata-6-is-now-end-of-life-eol/4790 Suricata 7.0 includes support for some new protocols, including HTTP/2 and QUIC, which means we could start supporting these protocols in L7NetworkPolicies. Two changes were required in Antrea for this upgrade: * The /etc/suricata/rules no longer seems to be created by default in the antrea-agent container image, when installing the Suricata package. So we create it if needed when starting Suricata from the Antrea Agent. * The alert events logged by Suricata for our default drop rules no longer include app-layer metadata. This should be expected for an ip rule, and the earlier behavior was probably invalid in a way. See also https://redmine.openinfosecfoundation.org/issues/7199. As a result of this behavioral change, e2e tests as well as L7NP documentation had to be updated. Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
c8c4105
to
444b560
Compare
/test-all |
/test-ipv6-e2e |
/test-kind-ipv6-e2e |
antreaPodName, err := data.getAntreaPodOnNode(l7LoggingNode) | ||
require.NoError(t, err, "Error occurred when trying to get the antrea-agent Pod running on Node %s", l7LoggingNode) | ||
|
||
// Find filename of L7 log file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great, test seems more stable 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Suricata 6.0 is now end-of-life so we upgrade to 7.0: https://forum.suricata.io/t/suricata-6-is-now-end-of-life-eol/4790
Suricata 7.0 includes support for some new protocols, including HTTP/2 and QUIC, which means we could start supporting these protocols in L7NetworkPolicies.
Two changes were required in Antrea for this upgrade: