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

Http mixin default http header overwrites bot configuration #2137

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Narzhan
Copy link
Contributor

@Narzhan Narzhan commented Jan 12, 2022

Description

The default value for http headers was present also in the constructor of the mixin class effectively overwriting the bot http header configuration if any was supplied. This should be aligned with the 3.x design pattern.

Update: I found issue with the user agent not being replaced in the http header since the section was unreachable. I've added this commit to this pr since I think that creating a new one wouldn't be needed.

Copy link
Member

@sebix sebix left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, but unfortunately this is a bit a can of worms. While most of the HTTP Mixin works perfectly fine, there's an unclear behaviour regarding the http_header variable which is the reason why it's re-set in the constructor (see the test fail https://github.com/certtools/intelmq/runs/4790117560?check_suite_focus=true#step:9:118).
A few months ago (still at cert.at), we tried to debug that but came to no conclusion, only this workaround. But as you now have found, that creates another bug :/
I fear there's a flaw elsewhere causing that.

The is not None changes look good at first sight.

@sebix sebix added bug Indicates an unexpected problem or unintended behavior component: core labels Jan 12, 2022
@Narzhan
Copy link
Contributor Author

Narzhan commented Jan 13, 2022

Understood. However there should either be some reliable way to construct a http requests or this should be kept the generic http collector (with the proper handling of all configuration). I think I'd be okay with both of those options.

Anyway these changes fixed the two issues I was facing so I'll leave it up to you whether you want this to be merged or not.

@sebix
Copy link
Member

sebix commented Jan 13, 2022

However there should either be some reliable way to construct a http requests or this should be kept the generic http collector (with the proper handling of all configuration).

I completely agree. However, as I am no longer working at CERT.at my work on IntelMQ is also not funded any more. I fear this is left unsolved unless someone steps up with either time or budget to fix this bug.

@sebix sebix added the help wanted Indicates that a maintainer wants help on an issue or pull request label Feb 3, 2022
waldbauer-certat added a commit that referenced this pull request Feb 4, 2022
**General**
Removed 'requests' MissingDependencyError, because requests is a core lib from intelmq
Removed HTTP variables from Bot class in favor of HttpMixin
Removed trying to import requests in pipeline, its a core lib from intelmq
Added additional configuration variables to HttpMixin ( from Bot class )

**Bots**
GitHub API is now using HttpMixin
MS Azure Collector is now using HttpMixin
DO-Portal Expert is now using HttpMixin
GeoHash using MissingDependencyError instead of ValueError (consistency)
HttpContentExpert is now using HttpMixin
HttpStatusExpert is now using HttpMixin
NationalCERTContactCertATExpert is now using HttpMixin
RDAPExpert is now using HttpMixin
RIPEExpert is now using HttpMixin
SplunkSavedSearchExpert is now using HttpMixin
TuencyExpert is now using HttpMixin
RestAPIOutput is now using HttpMixin

**Bot tests**
GitHub API Collector is now using requests_mock instead of MagicMock (consistency)
RestAPI Output is now using correct headers

Fixes #2150
Fixes #2137

Signed-off-by: Sebastian Waldbauer <[email protected]>
waldbauer-certat added a commit that referenced this pull request Feb 4, 2022
**General**
Removed 'requests' MissingDependencyError, because requests is a core lib from intelmq
Removed HTTP variables from Bot class in favor of HttpMixin
Removed trying to import requests in pipeline, its a core lib from intelmq
Added additional configuration variables to HttpMixin ( from Bot class )

**Bots**
GitHub API is now using HttpMixin
MS Azure Collector is now using HttpMixin
DO-Portal Expert is now using HttpMixin
GeoHash using MissingDependencyError instead of ValueError (consistency)
HttpContentExpert is now using HttpMixin
HttpStatusExpert is now using HttpMixin
NationalCERTContactCertATExpert is now using HttpMixin
RDAPExpert is now using HttpMixin
RIPEExpert is now using HttpMixin
SplunkSavedSearchExpert is now using HttpMixin
TuencyExpert is now using HttpMixin
RestAPIOutput is now using HttpMixin

**Bot tests**
GitHub API Collector is now using requests_mock instead of MagicMock (consistency)
RestAPI Output is now using correct headers

Fixes #2150
Fixes #2137

Signed-off-by: Sebastian Waldbauer <[email protected]>
waldbauer-certat added a commit that referenced this pull request Feb 4, 2022
**General**
Removed 'requests' MissingDependencyError, because requests is a core lib from intelmq
Removed HTTP variables from Bot class in favor of HttpMixin
Removed trying to import requests in pipeline, its a core lib from intelmq
Added additional configuration variables to HttpMixin ( from Bot class )

**Bots**
GitHub API is now using HttpMixin
MS Azure Collector is now using HttpMixin
DO-Portal Expert is now using HttpMixin
GeoHash using MissingDependencyError instead of ValueError (consistency)
HttpContentExpert is now using HttpMixin
HttpStatusExpert is now using HttpMixin
NationalCERTContactCertATExpert is now using HttpMixin
RDAPExpert is now using HttpMixin
RIPEExpert is now using HttpMixin
SplunkSavedSearchExpert is now using HttpMixin
TuencyExpert is now using HttpMixin
RestAPIOutput is now using HttpMixin

**Bot tests**
GitHub API Collector is now using requests_mock instead of MagicMock (consistency)
RestAPI Output is now using correct headers

Fixes #2150
Fixes #2137

Signed-off-by: Sebastian Waldbauer <[email protected]>
waldbauer-certat added a commit that referenced this pull request Feb 4, 2022
**General**
Removed 'requests' MissingDependencyError, because requests is a core lib from intelmq
Removed HTTP variables from Bot class in favor of HttpMixin
Removed trying to import requests in pipeline, its a core lib from intelmq
Added additional configuration variables to HttpMixin ( from Bot class )

**Bots**
GitHub API is now using HttpMixin
MS Azure Collector is now using HttpMixin
DO-Portal Expert is now using HttpMixin
GeoHash using MissingDependencyError instead of ValueError (consistency)
HttpContentExpert is now using HttpMixin
HttpStatusExpert is now using HttpMixin
NationalCERTContactCertATExpert is now using HttpMixin
RDAPExpert is now using HttpMixin
RIPEExpert is now using HttpMixin
SplunkSavedSearchExpert is now using HttpMixin
TuencyExpert is now using HttpMixin
RestAPIOutput is now using HttpMixin

**Bot tests**
GitHub API Collector is now using requests_mock instead of MagicMock (consistency)
RestAPI Output is now using correct headers

Fixes #2150
Fixes #2137

Signed-off-by: Sebastian Waldbauer <[email protected]>
waldbauer-certat added a commit that referenced this pull request Feb 4, 2022
**General**
Removed 'requests' MissingDependencyError, because requests is a core lib from intelmq
Removed HTTP variables from Bot class in favor of HttpMixin
Removed trying to import requests in pipeline, its a core lib from intelmq
Added additional configuration variables to HttpMixin ( from Bot class )

**Bots**
GitHub API is now using HttpMixin
MS Azure Collector is now using HttpMixin
DO-Portal Expert is now using HttpMixin
GeoHash using MissingDependencyError instead of ValueError (consistency)
HttpContentExpert is now using HttpMixin
HttpStatusExpert is now using HttpMixin
NationalCERTContactCertATExpert is now using HttpMixin
RDAPExpert is now using HttpMixin
RIPEExpert is now using HttpMixin
SplunkSavedSearchExpert is now using HttpMixin
TuencyExpert is now using HttpMixin
RestAPIOutput is now using HttpMixin

**Bot tests**
GitHub API Collector is now using requests_mock instead of MagicMock (consistency)
RestAPI Output is now using correct headers

Fixes #2150
Fixes #2137

Signed-off-by: Sebastian Waldbauer <[email protected]>
waldbauer-certat added a commit that referenced this pull request Feb 4, 2022
**General**
Removed 'requests' MissingDependencyError, because requests is a core lib from intelmq
Removed HTTP variables from Bot class in favor of HttpMixin
Removed trying to import requests in pipeline, its a core lib from intelmq
Added additional configuration variables to HttpMixin ( from Bot class )

**Bots**
GitHub API is now using HttpMixin
MS Azure Collector is now using HttpMixin
DO-Portal Expert is now using HttpMixin
GeoHash using MissingDependencyError instead of ValueError (consistency)
HttpContentExpert is now using HttpMixin
HttpStatusExpert is now using HttpMixin
NationalCERTContactCertATExpert is now using HttpMixin
RDAPExpert is now using HttpMixin
RIPEExpert is now using HttpMixin
SplunkSavedSearchExpert is now using HttpMixin
TuencyExpert is now using HttpMixin
RestAPIOutput is now using HttpMixin

**Bot tests**
GitHub API Collector is now using requests_mock instead of MagicMock (consistency)
RestAPI Output is now using correct headers

Fixes #2150
Fixes #2137

Signed-off-by: Sebastian Waldbauer <[email protected]>
waldbauer-certat added a commit that referenced this pull request Feb 7, 2022
**General**
Removed 'requests' MissingDependencyError, because requests is a core lib from intelmq
Removed HTTP variables from Bot class in favor of HttpMixin
Removed trying to import requests in pipeline, its a core lib from intelmq
Added additional configuration variables to HttpMixin ( from Bot class )

**Bots**
GitHub API is now using HttpMixin
MS Azure Collector is now using HttpMixin
DO-Portal Expert is now using HttpMixin
GeoHash using MissingDependencyError instead of ValueError (consistency)
HttpContentExpert is now using HttpMixin
HttpStatusExpert is now using HttpMixin
NationalCERTContactCertATExpert is now using HttpMixin
RDAPExpert is now using HttpMixin
RIPEExpert is now using HttpMixin
SplunkSavedSearchExpert is now using HttpMixin
TuencyExpert is now using HttpMixin
RestAPIOutput is now using HttpMixin

**Bot tests**
GitHub API Collector is now using requests_mock instead of MagicMock (consistency)
RestAPI Output is now using correct headers

Fixes #2150
Fixes #2137

Signed-off-by: Sebastian Waldbauer <[email protected]>
waldbauer-certat added a commit that referenced this pull request Feb 7, 2022
**General**
Removed 'requests' MissingDependencyError, because requests is a core lib from intelmq
Removed HTTP variables from Bot class in favor of HttpMixin
Removed trying to import requests in pipeline, its a core lib from intelmq
Added additional configuration variables to HttpMixin ( from Bot class )

**Bots**
GitHub API is now using HttpMixin
MS Azure Collector is now using HttpMixin
DO-Portal Expert is now using HttpMixin
GeoHash using MissingDependencyError instead of ValueError (consistency)
HttpContentExpert is now using HttpMixin
HttpStatusExpert is now using HttpMixin
NationalCERTContactCertATExpert is now using HttpMixin
RDAPExpert is now using HttpMixin
RIPEExpert is now using HttpMixin
SplunkSavedSearchExpert is now using HttpMixin
TuencyExpert is now using HttpMixin
RestAPIOutput is now using HttpMixin

**Bot tests**
GitHub API Collector is now using requests_mock instead of MagicMock (consistency)
RestAPI Output is now using correct headers

Fixes #2150
Fixes #2137

Signed-off-by: Sebastian Waldbauer <[email protected]>
waldbauer-certat added a commit that referenced this pull request Feb 7, 2022
**General**
Removed 'requests' MissingDependencyError, because requests is a core lib from intelmq
Removed HTTP variables from Bot class in favor of HttpMixin
Removed trying to import requests in pipeline, its a core lib from intelmq
Added additional configuration variables to HttpMixin ( from Bot class )

**Bots**
GitHub API is now using HttpMixin
MS Azure Collector is now using HttpMixin
DO-Portal Expert is now using HttpMixin
GeoHash using MissingDependencyError instead of ValueError (consistency)
HttpContentExpert is now using HttpMixin
HttpStatusExpert is now using HttpMixin
NationalCERTContactCertATExpert is now using HttpMixin
RDAPExpert is now using HttpMixin
RIPEExpert is now using HttpMixin
SplunkSavedSearchExpert is now using HttpMixin
TuencyExpert is now using HttpMixin
RestAPIOutput is now using HttpMixin

**Bot tests**
GitHub API Collector is now using requests_mock instead of MagicMock (consistency)
RestAPI Output is now using correct headers

Fixes #2150
Fixes #2137

Signed-off-by: Sebastian Waldbauer <[email protected]>
waldbauer-certat added a commit that referenced this pull request Feb 7, 2022
**General**
Removed 'requests' MissingDependencyError, because requests is a core lib from intelmq
Removed HTTP variables from Bot class in favor of HttpMixin
Removed trying to import requests in pipeline, its a core lib from intelmq
Added additional configuration variables to HttpMixin ( from Bot class )

**Bots**
GitHub API is now using HttpMixin
MS Azure Collector is now using HttpMixin
DO-Portal Expert is now using HttpMixin
GeoHash using MissingDependencyError instead of ValueError (consistency)
HttpContentExpert is now using HttpMixin
HttpStatusExpert is now using HttpMixin
NationalCERTContactCertATExpert is now using HttpMixin
RDAPExpert is now using HttpMixin
RIPEExpert is now using HttpMixin
SplunkSavedSearchExpert is now using HttpMixin
TuencyExpert is now using HttpMixin
RestAPIOutput is now using HttpMixin

**Bot tests**
GitHub API Collector is now using requests_mock instead of MagicMock (consistency)
RestAPI Output is now using correct headers

Fixes #2150
Fixes #2137

Signed-off-by: Sebastian Waldbauer <[email protected]>
@sebix
Copy link
Member

sebix commented Jul 25, 2022

@waldbauer-certat I think you are more experienced with the HTTP Mixin than me (#2151), can you take charge of this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior component: core help wanted Indicates that a maintainer wants help on an issue or pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants