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

[v7r3] added a flag for disabling the use of SecurityLogging service #5760

Merged
merged 5 commits into from
Jan 27, 2022

Conversation

fstagni
Copy link
Contributor

@fstagni fstagni commented Jan 5, 2022

See https://gitlab.cern.ch/ai/it-puppet-module-dirac/-/merge_requests/75

BEGINRELEASENOTES

*Framework
CHANGE: added a flag for disabling the use of SecurityLogging service

ENDRELEASENOTES

@DIRACGridBot DIRACGridBot added the alsoTargeting:integration Cherry pick this PR to integration after merge label Jan 5, 2022
@fstagni
Copy link
Contributor Author

fstagni commented Jan 5, 2022

Seems like there's a psutil bug: giampaolo/psutil#2048

@fstagni fstagni marked this pull request as draft January 6, 2022 13:34
@fstagni
Copy link
Contributor Author

fstagni commented Jan 6, 2022

54b6db3 is very much subject to review from @chaen @TaykYoku

Copy link
Contributor

@TaykYoku TaykYoku left a comment

Choose a reason for hiding this comment

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

SecurityLogClient uses DIRAC.Core.Utilities.ThreadScheduler for reporting.
Since we are moving from DISET to Tornado, it is worth using tornado.web.IOLoop to run periodic tasks. I think the following PR will be useful for this implementation.

src/DIRAC/Core/DISET/private/Service.py Show resolved Hide resolved
src/DIRAC/Core/Tornado/Server/TornadoService.py Outdated Show resolved Hide resolved
src/DIRAC/Core/Tornado/Server/TornadoService.py Outdated Show resolved Hide resolved
src/DIRAC/Core/Tornado/Server/TornadoService.py Outdated Show resolved Hide resolved
src/DIRAC/Core/Tornado/Server/TornadoService.py Outdated Show resolved Hide resolved
@chaen
Copy link
Contributor

chaen commented Jan 21, 2022

Given that we want to ditch it, I really do not think it is worth the pain of adding the securityLog to TornadoService

@fstagni
Copy link
Contributor Author

fstagni commented Jan 24, 2022

Given that we want to ditch it, I really do not think it is worth the pain of adding the securityLog to TornadoService

Yes, we want to ditch the securityLog, but ATM it's still needed. In order to ditch it, we need in fact to store on ES 6 months worth of log data, for each of the running DIRAC services. This is not impossible but rather expensive, considering that ATM we are correctly sending to ES everything that we put in verbose level. What is stored on the current securityLog is a rather minimal subset of that. So, ditching "tomorrow" the current securityLog service is not possible.

What we can do is to use again ES, in combination with logstash: meaning that we create ES indices with the content of what would normally go in the CSV files created by SecurityLog services, driven by a logstash rule. Would this work?

@chaen
Copy link
Contributor

chaen commented Jan 25, 2022

yes it would work. But I still would not add it to Tornado now

@fstagni fstagni marked this pull request as ready for review January 25, 2022 18:17
@fstagni
Copy link
Contributor Author

fstagni commented Jan 25, 2022

I have removed the changes to tornado, and added changes for logstash configuration. Please review this PR, and the one for puppet/logstash. If you are happy we can try this in the coming hackathon.

@@ -1,7 +1,8 @@
Systems / Framework / <INSTANCE> / Service / SecurityLogging - Sub-subsection
=============================================================================

SecurityLogging service is used by all server to log all connections.
SecurityLogging service can be used by all services to log all connections, for security-related purpose.
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, the ConfReference is going away

@chaen
Copy link
Contributor

chaen commented Jan 27, 2022

The code and puppet code seems fine. Just you need to add the option doc to the dirac.cfg instead of the ConfReference

chaen
chaen previously approved these changes Jan 27, 2022
@fstagni fstagni merged commit a0651b3 into DIRACGrid:rel-v7r3 Jan 27, 2022
@DIRACGridBot DIRACGridBot added sweep:done All sweeping actions have been done for this PR sweep:failed Sweeping failed and needs manual intervention labels Jan 27, 2022
@DIRACGridBot
Copy link

Sweep summary

Sweep ran in https://github.com/DIRACGrid/DIRAC/actions/runs/1755279881

Failed:

  • integration
    cherry-pick a0651b3 into integration failed
    check merge conflicts on a local copy of this repository
    git fetch upstream
    git checkout upstream/integration -b cherry-pick-2-a0651b3e9-integration
    git cherry-pick -x -m 1 a0651b3e9
    # Fix the conflicts
    git cherry-pick --continue
    git commit --amend -m 'sweep: #5760 added a flag for disabling the use of SecurityLogging service' --author='fstagni <[email protected]>'
    git push -u origin cherry-pick-2-a0651b3e9-integration
    
    # If you have the GitHub CLI installed the PR can be made with
    gh pr create \
         --label 'sweep:from rel-v7r3' \
         --base integration \
         --repo DIRACGrid/DIRAC \
         --title '[sweep:integration] added a flag for disabling the use of SecurityLogging service' \
         --body 'Sweep #5760 `added a flag for disabling the use of SecurityLogging service` to `integration`.
    
    Adding original author @fstagni as watcher.
    
    BEGINRELEASENOTES
    
    *Framework
    CHANGE: added a flag for disabling the use of SecurityLogging service
    
    ENDRELEASENOTES
    Closes #1115939097'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alsoTargeting:integration Cherry pick this PR to integration after merge sweep:done All sweeping actions have been done for this PR sweep:failed Sweeping failed and needs manual intervention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants