fix: use namespace for spc lookup + unit tests #264

aramase · 2020-07-29T18:15:16Z

What this PR does / why we need it:

Sets the podNamespace env var for strict namespace enforcement
Refactors NodePublishVolume to add more unit test for the NodePublish code path
Updates logs to include pod name and pod namespace for debugging
Adds unit tests

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 #263

Special notes for your reviewer:

k8s-ci-robot · 2020-07-29T18:15:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aramase

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:

~~OWNERS~~ [aramase]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

aramase · 2020-07-29T19:41:13Z

/test pull-secrets-store-csi-driver-unit

pkg/secrets-store/nodeserver_test.go

ritazh · 2020-07-30T00:31:05Z

cmd/secrets-store-csi-driver/main.go

@@ -96,5 +98,13 @@ func main() {

 func handle() {
 	driver := secretsstore.GetDriver()
-	driver.Run(*driverName, *nodeID, *endpoint, *providerVolumePath, *minProviderVersion)
+	cfg, err := config.GetConfig()


I recall we had to call getClient() to get the latest client to prevent from reading from cache. Is this no longer a concern?

https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/client.go#L42-L52

// New returns a new Client using the provided config and Options. // The returned client reads *and* writes directly from the server // (it doesn't use object caches). It understands how to work with // normal types (both custom resources and aggregated/built-in resources), // as well as unstructured types.

I was thinking about this one kubernetes-sigs/controller-runtime#403
But I dont think we have multiple controllers updating the same objects now.

Right, we don't have any update operations on spc. There is only GET for spc and CREATE for spcpodstatus

Adding the links here for future ref

https://github.com/kubernetes-sigs/controller-runtime/blob/cea989b02ed02254d751ad30471c40bacd5888fc/pkg/client/client_cache.go#L88-L96

newResource always creates a new client - https://github.com/kubernetes-sigs/controller-runtime/blob/cea989b02ed02254d751ad30471c40bacd5888fc/pkg/client/client_cache.go#L51-L68

test/bats/azure.bats

aramase · 2020-07-30T18:44:19Z

/test pull-secrets-store-csi-driver-e2e-windows

aramase · 2020-07-30T18:44:36Z

@ritazh Updated the PR, PTAL!

ritazh · 2020-07-31T01:02:08Z

test/bats/vault.bats

+@test "Test Namespaced scope SecretProviderClass - Sync with K8s secrets - read secret from pod, read K8s secret, read env var, check secret ownerReferences" {
+  POD=$(kubectl get pod -l app=nginx -n test-ns -o jsonpath="{.items[0].metadata.name}")
+  result=$(kubectl exec -n test-ns -it $POD -- cat /mnt/secrets-store/foo)
+  [[ "$result" == "hello" ]]


can you pls update all these == to -eq

I think we should use == for string comparisons. Using -eq returns 0 even when the strings don't match.

#!/usr/bin/env bats export result="hel" @test "testing with eq" { [[ "$result" -eq "hello" ]] } @test "testing with other" { [[ "$result" == *"hello"* ]] } ➜ bats test.bats -t 1..2 ok 1 testing with eq not ok 2 testing with other # (in test file /Users/anishramasekar/Desktop/test.bats, line 9) # `@test "testing with other" {' failed

WDYT?

ritazh · 2020-07-31T18:51:12Z

test/bats/vault.bats

+
+  run kubectl delete -f $BATS_TESTS_DIR/vault_synck8s_v1alpha1_secretproviderclass.yaml
+  assert_success
+
  sleep 20
  result=$(kubectl get secret | grep foosecret | wc -l)
  [[ "$result" == "0" ]]


As discussed, lets use -eq for numeric and == for string.

aramase · 2020-07-31T19:40:43Z

/test pull-secrets-store-csi-driver-e2e-windows

aramase · 2020-07-31T19:41:04Z

/hold

(for windows tests to pass)

aramase · 2020-07-31T20:45:28Z

/hold cancel

ritazh · 2020-07-31T22:04:07Z

/lgtm

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 29, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2020

k8s-ci-robot requested a review from ritazh July 29, 2020 18:15

aramase changed the title ~~fix: use namespace for spc lookup + unit tests~~ [WIP] fix: use namespace for spc lookup + unit tests Jul 29, 2020

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 29, 2020

aramase force-pushed the same-ns-and-unit-tests branch 4 times, most recently from 836c715 to 2b5e7e9 Compare July 29, 2020 19:23

aramase changed the title ~~[WIP] fix: use namespace for spc lookup + unit tests~~ fix: use namespace for spc lookup + unit tests Jul 29, 2020

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 29, 2020

aramase force-pushed the same-ns-and-unit-tests branch 4 times, most recently from 136a827 to 076583c Compare July 29, 2020 23:14

ritazh reviewed Jul 30, 2020

View reviewed changes

pkg/secrets-store/nodeserver_test.go Show resolved Hide resolved

ritazh reviewed Jul 30, 2020

View reviewed changes

test/bats/azure.bats Outdated Show resolved Hide resolved

ritazh reviewed Jul 30, 2020

View reviewed changes

test/bats/azure.bats Show resolved Hide resolved

ritazh reviewed Jul 30, 2020

View reviewed changes

test/bats/azure.bats Show resolved Hide resolved

aramase force-pushed the same-ns-and-unit-tests branch from aa01e6c to b06260d Compare July 30, 2020 18:33

aramase added this to the v0.0.13 milestone Jul 31, 2020

aramase force-pushed the same-ns-and-unit-tests branch from b06260d to 04b05ee Compare July 31, 2020 00:12

aramase force-pushed the same-ns-and-unit-tests branch from 04b05ee to d2802c5 Compare July 31, 2020 00:23

ritazh reviewed Jul 31, 2020

View reviewed changes

aramase force-pushed the same-ns-and-unit-tests branch from d2802c5 to eef29c1 Compare July 31, 2020 19:20

aramase added 2 commits July 31, 2020 12:20

use namespace for spc lookup + unit tests

e55850a

add e2e tests for namespace

01ef150

aramase force-pushed the same-ns-and-unit-tests branch from eef29c1 to 00129f7 Compare July 31, 2020 19:20

add negative e2e tests

86e95c3

aramase force-pushed the same-ns-and-unit-tests branch from 00129f7 to 86e95c3 Compare July 31, 2020 19:29

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 31, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 31, 2020

k8s-ci-robot assigned ritazh Jul 31, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2020

k8s-ci-robot merged commit ea233c0 into kubernetes-sigs:master Jul 31, 2020

aramase deleted the same-ns-and-unit-tests branch July 31, 2020 22:24

aramase mentioned this pull request Aug 7, 2020

Sync with K8s secret requires SecretProviderClass in the same namespace as the pod Azure/secrets-store-csi-driver-provider-azure#177

Closed

aramase mentioned this pull request Aug 18, 2020

driver reads secrets from secretproviderclass of wrong namespace Azure/secrets-store-csi-driver-provider-azure#195

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use namespace for spc lookup + unit tests #264

fix: use namespace for spc lookup + unit tests #264

aramase commented Jul 29, 2020

k8s-ci-robot commented Jul 29, 2020

aramase commented Jul 29, 2020

ritazh Jul 30, 2020

aramase Jul 30, 2020

ritazh Jul 30, 2020

aramase Jul 31, 2020

aramase Jul 31, 2020

aramase commented Jul 30, 2020

aramase commented Jul 30, 2020

ritazh Jul 31, 2020

aramase Jul 31, 2020 •

edited

ritazh Jul 31, 2020

aramase commented Jul 31, 2020

aramase commented Jul 31, 2020

aramase commented Jul 31, 2020

ritazh commented Jul 31, 2020

fix: use namespace for spc lookup + unit tests #264

fix: use namespace for spc lookup + unit tests #264

Conversation

aramase commented Jul 29, 2020

k8s-ci-robot commented Jul 29, 2020

aramase commented Jul 29, 2020

ritazh Jul 30, 2020

Choose a reason for hiding this comment

aramase Jul 30, 2020

Choose a reason for hiding this comment

ritazh Jul 30, 2020

Choose a reason for hiding this comment

aramase Jul 31, 2020

Choose a reason for hiding this comment

aramase Jul 31, 2020

Choose a reason for hiding this comment

aramase commented Jul 30, 2020

aramase commented Jul 30, 2020

ritazh Jul 31, 2020

Choose a reason for hiding this comment

aramase Jul 31, 2020 • edited

Choose a reason for hiding this comment

ritazh Jul 31, 2020

Choose a reason for hiding this comment

aramase commented Jul 31, 2020

aramase commented Jul 31, 2020

aramase commented Jul 31, 2020

ritazh commented Jul 31, 2020

aramase Jul 31, 2020 •

edited