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

test: use local ca to generate device certificates #2619

Merged
merged 12 commits into from
Jan 25, 2024
Merged

test: use local ca to generate device certificates #2619

merged 12 commits into from
Jan 25, 2024

Conversation

reubenmiller
Copy link
Contributor

Proposed changes

Use a local CA to generate the device certificates used in each test. This should improve overall performance, and avoid creating large amounts of device certificates in Cumulocity IoT

  • Generate device certificate during bootstrap.sh script using a local CA provided via environment variables (if CA_KEY and CA_PUB are present, which are base64 encoded values), otherwise continue using self signed certificate
  • Only run "delete cert" logic in the cleanup of tests if a self-signed device certificate is detected
  • Decouple the certificate deletion and the device deletion to avoid problems where the device is not cleaned up if the cert deletion fails

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue


Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@reubenmiller
ci: set c8y tenant id to avoid api call to look it up
b673360 
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
@reubenmiller
test: update c8y lib to handle cert deletion corner cases
ba420e4 
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
@reubenmiller
test: support using local ca to create device certificates
6a4efe1 
Local CA is provided via environment variables which have the cert as base64 encoded, CA_PUB and CA_KEY.

Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
@reubenmiller
test: decouple device and cert removal
73e02f0 
Only remove self signed device certificates

Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
@reubenmiller
ci: use local ca in system tests
877524c 
Inject local ca via environment variables so that each container can create their own leaf certificate. This avoid maintaining multiple trusted certificates in the cloud

Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
@codecov Codecov
Copy link

codecov bot commented Jan 24, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a53611a) 75.8% compared to head (c32c504) 75.8%.

Additional details and impacted files

see 2 files with indirect coverage changes

@reubenmiller reubenmiller mentioned this pull request Jan 24, 2024
Closed
11 tasks
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 24, 2024 •
edited

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
381 0 3 381 100 57m56.061s

@reubenmiller reubenmiller marked this pull request as ready for review January 24, 2024 13:00
didier-wenzek
didier-wenzek reviewed Jan 24, 2024
View reviewed changes
Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

I only have comments related to the documentation.

.github/workflows/integration-tests.yml Show resolved Hide resolved
tests/RobotFramework/README.md Outdated Show resolved Hide resolved
tests/RobotFramework/README.md Outdated Show resolved Hide resolved
tests/RobotFramework/README.md Outdated Show resolved Hide resolved
tests/images/debian-systemd/files/bootstrap.sh Show resolved Hide resolved
rina23q
rina23q approved these changes Jan 24, 2024
View reviewed changes
Copy link
Member

@rina23q rina23q left a comment

Choose a reason for hiding this comment

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

LGTM

didier-wenzek
didier-wenzek approved these changes Jan 24, 2024
View reviewed changes
Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Approved. This is working as expected on a dev laptop.

@reubenmiller
test: add instructions on how to specify your local ca key/cert
405098a 
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
@reubenmiller
chore: fix shellcheck warning which arose due to switching from posix…
01afbd0 
… shell to bash

Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
@reubenmiller reubenmiller added this pull request to the merge queue Jan 24, 2024
@reubenmiller
Copy link
Contributor Author

Some tests will need to be updated as they are failing due to switching to a CA signed certificate...will update the tests shortly

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 24, 2024
@reubenmiller
test(bootstrap): support forcing a specific certificate generation me…
fc5d20e 
…thod

Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
@reubenmiller
test(bootstrap): fallback to older agent config keys if the newer key…
00bd525 
… name is not accepted

Some tests use older versions of the agent, so the bootstrap script needs to support both the new and older key names to get the certificate paths

Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
@reubenmiller
test: refactor test to reduce side-effects across tests and enforce s…
29b4ed4 
…elf signed certificates when needed

Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
@reubenmiller
test: skip warning log entries when cleaning up devices if the manage…
d379c3f 
…d object does not exist in c8y

The warning log entries cause too much noise in the system test log output and give the impression that something is wrong

Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
@reubenmiller
test: increase test timeout to avoid sporadic failures in CI
c32c504 
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
@reubenmiller reubenmiller added this pull request to the merge queue Jan 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 25, 2024
@reubenmiller reubenmiller added this pull request to the merge queue Jan 25, 2024
Merged via the queue into thin-edge:main with commit 4617019 Jan 25, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@didier-wenzek didier-wenzek didier-wenzek approved these changes

@rina23q rina23q rina23q approved these changes

@albinsuresh albinsuresh Awaiting requested review from albinsuresh albinsuresh is a code owner

@jarhodes314 jarhodes314 Awaiting requested review from jarhodes314 jarhodes314 is a code owner

@Bravo555 Bravo555 Awaiting requested review from Bravo555 Bravo555 is a code owner

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@reubenmiller @didier-wenzek @rina23q