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.

Sign up for GitHub

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

Jump to bottom

Add Security Logging Plugin. #1125

Merged
merged 25 commits into from
Apr 14, 2020
Merged

Add Security Logging Plugin. #1125

merged 25 commits into from
Apr 14, 2020

Conversation

artivis
Copy link
Contributor

@artivis artivis commented Apr 2, 2020 •
edited
Loading

Add the Security Logging Plugin.

Currently the plugin logs only to files but ready for a first round of review.

Todo:

Related PR:

Note:

  • logging over-DDS will be implemented in a subsequent pull request

Summary:

  • add security logging to file as per dds-spec
  • add set/get logger to security plugin base classes
  • logging integration to SecurityManager
  • add SECURITY_LOGGING macros
  • logging macro show-case

Signed-off-by: artivis jeremie.deray@canonical.com

artivis and others added 2 commits April 2, 2020 15:31
@artivis
Add security logging (#1)
fa9250c 
* add security logging to file as per dds-spec
* add set/get logger to security plugin base classes
* logging integration to SecurityManager
* add SECURITY_LOGGING macros

Signed-off-by: artivis <jeremie.deray@canonical.com>
@artivis
Merge branch 'master' into feature/security-logging
897dff2 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@artivis
handle empty filename
79ee043 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@artivis
post-master-merge fix
5a4c971 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@artivis
handle empty filename
c31bbe7 
Signed-off-by: artivis <jeremie.deray@canonical.com>
This was referenced Apr 2, 2020
Closed
Closed
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@artivis
fix typo test filename
04dd0de 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@artivis
some doc fixes
9ffcf08 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@artivis
some doc fixes
6266390 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@artivis
Copy link
Contributor Author

artivis commented Apr 6, 2020 •
edited
Loading

Hi @richiware (@richiprosima),
I'm not sure why is this PR not compiling in CI whereas it does on my machine. The error is a bit unexpected. Do you have any clue of what's going on and/or how I could reproduce that?
Thanks in advance.

Edit: After setting up a new vm with the correct google-mock version I can reproduce this issue. On it 👍.

@artivis
support logging in SecurityPluginFactory Mock
5bd340e 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@artivis
include log in security logging for macro support & cleanup
21c0a10 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@artivis
add missing source
271c262 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

richiware
richiware requested changes Apr 7, 2020
View reviewed changes
Copy link
Member

@richiware richiware left a comment

Choose a reason for hiding this comment

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

Greate job!! There are some coding-style issues. Maybe you might use this uncrustify config file.

include/fastdds/rtps/security/logging/LogOptions.h Outdated

#include <string>

#include "fastdds/rtps/security/logging/LoggingLevel.h"
Copy link
Member

Choose a reason for hiding this comment

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

why use " " used for relative paths instead of < >? Do you mind to change non-relative includes to < >? This change could improve compilation time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 5e099c1

@artivis
replace 'library' includes with 'system'
5e099c1 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@artivis
Copy link
Contributor Author

artivis commented Apr 7, 2020 •
edited
Loading

Greate job!! There are some coding-style issues. Maybe you might use this uncrustify config file.

Thanks @richiware. The code style is not homogeneous throughout the project. If I apply the config you provide only to my changes, some files may end up with two different styles. On the other hand applying it globally to the files I edited will result in unnecessary long diff. Do you want me to apply the conf only to my changes and have some files with two styles or only where it seems to make sense ?

Edit:
I applied the uncrustify config throughout my changes with a couple exceptions to avoid having two too different styles in a single file. Please let me know if that works for you.

@artivis
uncrustify
eeb656a 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@artivis
Copy link
Contributor Author

artivis commented Apr 7, 2020

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

The test failures seem to be only related to timeouts. If anyone can confirm?
Despite this, I would rate this PR as ready for first reviews.

@richiware richiware self-requested a review April 7, 2020 20:33
richiware
richiware requested changes Apr 7, 2020
View reviewed changes
Copy link
Member

@richiware richiware left a comment

Choose a reason for hiding this comment

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

Could you address the warnings on Mac and Windows?

@artivis
add log to file test
64b0d16 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@artivis
fix warnings
98fa09d 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@artivis
fix error msg typo
b5ff52e 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@kyrofa kyrofa mentioned this pull request Apr 7, 2020
Merged
richiware
richiware requested changes Apr 8, 2020
View reviewed changes
Copy link
Member

@richiware richiware left a comment

Choose a reason for hiding this comment

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

There is a linkage error

@artivis
fix linking error
f600b44 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@artivis
fix warning
0d5b9b5 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@artivis
add verbosity level to file logs
430a0c2 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@artivis
test file logs header
8f092fc 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

richiware
richiware previously approved these changes Apr 10, 2020
View reviewed changes
@richiware
Copy link
Member

This PR is ready to be merged. It only needs to be fixed the failure with DCO plugin.

@artivis
use logging in Permission
0598c11 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@artivis
use logging in PKIDH
14922df 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@artivis
use logging in SecurityManager
ed23fdd 
Signed-off-by: artivis <jeremie.deray@canonical.com>
@richiware
Copy link
Member

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@MiguelCompany
Copy link
Member

@richiware I manually accepted the DCO, as the offending commits were clearly from @artivis . This is ready to be merged

@richiware
Copy link
Member

@artivis Could you confirm it is ready for merging? or is something still pending?

@artivis
Copy link
Contributor Author

artivis commented Apr 14, 2020

@richiware this PR is ready indeed 👍.

@richiware richiware merged commit 8492d5a into eProsima:master Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richiware richiware richiware approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@artivis @richiware @MiguelCompany