-
Notifications
You must be signed in to change notification settings - Fork 189
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
test: new test framework for AKV provider #242
Conversation
c551d59
to
e913374
Compare
@bdlb77 This is the new test framework I was working on last week. PTAL when you get a chance! |
@aramase Will do! For splitting #45 Would you say for the testing section I should wait for this to be merged before making the PR to update the tests for local / remote container development or open a PR using bats and then a follow-up which will add any possible tests needed to make it compatible in Golang test suite? |
@bdlb77 I think we can start with PR using bats and then follow up with a new PR for the golang test suite. WDYT? |
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.
looks really good and awesome to see the test suite in Go! Just have a couple of questions mainly for clarity :)
Just curious: these e2e tests still require a Azure KeyVault filled with secrets, keys and certs, right? |
That's right. @bdlb77 has a script in a branch that can be run to initially setup all the required artifacts for testing. Once we get this framework change updated and merged, we can add the scripts as part of the repo. |
2ad538e
to
b12403e
Compare
5782572
to
8c9db7a
Compare
"install", | ||
podIdentityChartName, | ||
fmt.Sprintf("--namespace=%s", framework.NamespaceKubeSystem), | ||
"aad-pod-identity/aad-pod-identity", "--set", "nmi.allowNetworkPluginKubenet=true", "--wait", "--timeout=5m", "--debug", |
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.
Current test clusters run kubenet. I'll update the cluster config in a separate PR to use Azure CNI and then remove this configuration from testing.
"objects": string(objects), | ||
"usePodIdentity": "false", | ||
"useVMManagedIdentity": "true", | ||
"userAssignedIdentityID": config.UserAssignedIdentityID, |
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.
Where do you grant permissions to the kv resources for this identity?
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.
Permissions and role assignments aren't done as part of the test suite. They should be done out of band as part of e2e setup.
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.
In a followup PR, can you pls document prerequisite steps (or a script) for how someone can setup the e2e?
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.
sounds good! this is the issue #151 to track the script and docs.
Config *framework.Config | ||
Name string | ||
Namespace string | ||
Spec v1alpha1.SecretProviderClassSpec |
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.
Might be good to create a json template for the spec to catch regressions for all the expected values in parameters. WDYT?
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.
Based on offline discussion:
- We'll keep the examples dir which will contain sample manifests for each supported configuration.
- For schema validation, we'll need to ensure with thorough unit testing that the AKV provider handles all expected values.
fccd4ef
to
4b5c081
Compare
Codecov Report
@@ Coverage Diff @@
## master #242 +/- ##
=======================================
Coverage 62.50% 62.50%
=======================================
Files 6 6
Lines 472 472
=======================================
Hits 295 295
Misses 145 145
Partials 32 32 |
|
||
.PHONY: run | ||
run: | ||
cd $(TEST_E2E_DIR); GODEBUG=x509ignoreCN=0 go test -tags=e2e -timeout=90m -v -ginkgo.v \ |
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 tests require x509ignoreCN=0
?
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.
The x509: certificate relies on legacy Common Name field. If we use SANs in the certificate, then we don't need this. With the current artifacts we have in keyvault for testing we enable Common Name matching with GODEBUG=x509ignoreCN=0
to resolve the error.
refactor certs util for tests add test for public key configure pod identity test
/azp run pr-e2e-azure |
Azure Pipelines successfully started running 1 pipeline(s). |
Reason for Change:
e2eteam/busybox:1.29
for testing in windows. This is a smaller image.Follow up after PR is merged:
Requirements
Issue Fixed:
Please answer the following questions with yes/no:
Special Notes for Reviewers: