test: e2e tests for kmp refresh logic #1742

duffney · 2024-08-23T15:50:29Z

Description

End to end tests that validate the KMP refresh feature.

Which issue(s) this PR fixes (optional, using `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged):

Fixes #

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

Validate the refresher is enabled by counting reconcile events in the Ratify logs
Validate certificate versions update with no version specified
Validate certificate versions do not update when a version is specified

Checklist:

Does the affected code have corresponding tests?
Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
Does this introduce breaking changes that would require an announcement or bumping the major version?
Do all new files have appropriate license header?

Post Merge Requirements

MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates Helm Chart Change

duffney · 2024-08-23T15:51:25Z

Successful build: https://github.com/duffney/ratify/actions/runs/10528215388

codecov · 2024-08-23T15:54:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

see 19 files with indirect coverage changes

binbin-li · 2024-08-26T04:25:02Z

test/bats/azure-test.bats

+        -e "s|https://yourkeyvault.vault.azure.net/|${VAULT_URI}|" \
+        -e "s/tenantID:/tenantID: ${TENANT_ID}/" \
+        -e "s/clientID:/clientID: ${IDENTITY_CLIENT_ID}/" \
+        ./config/samples/clustered/kmp/config_v1beta1_keymanagementprovider_akv_refresh_enabled.yaml


could you redirect to a new file instead of updating in place?
Just add >your_file.yaml in the end.

Can do. :) Does it matter where it's output? I was thinking .tests/bats/tests/config

you can even just specify a file without path like test.yaml, and delete it after test completes.

binbin-li · 2024-08-26T04:36:14Z

test/bats/azure-test.bats

@@ -318,3 +318,47 @@ SLEEP_TIME=1
    result=$(kubectl get pod mutate-demo --namespace default -o json | jq -r ".spec.containers[0].image" | grep @sha)
    assert_mutate_success
 }
+
+@test "validate refresher reconcile count" {


wonder if we can add a teardown method to reset the refreshInterval after the test is done to avoid throttling on AKV.

In the third test there is a teardown that deletes the KMP resource, which would only keep it alive until the final test finishes. However, I could add a clean up to each test and recreate the kmp within each test so it doesn't run longer than it needs to.

QQ: does 👍 mean it's okay to delete it at the end in the final test or should I modify each test to create and delete the resource? :)

In general, bats tests can be run independent of each other via the bats cli. We should try to keep tests independent and self-contained. I think we should add cleanup logic to each test. Just my opinion.

Roger roger (StarWars reference) I'll get cleanup logic for each test added.

binbin-li · 2024-08-26T04:47:17Z

test/bats/azure-test.bats

+    sleep 15
+    run rm policy.json
+    refreshResult=$(kubectl get keymanagementprovider kmp-akv-refresh -o jsonpath='{.status.properties.Certificates[0].Version}')
+    [ "$result" != "$refreshResult" ]


wonder the difference between it and

run [] assert_success

result is the certificate version pulled from the kmp resources at the start of the test, which is the old version of the certificate. And refreshResult is the version pulled after a new version of the certificate has been created. I then use != ensure the version are not equal because if they are then the refresher didn't work.

My understanding that run and assert_success would only validate that the command ran successfully, which it would no matter what, as long as the resource existed. Please lmk if my understanding is incorrect and if I could use that instead. :)

the assert_success checks the exit status, 0 if the run command holds true and 1 otherwise. And assert_success would return 1 to stop executing the test. Wonder if [ "$result" != "$refreshResult" ] behaves the same, as long as it will fail early we can keep it.

That's a good question, I don't have the answer too. :) I'm not to familiar with bats.

binbin-li · 2024-08-26T04:53:06Z

test/bats/azure-test.bats

+    [ $count -ge 4 ]
+}
+
+@test "validate certificate version update" {


nit: the test description is not clear enough to me, wonder if you can provide more information.

How about validate refresher updates kmp with latest certificate version?

yeah, that makes more sense to me

duffney · 2024-08-27T18:02:11Z

Just noticed a few comments aren't verified so I'll squash them into one and push those changes up after the open comments have been resolved. :)

binbin-li

lgtm, thanks for adding e2e tests!

susanshi · 2024-09-03T00:06:32Z

Hi @duffney, looks like this PR is also blocked due to unsigned commit. Could you take a look? thanks!

…idation - Added a test to validate the refresher reconcile count with modified timing and Key Vault configuration. - Implemented a test to ensure certificate version updates are correctly reflected in KeyManagementProvider after creating a new version in Azure Key Vault. - Created a test to verify that a specified certificate version in KeyManagementProvider remains consistent after attempting to update the certificate in Azure Key Vault. Signed-off-by: Joshua Duffney <[email protected]>

Signed-off-by: akashsinghal <[email protected]>

duffney requested review from akashsinghal, binbin-li, jimmyraywv, luisdlp, susanshi and toddysm as code owners August 23, 2024 15:50

duffney changed the title ~~Tests: e2e tests for KeyManagementProvider refresh logic~~ tests: e2e tests for KeyManagementProvider refresh logic Aug 23, 2024

duffney changed the title ~~tests: e2e tests for KeyManagementProvider refresh logic~~ tests: e2e tests for kmp refresh logic Aug 23, 2024

duffney force-pushed the refresher-e2etests branch from 9e4a35d to 4e9d638 Compare August 23, 2024 16:14

duffney changed the title ~~tests: e2e tests for kmp refresh logic~~ test: e2e tests for kmp refresh logic Aug 23, 2024

duffney force-pushed the refresher-e2etests branch from 4e9d638 to 9622f6d Compare August 23, 2024 16:21

binbin-li reviewed Aug 26, 2024

View reviewed changes

duffney force-pushed the refresher-e2etests branch 3 times, most recently from 8284f31 to 747fe5c Compare August 27, 2024 13:50

binbin-li previously approved these changes Aug 29, 2024

View reviewed changes

duffney dismissed binbin-li’s stale review via 90d8d38 September 3, 2024 15:04

duffney force-pushed the refresher-e2etests branch from bed5550 to 90d8d38 Compare September 3, 2024 15:04

duffney force-pushed the refresher-e2etests branch from 90d8d38 to 189d4e2 Compare September 3, 2024 15:05

duffney added 2 commits September 3, 2024 10:35

fix: increase sleep for akv cert to create

e3d9a7a

fix: add sleep for status to be populated on kmp resource

0789c81

binbin-li approved these changes Sep 4, 2024

View reviewed changes

binbin-li merged commit 1c52df8 into ratify-project:dev Sep 4, 2024
19 checks passed

akashsinghal pushed a commit to akashsinghal/ratify that referenced this pull request Sep 13, 2024

test: e2e tests for kmp refresh logic (ratify-project#1742)

bb12ea3

Signed-off-by: akashsinghal <[email protected]>

binbin-li pushed a commit to binbin-li/ratify that referenced this pull request Sep 14, 2024

test: e2e tests for kmp refresh logic (ratify-project#1742)

de8531b

duffney deleted the refresher-e2etests branch September 25, 2024 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: e2e tests for kmp refresh logic #1742

test: e2e tests for kmp refresh logic #1742

duffney commented Aug 23, 2024

duffney commented Aug 23, 2024

codecov bot commented Aug 23, 2024 •

edited

Loading

binbin-li Aug 26, 2024

duffney Aug 26, 2024 •

edited

Loading

binbin-li Aug 27, 2024

binbin-li Aug 26, 2024

duffney Aug 26, 2024

duffney Aug 27, 2024

akashsinghal Aug 27, 2024 •

edited

Loading

duffney Aug 28, 2024

binbin-li Aug 26, 2024

duffney Aug 26, 2024 •

edited

Loading

binbin-li Aug 27, 2024

duffney Aug 27, 2024

binbin-li Aug 26, 2024

duffney Aug 26, 2024 •

edited

Loading

binbin-li Aug 27, 2024

duffney commented Aug 27, 2024

binbin-li left a comment

susanshi commented Sep 3, 2024

test: e2e tests for kmp refresh logic #1742

test: e2e tests for kmp refresh logic #1742

Conversation

duffney commented Aug 23, 2024

Description

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

How Has This Been Tested?

Checklist:

Post Merge Requirements

duffney commented Aug 23, 2024

codecov bot commented Aug 23, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

duffney Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akashsinghal Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duffney Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duffney Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duffney commented Aug 27, 2024

binbin-li left a comment

Choose a reason for hiding this comment

susanshi commented Sep 3, 2024

Which issue(s) this PR fixes (optional, using `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged):

codecov bot commented Aug 23, 2024 •

edited

Loading

duffney Aug 26, 2024 •

edited

Loading

akashsinghal Aug 27, 2024 •

edited

Loading

duffney Aug 26, 2024 •

edited

Loading

duffney Aug 26, 2024 •

edited

Loading