-
Notifications
You must be signed in to change notification settings - Fork 278
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: add e2e for conjur csi provider #1498
Conversation
|
Welcome @gl-johnson! |
Hi @gl-johnson. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
135d1d1
to
d1d197f
Compare
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.
/ok-to-test
/test pull-secrets-store-csi-driver-e2e-conjur |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1498 +/- ##
==========================================
- Coverage 36.58% 35.78% -0.81%
==========================================
Files 63 63
Lines 4532 3759 -773
==========================================
- Hits 1658 1345 -313
+ Misses 2730 2270 -460
Partials 144 144 ☔ View full report in Codecov by Sentry. |
test/bats/conjur.bats
Outdated
@test "allow conjur provider token requests and enable rotation" { | ||
helm upgrade csi-secrets-store manifest_staging/charts/secrets-store-csi-driver --namespace=kube-system \ | ||
--set tokenRequests[0].audience="conjur" \ | ||
--set enableSecretRotation=true \ | ||
--set rotationPollInterval=30s | ||
|
||
kubectl wait --for=condition=ready --timeout=${WAIT_TIME}s --namespace=kube-system pod -l "app=secrets-store-csi-driver" | ||
} |
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.
Could you add --set tokenRequests[2].audience="conjur"
to
secrets-store-csi-driver/Makefile
Lines 439 to 440 in 5f6daa0
--set tokenRequests[0].audience="aud1" \ | |
--set tokenRequests[1].audience="aud2" |
test/bats/conjur.bats
Outdated
# Create Conjur namespace | ||
kubectl create namespace $CONJUR_NAMESPACE |
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.
You can use --create-namespace
flag as part of helm install
command
test/bats/conjur.bats
Outdated
|
||
# Install Conjur CSI Provider | ||
helm install conjur-csi-provider cyberark/conjur-k8s-csi-provider --wait --timeout ${WAIT_TIME}s --namespace=kube-system \ | ||
--set providerServer.image.tag=latest |
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.
latest
tag in provider could break CI in the driver. Is there a pinned version you can use here?
test/bats/conjur.bats
Outdated
docker pull cyberark/conjur-cli:8 | ||
kind load docker-image cyberark/conjur-cli:8 |
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 image is public, so I don't think this step is required.
test/bats/conjur.bats
Outdated
} | ||
|
||
@test "CSI inline volume test with pod portability" { | ||
kubectl apply --namespace=kube-system -f $BATS_TESTS_DIR/pod-secrets-store-inline-volume-crd.yaml |
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 the workload need to be in kube-system
namespace? If someone looks at this file for reference, I don't want them to assume this is a requirement. Could we use any namespace other than kube-system
for the workloads being deployed?
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've moved the test workloads to the default
namespace
/test pull-secrets-store-csi-driver-e2e-conjur |
/retest-required |
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.
Just one minor comment!
After the update, could you squash to a single commit?
test/bats/conjur.bats
Outdated
helm install conjur cyberark/conjur-oss --namespace=$CONJUR_NAMESPACE --wait --timeout ${WAIT_TIME}s \ | ||
--create-namespace --namespace=$CONJUR_NAMESPACE \ |
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.
helm install conjur cyberark/conjur-oss --namespace=$CONJUR_NAMESPACE --wait --timeout ${WAIT_TIME}s \ | |
--create-namespace --namespace=$CONJUR_NAMESPACE \ | |
helm install conjur cyberark/conjur-oss --namespace=$CONJUR_NAMESPACE --wait --timeout ${WAIT_TIME}s \ | |
--create-namespace \ |
--namespace
flag is already defined
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.
Woops - all should be addressed now!
/retest-required |
1 similar comment
/retest-required |
/test pull-secrets-store-csi-driver-e2e-conjur |
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aramase, gl-johnson The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
What this PR does / why we need it:
Adds e2e test retrieving secrets via the Conjur provider
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 #
Special notes for your reviewer:
TODOs: