Skip to content
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

Add integration tests for cloud storage #1246

Merged
merged 22 commits into from May 1, 2018

Conversation

Projects
None yet
2 participants
@c-w
Copy link
Contributor

commented Apr 17, 2018

In order to facilitate onboarding of new storage backends in the future (e.g. Azure Blob Storage), this pull request refactors the existing tests for the local storage backend to also run for cloud storage backends.

The integration tests are disabled by default. For Google Cloud Storage, the integration tests can be enabled by setting the environment variables GCP_STORAGE_KEY and GCP_STORAGE_SECRET.

This pull request also fixes two edge-cases in the storage implementation that were uncovered by the tests:

  1. The use of datetime.strftime('%s') relies on platform-specific undocumented behavior and may not work across Python versions or deployment environments. This was switched to a cross-platform way of converting a datetime to epoch via datetime.timestamp().

  2. Submitting malicious files to Google Cloud Storage returned a 400 error rejecting the file. To mirror the behavior of the local storage provider, "special" parts of files uploaded to non-local storage such as . and .. now get sanitized to underscores.

Furthermore, the pull request also:

  1. Updates to the latest version of apache-libcloud which improves handling of missing files. Without this update, fetching non-existing blobs produces a 302 result and test_malicious_local_get_blob fails.

  2. Adds new tests to verify the results of binary downloads (previously only text downloads were verified).

  3. Disables parallel test execution on CircleCI. The cloud storage tests rely on mutating environment variables before creation of the Flask app which means that the tests are no longer guaranteed to be threadsafe.

All the changes described above can be found in d16b5ac. The commit depends on 42e40d6 which introduces more parameterization to the test settings such that the storage backend used during the tests can be configured via environment variables.

The pull request also contains a number of additional minor fixes:

  • 3b273be and 0ff08d0: Remove creation of directories in settings module and instead ensure that the storage module always creates a container if it doesn't yet exist.
  • 0559126: Add SQLite journal file to git-ignore to avoid committing the binary file (e.g during long tests runs).
  • aff57b6: Remove direct usage of pytest for exception checking and instead use standard unittest like the rest of the tests in the project.

@colinschoen colinschoen self-requested a review Apr 17, 2018

@colinschoen
Copy link
Member

left a comment

Thanks for all your contributions.

def test_malicious_local_get_blob(self):
with pytest.raises(ObjectDoesNotExistError):
with self.assertRaises(ObjectDoesNotExistError):

This comment has been minimized.

Copy link
@colinschoen

colinschoen Apr 17, 2018

Member

Nice. I'm not sure how pytest.raises got in here.

CloudTestFile.
To run tests for Google Cloud Platform, set the following environment variables:
- GCP_STORAGE_KEY

This comment has been minimized.

Copy link
@colinschoen

colinschoen Apr 17, 2018

Member

TODO(@colinschoen) Configure CI to run the full integration test suite only on protected branches like master.

This comment has been minimized.

Copy link
@c-w

c-w Apr 18, 2018

Author Contributor

Added TODO note in 66dfffb

cls.storage_key = os.environ.get(cls.key_env_name)
cls.storage_secret = os.environ.get(cls.secret_env_name)
if not cls.storage_key or not cls.storage_secret:
raise unittest.SkipTest("Cloud storage credentials for "

This comment has been minimized.

Copy link
@colinschoen

colinschoen Apr 17, 2018

Member

nit: Prefer .format vs %

This comment has been minimized.

Copy link
@c-w

c-w Apr 18, 2018

Author Contributor

Done in 7b2040c

os.environ["STORAGE_PROVIDER"] = self.storage_provider
os.environ["STORAGE_KEY"] = self.storage_key
os.environ["STORAGE_SECRET"] = self.storage_secret
os.environ.setdefault("STORAGE_CONTAINER", "okpycloudfilestest%d" % random.randint(0, 100000))

This comment has been minimized.

Copy link
@colinschoen

colinschoen Apr 17, 2018

Member

nit: Prefer .format vs %

This comment has been minimized.

Copy link
@c-w

c-w Apr 18, 2018

Author Contributor

Done in 7b2040c

def create_app(self):
os.environ["STORAGE_PROVIDER"] = self.storage_provider
os.environ["STORAGE_KEY"] = self.storage_key
os.environ["STORAGE_SECRET"] = self.storage_secret

This comment has been minimized.

Copy link
@colinschoen

colinschoen Apr 17, 2018

Member

I suspect service accounts will be the primary form of authentication used for the integration tests to access to the GCS buckets. There isn't currently a way for users to reference the *.json key file which can be downloaded in the Google Cloud Console so in order to set the key data they will need to copy and paste the value of the private key. It is likely that they will forget to use literal line breaks and instead just copy and paste the private key given in the downloaded .json key file which is of the form:

-----BEGIN PRIVATE KEY-----\n{SECRET_HERE}\n-----END PRIVATE KEY-----\n

The shell will interpret \n as a literal backslash n. backslash leading to some cryptic error messages about being raised in the Crypto library.

This should fix that:
os.environ["STORAGE_SECRET"] = self.storage_secret.replace('\\n', '\n')

This comment has been minimized.

Copy link
@colinschoen

colinschoen Apr 17, 2018

Member

We should also add this here and in the rest of the settings files that use cloud storage. I can handle that.

This comment has been minimized.

Copy link
@c-w

c-w Apr 18, 2018

Author Contributor

Added TODO note in 66dfffb

for obj in storage.container.list_objects():
obj.delete()

storage.container.delete()

This comment has been minimized.

Copy link
@colinschoen

colinschoen Apr 17, 2018

Member

Idea: With the goal being to restrict the permissions needed for the service account used in the integration test can we not delete the buckets unless they were created in the test.

This comment has been minimized.

Copy link
@c-w

c-w Apr 18, 2018

Author Contributor

Done in f95d8f8

del os.environ["STORAGE_CONTAINER"]

for obj in storage.container.list_objects():
obj.delete()

This comment has been minimized.

Copy link
@colinschoen

colinschoen Apr 17, 2018

Member

Idea: it might be nice to be more explicit about only deleting objects in the container that were created during the test.

This comment has been minimized.

Copy link
@c-w

c-w Apr 18, 2018

Author Contributor

Done in f95d8f8

del os.environ["STORAGE_PROVIDER"]
del os.environ["STORAGE_KEY"]
del os.environ["STORAGE_SECRET"]
del os.environ["STORAGE_CONTAINER"]

This comment has been minimized.

Copy link
@colinschoen

colinschoen Apr 17, 2018

Member

We shouldn't necessarily delete the STORAGE_PROVIDER env variable unless we explicitly set it. Otherwise, only the first test will use the user configured value and future tests will create their own containers.

This comment has been minimized.

Copy link
@c-w

c-w Apr 18, 2018

Author Contributor

Refactored the environment variable handling in 3afc998. We now only set the environment variables at class creation time and restore them back to original values at class cleanup time.

try:
self.container = self.driver.get_container(self.container_name)
except ContainerDoesNotExistError:
self.container = self.driver.create_container(self.container_name)

This comment has been minimized.

Copy link
@colinschoen

colinschoen Apr 17, 2018

Member

Creating a container using a service account will also require the project_id to be included in the arguments when creating the storage driver ref.

This is outside the scope of this PR, but we should probably make a change here to support that as well as adding the necessary variable in the settings templates.

This comment has been minimized.

Copy link
@c-w

c-w Apr 18, 2018

Author Contributor

Added TODO note in 66dfffb

@c-w

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2018

Hi @colinschoen. Thanks for the review. I should have addressed everything now and the PR should be ready for a second round of review. Looking forward to your comments!

@colinschoen

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

Thanks Clemens! Taking a look now.

@colinschoen
Copy link
Member

left a comment

The mixin looks great. I only see one issue with explicitly setting the storage container to use.

cls.set_environment_variable("STORAGE_PROVIDER", cls.storage_provider)
cls.set_environment_variable("STORAGE_KEY", storage_key)
cls.set_environment_variable("STORAGE_SECRET", storage_secret)
cls.set_environment_variable("STORAGE_CONTAINER", cls._storage_container)

This comment has been minimized.

Copy link
@colinschoen

colinschoen Apr 19, 2018

Member

I think this will prevent users from setting a storage container to use for the integration test via an environment variable. Can we first check to see if STORAGE_CONTAINER is set. If not then use cls._storage_container.

This comment has been minimized.

Copy link
@c-w

c-w Apr 19, 2018

Author Contributor

I believe this is by design. The way that the code is currently set up is that the touch-point to enable/disable certain integration tests are the environment variables in key_env_name and secret_env_name. Being an abstract test base, the CloudTestFile never actually executes and each cloud-specific subclass will set the storage provider environment variable that it's testing for its tests and then we revert back to the default test storage (local) for the remainder of the tests suite.

For example, if the GCP_STORAGE_KEY and GCP_STORAGE_SECRET environment variables are set, the GoogleCloudTestFile sets the storage provider to GOOGLE_STORAGE for the 14 tests it inherits from TestFile and then we revert back to local for the rest of the test suite.

class GoogleCloudTestFile(CloudTestFile):
    storage_provider = "GOOGLE_STORAGE"
    key_env_name = "GCP_STORAGE_KEY"
    secret_env_name = "GCP_STORAGE_SECRET"

    test_prefix_expected_obj_name = 'test/fizz.txt'
    test_malicious_directory_traversal_expected_obj_name = 'test/_/_/fizz.txt'

Similarly, if we wanted to make a additional integration test for S3 storage, we'd add a new class like this:

class AWSCloudTestFile(CloudTestFile):
    storage_provider = "S3"
    key_env_name = "S3_STORAGE_KEY"
    secret_env_name = "S3_STORAGE_SECRET"

    test_prefix_expected_obj_name = 'test/fizz.txt'
    test_malicious_directory_traversal_expected_obj_name = 'test/_/_/fizz.txt'

The system STORAGE_PROVIDER environment variable is still read and determines the storage provider that's used throughout the entire test suite (including TestFile). The CloudTestFile subclasses override this value to be able to verify each of the supported storage backends specifically.

This setup of the code enables us to run integration tests for the 14 storage tests against multiple backends (local, GCP, AWS, etc.) during the same test run without having to re-run the entire test suite (100+ tests) for each of the storage backends.

Hope that clears things up (and I'd be happy to hear suggestions on how this could be made clearer in the code to avoid future confusion).

If we want to run the entire test suite for each of the storage providers (instead of just the TestFile cases), please let me know @colinschoen and I can make that change.

This comment has been minimized.

Copy link
@colinschoen

colinschoen Apr 19, 2018

Member

Maybe I am misunderstanding, but I am referring to STORAGE_CONTAINER not STORAGE_PROVIDER. This should still be able to be explicitly set by the user via an environment variable, right?

This comment has been minimized.

Copy link
@c-w

c-w Apr 20, 2018

Author Contributor

Apologies, @colinschoen, I completely misread that for some reason. Implemented respecting of any pre-existing storage container value in 8799fcd.

@colinschoen
Copy link
Member

left a comment

I'm also now failing some of the original test_files, but those seem to be passing on CI of your PR so I'll figure out what is going on in my environment.

cls.restore_environment_variable("STORAGE_SECRET")
cls.restore_environment_variable("STORAGE_CONTAINER")

container = storage.driver.get_container(cls._storage_container)

This comment has been minimized.

Copy link
@colinschoen

colinschoen Apr 20, 2018

Member

This will throw a ContainerDoesNotExistError when STORAGE_CONTAINER is set by the user.

This comment has been minimized.

Copy link
@c-w

c-w Apr 20, 2018

Author Contributor

Thanks for the continued thorough review, @colinschoen.

At this point, I think it's simpler if we just require the storage container to be already created by the time that the integration tests run, so I've implemented that in f1b2e38. We can re-use the same container for all test runs so it's only a one-time setup cost.

Requiring a pre-existing container simplifies handling of the environment variables and also means that we don't require a special service account with create/delete bucket permissions to run the tests.

Note that I've introduced a new environment variable for the storage account name used in the integration tests. This has two advantages:

  • Increased consistency with the way that the storage key and storage secret are set.
  • A container name for the local-storage tests can be set via CONTAINER_NAME and a different container name for the the GCP cloud storage integration tests can be set via GCP_STORAGE_CONTAINER (to prevent potential clashes between names that are valid local containers but not in the cloud provider).

Here is how I ran the tests:

  1. Create a new storage project in GCP.
  2. Set the project as the default project for interoperable access.
  3. Copy the access key to GCP_STORAGE_KEY and the secret to GCP_STORAGE_SECRET.
  4. Create a new bucket and copy its name to GCP_STORAGE_CONTAINER.
  5. Execute python manage.py test (passes).
  6. Unset all the GCP_ environment variables.
  7. Run python manage.py test (passes).

Thoughts?

This comment has been minimized.

Copy link
@colinschoen

colinschoen Apr 20, 2018

Member

Awesome, I like this a lot. I will give it a go.

This comment has been minimized.

Copy link
@colinschoen

colinschoen Apr 24, 2018

Member

I haven't had a chance to debug what is going on, but I am failing a few GoogleCloudTestFile tests as well as TestFile tests when setting the following environment variables: GCP_STORAGE_KEY, GCP_STORAGE_SECRET, GCP_STORAGE_CONTAINER.

Interestingly, I don't fail those TestFile tests when the said environment variables are unset.

I will be taking a look to figure out why.
test_output.txt

@colinschoen
Copy link
Member

left a comment

As discussed per OK Slack. This seems to be working when using the interoperability storage API. The one test that was failing then is now passing, maybe a flake. It would be nice to make this working using a service account, but I think for now this is fine.

LGTM

c-w added some commits Apr 16, 2018

Remove redundant directory creation
The directory creation already happens in init_app in server.storage so
it's not necessary to create the directories in the test settings.

Additionally, this change ensures that for non-local storage providers
we don't create the directories.
Add integration tests for cloud storage
The new integration tests run slightly modified versions of the existing
tests for the local storage backend. For Google Cloud Storage, the
integration tests can be enabled by setting the environment variables
`GCP_STORAGE_KEY` and `GCP_STORAGE_SECRET`. The code is designed to be
reusable across a wide range of cloud storage backends which will make
onboarding of other backends (e.g. Azure Blob Storage) easier in the
future.

This commit also fixes two issues in the storage implementation that
were uncovered by the tests:

1) The use of `datetime.strftime('%s')` relies on platform-specific
undocumented behavior and may not work across Python versions or
deployment environments. This was switched to a cross-platform way of
converting a datetime to epoch via `datetime.timestamp()`.

2) Submitting malicious files to Google Cloud Storage returned a 400
error rejecting the file. To mirror the behavior of the local storage
provider, "special" parts of files uploaded to non-local storage (such
as "." and "..") now get sanitized to underscores.

Furthermore, the commit also:

3) Updates to the latest version of apache-libcloud which improves
handling of missing files. Without this update, fetching non-existing
blobs produces a 302 result and `test_malicious_local_get_blob` fails.

4) Adds new tests to verify the results of binary downloads (previously
only text downloads were verified).

5) Disables parallel test execution on CircleCI. The cloud storage tests
rely on mutating environment variables before creation of the Flask app
which means that the tests are no longer guaranteed to be threadsafe.
Pull-up expected file-name overrides
These will likely be the same for all cloud storage providers since AWS,
GCP, Azure, etc. all support folders.
@c-w

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2018

The CI failed due to TestApi.test_get_backup which compares date-time values at seconds granularity (Travis CI link). Running the test locally passes so it's a flaky one. Rebasing to re-trigger the CI run.

The test could be fixed by comparing date-time values with a certain tolerance threshold (e.g. one second) but that's outside of the scope of this PR.

@c-w

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2018

CI is now green. Let me know if you'd like me to do anything else before merging, @colinschoen (And thanks for your continued support throughout the pull request!)

@colinschoen

This comment has been minimized.

Copy link
Member

commented May 1, 2018

Thank you and the rest of the Microsoft folks for your contributions!!

@colinschoen colinschoen merged commit 8b82c89 into okpy:master May 1, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@c-w c-w deleted the c-w:enhancement/c-w/cloud-storage-backend-tests branch May 1, 2018

tmbgreaves added a commit to icokpy/ok that referenced this pull request May 2, 2018

Add integration tests for cloud storage (okpy#1246)
* Remove redundant directory creation

The directory creation already happens in init_app in server.storage so
it's not necessary to create the directories in the test settings.

Additionally, this change ensures that for non-local storage providers
we don't create the directories.

* Create storage container if it doesn't exist

* Add test settings for non-local storages

* Add integration tests for cloud storage

The new integration tests run slightly modified versions of the existing
tests for the local storage backend. For Google Cloud Storage, the
integration tests can be enabled by setting the environment variables
`GCP_STORAGE_KEY` and `GCP_STORAGE_SECRET`. The code is designed to be
reusable across a wide range of cloud storage backends which will make
onboarding of other backends (e.g. Azure Blob Storage) easier in the
future.

This commit also fixes two issues in the storage implementation that
were uncovered by the tests:

1) The use of `datetime.strftime('%s')` relies on platform-specific
undocumented behavior and may not work across Python versions or
deployment environments. This was switched to a cross-platform way of
converting a datetime to epoch via `datetime.timestamp()`.

2) Submitting malicious files to Google Cloud Storage returned a 400
error rejecting the file. To mirror the behavior of the local storage
provider, "special" parts of files uploaded to non-local storage (such
as "." and "..") now get sanitized to underscores.

Furthermore, the commit also:

3) Updates to the latest version of apache-libcloud which improves
handling of missing files. Without this update, fetching non-existing
blobs produces a 302 result and `test_malicious_local_get_blob` fails.

4) Adds new tests to verify the results of binary downloads (previously
only text downloads were verified).

5) Disables parallel test execution on CircleCI. The cloud storage tests
rely on mutating environment variables before creation of the Flask app
which means that the tests are no longer guaranteed to be threadsafe.

* Add SQLite journal file to git-ignore

* Make exception check consistent with other tests

* Fix inconsistent use of single vs double quotes

* Add TODO comments

* Replace % string formatting with .format

* Fix resetting of STORAGE_PROVIDER env var

* Refactor handling of environment variables

* Only cleanup objects explicitly created by test

* Split out EnvironmentMutationMixin

* Handle restore after multiple env var set calls

* Fix file lock error when running tests on Windows

* Respect pre-existing STORAGE_CONTAINER value

* Always require the storage container to pre-exist

* Ensure correct parsing of secrets with newlines

* Remove unused import

* List all required env vars in module docstring

* Revert unnecessary line change

* Pull-up expected file-name overrides

These will likely be the same for all cloud storage providers since AWS,
GCP, Azure, etc. all support folders.

tmbgreaves added a commit to icokpy/ok that referenced this pull request May 2, 2018

Add integration tests for cloud storage (okpy#1246)
* Remove redundant directory creation

The directory creation already happens in init_app in server.storage so
it's not necessary to create the directories in the test settings.

Additionally, this change ensures that for non-local storage providers
we don't create the directories.

* Create storage container if it doesn't exist

* Add test settings for non-local storages

* Add integration tests for cloud storage

The new integration tests run slightly modified versions of the existing
tests for the local storage backend. For Google Cloud Storage, the
integration tests can be enabled by setting the environment variables
`GCP_STORAGE_KEY` and `GCP_STORAGE_SECRET`. The code is designed to be
reusable across a wide range of cloud storage backends which will make
onboarding of other backends (e.g. Azure Blob Storage) easier in the
future.

This commit also fixes two issues in the storage implementation that
were uncovered by the tests:

1) The use of `datetime.strftime('%s')` relies on platform-specific
undocumented behavior and may not work across Python versions or
deployment environments. This was switched to a cross-platform way of
converting a datetime to epoch via `datetime.timestamp()`.

2) Submitting malicious files to Google Cloud Storage returned a 400
error rejecting the file. To mirror the behavior of the local storage
provider, "special" parts of files uploaded to non-local storage (such
as "." and "..") now get sanitized to underscores.

Furthermore, the commit also:

3) Updates to the latest version of apache-libcloud which improves
handling of missing files. Without this update, fetching non-existing
blobs produces a 302 result and `test_malicious_local_get_blob` fails.

4) Adds new tests to verify the results of binary downloads (previously
only text downloads were verified).

5) Disables parallel test execution on CircleCI. The cloud storage tests
rely on mutating environment variables before creation of the Flask app
which means that the tests are no longer guaranteed to be threadsafe.

* Add SQLite journal file to git-ignore

* Make exception check consistent with other tests

* Fix inconsistent use of single vs double quotes

* Add TODO comments

* Replace % string formatting with .format

* Fix resetting of STORAGE_PROVIDER env var

* Refactor handling of environment variables

* Only cleanup objects explicitly created by test

* Split out EnvironmentMutationMixin

* Handle restore after multiple env var set calls

* Fix file lock error when running tests on Windows

* Respect pre-existing STORAGE_CONTAINER value

* Always require the storage container to pre-exist

* Ensure correct parsing of secrets with newlines

* Remove unused import

* List all required env vars in module docstring

* Revert unnecessary line change

* Pull-up expected file-name overrides

These will likely be the same for all cloud storage providers since AWS,
GCP, Azure, etc. all support folders.

colinschoen added a commit that referenced this pull request Jun 18, 2018

Add encrypted Google Cloud Storage env vars to CI
Adds three encrypted environment variables to travis which will
run the suite of Google Cloud Storage Integration tests (#1246) on
repo PRs (excluding forks)

Signed-off-by: Colin Schoen <cschoen@berkeley.edu>

colinschoen added a commit that referenced this pull request Jun 26, 2018

Add encrypted Google Cloud Storage env vars to CI (#1287)
* Add encrypted Google Cloud Storage env vars to CI

Adds three encrypted environment variables to travis which will
run the suite of Google Cloud Storage Integration tests (#1246) on
repo PRs (excluding forks)

Signed-off-by: Colin Schoen <cschoen@berkeley.edu>

* Move new environment variables to global section

Signed-off-by: Colin Schoen <cschoen@berkeley.edu>

* Fix secure env variable formatting

Signed-off-by: Colin Schoen <cschoen@berkeley.edu>

* Remove comment breaking travis config parsing

Signed-off-by: Colin Schoen <cschoen@berkeley.edu>

* Fix encrypted env var values

Signed-off-by: Colin Schoen <cschoen@berkeley.edu>

* Remove completed todo

Signed-off-by: Colin Schoen <cschoen@berkeley.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.