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

enhancement: enable icap preview mode and fix client according to the spec #8062

Merged
merged 8 commits into from
Jan 5, 2024

Conversation

fschade
Copy link
Contributor

@fschade fschade commented Dec 22, 2023

Description

the used icap lib is not active maintained and we've spotted some bugs:

  • TCP socket keepalive
  • broken preview requests

both of the above topics are handled in my fork but needs confirmation and polishing before sending the changes upstream.

The implementation for preview requests follows the rfc 3507 (0 DoubleCRLF) and works (at least in my test-cases) now as expected.

Making use of the preview mechanism could drastically speed up virus detection and therefore improves the end user experience and by reducing the waiting time and on also reduces the network traffic (if a virus is detected within the firs N bytes).

The implementation still needs some more options later on, for now i just wanna wait for confirmation that the bug reported via #6764 is fixed, Once that is done, i start polishing.

The preview requests are enabled by default in this PR, no change needed.
To see the benefit of TCP connection keepalive, changes in the c-icap are needed, e.g.: KeepAliveTimeout 600

Related Issue

Motivation and Context

speedup virus scanning and improve end user experience

How Has This Been Tested?

  • locally only, needs confirmation by devops team

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Copy link

update-docs bot commented Dec 22, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

Copy link
Collaborator

@kobergj kobergj left a comment

Choose a reason for hiding this comment

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

👍 LGTM

Just some remark for future improvement...

services/antivirus/pkg/service/service.go Outdated Show resolved Hide resolved
@fschade
Copy link
Contributor Author

fschade commented Jan 4, 2024

i updated the client lib, can be found here: fschade/icap-client#1

@fschade
Copy link
Contributor Author

fschade commented Jan 5, 2024

@mmattel, please see the message from jfd regarding the deprecation.
It contains a new env var for the timeout, e.g. ANTIVIRUS_ICAP_SCAN_TIMEOUT

@fschade
Copy link
Contributor Author

fschade commented Jan 5, 2024

the client lib is in a good shape now, still, we could (and should) invest more time integrating a protocol layer or similar.

Can be done later!

we have to decide if we backport it or not, my personal opinion, 4.0, master and 5.0 should contain the change.

i tested (locally) every known case, works for me! cc.: @wkloucek. please confirm if it solves your icap problems.

@fschade fschade marked this pull request as ready for review January 5, 2024 15:36
@mmattel
Copy link
Contributor

mmattel commented Jan 5, 2024

Adding @ScharfViktor as this is testing relevant
We also need to add a upgrade note into the upgrade section of the admin docs.

@fschade
Copy link
Contributor Author

fschade commented Jan 5, 2024

thanks martin for taking care :)

@mmattel
Copy link
Contributor

mmattel commented Jan 5, 2024

@fschade we need a changelog!

Copy link
Contributor

@mmattel mmattel left a comment

Choose a reason for hiding this comment

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

LGTM from a docs pov 👍

Copy link

sonarcloud bot commented Jan 5, 2024

@@ -34,5 +36,10 @@ func ParseConfig(cfg *config.Config) error {

// Validate validates our little config
func Validate(cfg *config.Config) error {
if cfg.Scanner.ICAP.DeprecatedTimeout != 0 {
cfg.Scanner.ICAP.Timeout = time.Duration(cfg.Scanner.ICAP.DeprecatedTimeout) * time.Second
fmt.Println("ANTIVIRUS_ICAP_TIMEOUT is deprecated, use ANTIVIRUS_ICAP_SCAN_TIMEOUT instead")
Copy link
Member

Choose a reason for hiding this comment

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

I see what you did there ❤️

@butonic butonic merged commit ac8676f into owncloud:master Jan 5, 2024
3 checks passed
ownclouders pushed a commit that referenced this pull request Jan 5, 2024
… spec (#8062)

* enhancement: enable icap preview mode and use a forked icap client which fixes tcp socket keepalive

* enhancement: make use of human time for the icap timout config option

* enhancement: update icap-client

* enhancement: bump icap client library and deprecate ANTIVIRUS_ICAP_TIMEOUT env

* chore: vendor icap library

* enhancement: set preview size only if greater than 0

* Update services/antivirus/pkg/config/config.go

Co-authored-by: Martin <[email protected]>

* enhancement: add changelog

---------

Co-authored-by: Martin <[email protected]>
2403905 pushed a commit to 2403905/ocis that referenced this pull request Jan 24, 2024
… spec (owncloud#8062)

* enhancement: enable icap preview mode and use a forked icap client which fixes tcp socket keepalive

* enhancement: make use of human time for the icap timout config option

* enhancement: update icap-client

* enhancement: bump icap client library and deprecate ANTIVIRUS_ICAP_TIMEOUT env

* chore: vendor icap library

* enhancement: set preview size only if greater than 0

* Update services/antivirus/pkg/config/config.go

Co-authored-by: Martin <[email protected]>

* enhancement: add changelog

---------

Co-authored-by: Martin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Antivirus relies on connection close to get ICAP result
4 participants