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.

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

Merged
merged 2 commits into from
Aug 28, 2024
Merged

Conversation

antoninbas
Copy link
Contributor

@antoninbas antoninbas commented Aug 5, 2024

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.

@antoninbas antoninbas added area/dependency Issues or PRs related to dependency changes. action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. labels Aug 5, 2024
@antoninbas antoninbas marked this pull request as draft August 6, 2024 02:50
@tnqn
Copy link
Member

tnqn commented Aug 6, 2024

suricata 7.0 seems to have incomptaible changes?

@antoninbas
Copy link
Contributor Author

antoninbas commented Aug 6, 2024

suricata 7.0 seems to have incomptaible changes?

Yes, that's why I made this a draft. I'm working on it.
One of the issue is basic (the /etc/suricata/rules directory is no longer created by default), but another seems more like a bug to me: https://redmine.openinfosecfoundation.org/issues/7199. The second one only affects logging.

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.

@antoninbas antoninbas marked this pull request as ready for review August 27, 2024 00:44
tnqn
tnqn previously approved these changes Aug 27, 2024
Copy link
Member

@tnqn tnqn left a 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

Comment on lines 513 to 517
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) {
Copy link
Member

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.

Copy link
Contributor Author

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 <[email protected]>
Signed-off-by: Antonin Bas <[email protected]>
@antoninbas
Copy link
Contributor Author

/test-all

@antoninbas
Copy link
Contributor Author

/test-ipv6-e2e
/test-ipv6-only-e2e

@antoninbas
Copy link
Contributor Author

/test-kind-ipv6-e2e
/test-kind-ipv6-only-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.
Copy link
Contributor

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 🎉

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@antoninbas antoninbas merged commit e6010fb into antrea-io:main Aug 28, 2024
54 of 61 checks passed
@antoninbas antoninbas deleted the upgrade-to-suricata-7 branch August 28, 2024 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/release-note Indicates a PR that should be included in release notes. area/dependency Issues or PRs related to dependency changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants