Skip to content

Symlink CA certs instead of copy#176

Merged
jacobperron merged 5 commits intomasterfrom
ruffsl/symlink
Mar 3, 2020
Merged

Symlink CA certs instead of copy#176
jacobperron merged 5 commits intomasterfrom
ruffsl/symlink

Conversation

@ruffsl
Copy link
Member

@ruffsl ruffsl commented Feb 22, 2020

This was a previous approach used with Keymint to prevent the keystore from falling out of sync with itself when renewing CA certs, as well as disk size down by avoiding the duplication of CA cert files. I'd like to test this against all RMW/OS combinations in CI to check which may not support symlinks.

Example:
https://github.com/keymint/keymint_tools/blob/9f5bda35dae4ddc927a7f70f2d961451c150f7c0/keymint_tools/build_types/keymint_ros2_dds.py#L167-L172

Signed-off-by: ruffsl <roxfoxpox@gmail.com>
Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

Thanks @ruffsl for opening this.
From memory the symlink approach used failed on Connext (though may have been working in versions>5.2.3) and also on Windows. Happy to test again to see if we can now get rid of file duplication and have nicer namespace layout.

At the moment this PR is invalid as it removes variables used in the lines below (e.g. keystore_ca_cert_path). Also code in for loop looks over-indented so it may fail the linters.
@ruffsl can you please test it locally to make sure that it works at least for the Linux + Fast-RTPS scenario and fix it for that use case?
Once that is working, we'll need to run CI with the following parameters to confirm secure communication still works:
CI parameters:
build: --packages-up-to test_security
test: --packages-select sros2 test_security

Signed-off-by: ruffsl <roxfoxpox@gmail.com>
Signed-off-by: ruffsl <roxfoxpox@gmail.com>
@ruffsl
Copy link
Member Author

ruffsl commented Feb 22, 2020

can you please test it locally to make sure that it works at least for the Linux + Fast-RTPS scenario and fix it for that use case?

65aeffd now passes sros2 tests locally using osrf/ros2:nightly-rmw-nonfree

Details
# colcon build --packages-up-to test_security
Starting >>> sros2   
Finished <<< sros2 [1.20s]          
Starting >>> test_security
Finished <<< test_security [3.86s]                  

Summary: 2 packages finished [5.29s]
root@6499bb1837bf:/opt/sros2_ws# colcon test --packages-select sros2 test_security
Starting >>> sros2   
Finished <<< sros2 [6.22s]          
Starting >>> test_security
Finished <<< test_security [2.95s]          

Summary: 2 packages finished [9.42s]
root@6499bb1837bf:/opt/sros2_ws# colcon test-result
Summary: 47 tests, 0 errors, 0 failures, 0 skipped

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

some symlink issue otherwise lgtm

Signed-off-by: ruffsl <roxfoxpox@gmail.com>
Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

@mjcarroll @jacobperron is it possible to run CI on this please?
CI parameters:
build: --packages-up-to test_security
test: --packages-select sros2 test_security

@jacobperron
Copy link
Member

jacobperron commented Feb 25, 2020

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows-container Build Status

@mikaelarguedas
Copy link
Member

GCC/CLang jobs unstable because of a false positive warning. It comes from cyclonedds' latest commit message being a full copy-paste of a build warning.

Unrelated to this PR

@jacobperron
Copy link
Member

haha, yeah, I noticed that in another job earlier.

Co-Authored-By: Jacob Perron <jacob@openrobotics.org>
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM, it looks like the macOS failures are unrelated (already occurring on the nightly job).

@jacobperron jacobperron merged commit 44479cd into master Mar 3, 2020
@delete-merged-branch delete-merged-branch bot deleted the ruffsl/symlink branch March 3, 2020 20:41
@mikaelarguedas mikaelarguedas mentioned this pull request Jun 5, 2020
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.

3 participants