search

linode / linode-blockstorage-csi-driver

Container Storage Interface (CSI) Driver for Linode Block Storage
Apache License 2.0
64 stars 54 forks source link

Luks e2e #165

Closed avestuk closed 1 month ago

avestuk commented 5 months ago

General:

Pull Request Guidelines:

  1. [x] Does your submission pass tests?
  2. [x] Have you added tests?
  3. [x] Are you addressing a single feature in this PR?
  4. [x] Are your commits atomic, addressing one change per commit?
  5. [x] Are you following the conventions of the language?
  6. [x] Have you saved your large formatting changes for a different PR, so we can focus on your work?
  7. [x] Have you explained your rationale for why this feature is needed?
  8. [ ] Have you linked your PR to an open issue

Addresses the lack of tests in #159 - Creating loopback devices in docker was sadly not as straight forward as I had hoped and I wasn't able to get something working. Given the ease with which we can now write e2e tests thanks to #163 and @mhmxs an e2e test should afford us a better level of comfort.

Currently this PR is set as a draft to get some initial commentary back on the overall approach. Mainly I'm not enamoured with using static names for the LUKS storageClass and Secrets and the subsequent cleanup that has to happen. I don't think that there is an issue in modifying the "global" storageClass as ginkgo does parallelism using separate processes so mutation of storageClass in one process will not affect others.

Breaking this out into a separate Describe node would work but would require copying JustBeforeEach or moving it to a function and substantially altering the tests. I would like to add additional LUKS tests to show that LUKS volumes require a proper AES-XTS key in order to function, there may well be other tests we would like to add such as LUKS volume expansion etc.

avestuk commented 3 months ago

@nesv @eljohnson92 Hello from the other side 👋 Tagging the two of you as you've been here recently.

Do you want to take this PR over? It's just an E2E test for the LUKS encrypted volumes, at least one user was interested in LUKS encrypted volumes because they can manage their own keys and having an e2e test seems to make sense. The reason that I statically defined the LUKS key was that I never quite figured out how the keys should be generated. You'd think it'd just be 256/512 bytes of random stuff but it didn't quite seem to work for me.

Anyway if you want to throw this in the bin then by all means go ahead :)

eljohnson92 commented 3 months ago

Hey @avestuk thanks for the context here! Yes, we will take this effort over. We have a separate initiative to revamp the e2e tests. We'll likely roll this in with those changes

avestuk commented 3 months ago

@eljohnson92 🫡