-
Notifications
You must be signed in to change notification settings - Fork 90
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
SecureComms: E2e Test SecureComms without KBS #2089
base: main
Are you sure you want to change the base?
SecureComms: E2e Test SecureComms without KBS #2089
Conversation
d7b9676
to
6dcef0f
Compare
f08e2d3
to
f582a4e
Compare
b7f3d93
to
815170c
Compare
So this change is just regression testing to check that the no_trustee version of securecomms doesn't cause any failures. Is there any easy way to validate that this set-up has been used? |
26d3602
to
524869d
Compare
9b5b9b4
to
0c3db17
Compare
a7cf406
to
2331371
Compare
c89bfc7
to
be301d2
Compare
if !testCase_secureComms_isActive { | ||
t.Skip("Skip - SecureComms is configured to be inactive - no need to test") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically do the skip in the provider test case, so libvirt_test in this case, so it might be worth following this pattern.
Also our current pattern for whether to skip tests or not is to use environment variables, so can you use TEST_E2E_SECURE_COMMS
rather than creating this extra go variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it really make sense to move the skip to libvirt_test, it needs to be skipped no matter who the provider is...
seems like common_suite is the natural place for it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your reasoning, but none of the other providers would call this test, unless you are planning to support testing this elsewhere in future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont see why SecureComms will not be tested with other providers in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TEST_E2E_SECURE_COMMS is a matrix parameter. It may have multiple values and the mapping is done once in config_libvirt.sh
. It results in potentially activating SecureComms. Here we need to identify if secureComms was activated in this matrix option and act accordingly. This is done by checking if props["SECURE_COMMS"] == "true" as shown on e2e/main_test.go.
Therefore we should not be using TEST_E2E_SECURE_COMMS env directly, but have our own env variable indicating is SecureComms is active - I am now using the SECURE_COMMS env variable for that.
if props["SECURE_COMMS"] == "true" { | ||
testCase_secureComms_isActive = true | ||
log.Info("Do setup secureComms is active") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code would not be needed if you followed the env var approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
src/cloud-api-adaptor/test/provisioner/libvirt/provision_common.go
Outdated
Show resolved
Hide resolved
551140b
to
6bd5e10
Compare
Add support for e2e testing SecureComms without KBS Signed-off-by: David Hadas <[email protected]>
6bd5e10
to
61929c7
Compare
Support testing SecureComms without KBS
See
Should be merged after #2065