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.

Sign up for GitHub

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

Jump to bottom

tests(monitoring): refresh VPCSC tests #9437

Merged
merged 3 commits into from
Oct 24, 2019
Merged

tests(monitoring): refresh VPCSC tests #9437

merged 3 commits into from
Oct 24, 2019

Conversation

steinwaywhw
Copy link
Contributor

I have tested these locally but I needed to modify noxfile in order to pass in the correct environment variables. Let's see how it runs on pull requests.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 9, 2019
@steinwaywhw
Copy link
Contributor Author

The VPCSC tests failed because PROJECT_ID is "". Do you think we should check "" in the test itself, or use NONE in the noxfile? I'm open to either option.

@busunkim96
Copy link
Contributor

Hi @steinwaywhw! Could you modify tests to be similar to what's in #9389? There's more info in this doc.

Thank you!

@steinwaywhw
Copy link
Contributor Author

Hi Bu Sun, #9389 disables the tests when run outside of VPCSC. Is it decided that when not inside VPCSC, the tests should not be run at all? The current code has valid test cases when run outside of VPCSC. So shall I disable these tests when running outside of VPCSC, or should we work together to supply proper env variables to make it work outside of VPCSC?

@busunkim96
Copy link
Contributor

If a test is checking behavior dependent on being inside/outside a VPCSC that should only be run in VPCSC. If you have tests that don't require that environment please remove the skipif. I think both the VPCSC team and the client library team agree that we'd prefer for as many tests to run here as possible.

What sort of env vars were you looking to set?

@steinwaywhw
Copy link
Contributor Author

steinwaywhw commented Oct 10, 2019 •
edited
Loading

In the code there are two sets of tests. One set is run from inside VPCSC, one set is run from outside of VPCSC. For both sets, one half are sending requests to PROJECT_ID, the other half are sending requests to GOOGLE_CLOUD_TESTS_VPCSC_OUTSIDE_PERIMETER_PROJECT. For all tests, if any of these two env vars is missing, the test will skip.

The failing reason is, however, unrelated. It is because the PROJECT_ID is set to an empty string "". But the check in the code is looking for None. Therefore it fails to skip the tests. The solution is:

  • Option 1: Either change noxfile to set PROJECT_ID: None in the env, or change the code to treat empty string as None.
  • Option 2: Set PROJECT_ID to a project ID that is inside the VPCSC when running inside VPCSC perimeter. Let's just say this project id is "inside_project". When running outside of VPCSC, the PROJECT_ID should still be "inside_project" since the tests will send requests to "inside_project" and expecting the requests to be rejected by the VPCSC perimeter.

If the goal is to run as many tests as possible, then we should go with Option 2.

@busunkim96
Copy link
Contributor

In the code there are two sets of tests. One set is run from inside VPCSC, one set is run from outside of VPCSC. For both sets, one half are sending requests to PROJECT_ID, the other half are sending requests to GOOGLE_CLOUD_TESTS_VPCSC_OUTSIDE_PERIMETER_PROJECT. For all tests, if any of these two env vars is missing, the test will skip.

Ah sorry, I misspoke when I said "in VPCSC". What I wanted to distinguish is the CI environment/project used here by the client libraries team (no VPC or VPC service controls) and the CI environment used by the VPC-SC team (which runs the tests inside a VPC). It looks to me like all the monitoring tests can only run correctly in the VPC-SC team's environment?

The failing reason is, however, unrelated. It is because the PROJECT_ID is set to an empty string "". But the check in the code is looking for None. Therefore it fails to skip the tests.

The current noxfile sets PROJECT_ID to an empty string, but that can be removed with the modifications I'd was looking to make with #9389.

IS_INSIDE_VPCSC = "GOOGLE_CLOUD_TESTS_IN_VPCSC" in os.environ

# If IS_INSIDE_VPCSC is set, these environment variables should also be set
if IS_INSIDE_VPCSC:
    PROJECT_INSIDE = os.environ["PROJECT_ID"]
    PROJECT_OUTSIDE = os.environ["GOOGLE_CLOUD_TESTS_VPCSC_OUTSIDE_PERIMETER_PROJECT"]

Test skip is dependent on GOOGLE_CLOUD_TESTS_IN_VPCSC rather than two separate variables.

    @pytest.mark.skipif(
        not IS_INSIDE_VPCSC,
        reason="This test must be run in VPCSC. To enable this test, set the environment variable GOOGLE_CLOUD_TESTS_IN_VPCSC to True",
    )

@tseaver tseaver changed the title Refresh VPCSC tests for monitoring v3 tests(monitoring): refresh VPCSC tests Oct 10, 2019
tseaver
tseaver reviewed Oct 10, 2019
View reviewed changes
monitoring/tests/system/test_vpcsc_v3.py
#
# DO NOT MODIFY! THIS FILE IS AUTO-GENERATED.
# This file is auto-generated on 09 Oct 19 18:09 UTC.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file does not appear to be part of autogen for monitoring.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been told that the monitoring team has their own script to generate these tests. @steinwaywhw Could this comment be more specific about the source?

Copy link
Contributor Author

@steinwaywhw steinwaywhw Oct 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tseaver, this file is auto-generated from service protobufs using a script that the monitoring team developed. If you'd like, I can change the comment. Do you have a style that I can follow?

monitoring/tests/system/test_vpcsc_v3.py
from google.api_core import exceptions
from google.cloud import monitoring_v3
from google.cloud.monitoring_v3 import enums

PROJECT_INSIDE = os.environ.get("PROJECT_ID", None)
if not PROJECT_INSIDE:
PROJECT_INSIDE = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is necessary for the case that the environment variable is set to an empty string: @busunkim96 added it explicitly in #8302.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update the script to treat empty string as None.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the "skipIf" to "not PROJECT_INSIDE" and "not PROJECT_OUTSIDE" correspondingly, so we don't need this two lines anymore.

monitoring/tests/system/test_vpcsc_v3.py Outdated
@@ -47,8 +43,10 @@ def _is_rejected(call):
# instance, or None.
list(responses)
except exceptions.PermissionDenied as e:
print(e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No print statements, please: use logging if absolutely needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to logger.debug(e).

monitoring/tests/system/test_vpcsc_v3.py Outdated Show resolved Hide resolved
monitoring/tests/system/test_vpcsc_v3.py
name_outside = client.alert_policy_path(PROJECT_OUTSIDE, "mock_alert_policy")
delayed_outside = lambda: client.update_alert_policy({"name": name_outside})
TestVPCServiceControlV3._do_test(delayed_inside, delayed_outside)
self._do_test(delayed_inside, delayed_outside)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this PR delete this test? I see that it is moved below: is there a reason for reordering the testcases here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No particular reason. It is just because the file is auto-generated. I can sort the input endpoints to the script and make it deterministic from now on.

monitoring/tests/system/test_vpcsc_v3.py
delayed_outside = lambda: client.list_time_series(
name_outside, "", {}, enums.ListTimeSeriesRequest.TimeSeriesView.FULL
)
TestVPCServiceControlV3._do_test(delayed_inside, delayed_outside)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this PR delete this test? I see that it is moved below: is there a reason for reordering the testcases here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the script to arrange tests in a deterministic order by sorting endpoint versions, then services, then methods. Should be good now.

monitoring/tests/system/test_vpcsc_v3.py
PROJECT_OUTSIDE, "mock_notification_channel"
)
delayed_outside = lambda: client.delete_notification_channel(name_outside)
TestVPCServiceControlV3._do_test(delayed_inside, delayed_outside)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this PR delete this test? I see that it is moved below: is there a reason for reordering the testcases here?

monitoring/tests/system/test_vpcsc_v3.py
delayed_outside = lambda: client.get_notification_channel_verification_code(
name_outside
)
self._do_test(delayed_inside, delayed_outside)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so this one is a new test.

monitoring/tests/system/test_vpcsc_v3.py
delayed_outside = lambda: client.send_notification_channel_verification_code(
name_outside
)
self._do_test(delayed_inside, delayed_outside)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so this one is a new test.

monitoring/tests/system/test_vpcsc_v3.py
delayed_inside = lambda: client.create_uptime_check_config(name_inside, {})
name_outside = client.project_path(PROJECT_OUTSIDE)
delayed_outside = lambda: client.create_uptime_check_config(name_outside, {})
TestVPCServiceControlV3._do_test(delayed_inside, delayed_outside)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this PR delete this test? I see that it is moved below: is there a reason for reordering the testcases here?

@steinwaywhw
Copy link
Contributor Author

It looks to me like all the monitoring tests can only run correctly in the VPC-SC team's environment?

Hi @busunkim96, the test is able to run from any environment as long as you set PROJECT_ID to some random project within some random VPCSC perimeter, and GOOGLE_CLOUD_TESTS_VPCSC_OUTSIDE_PERIMETER_PROJECT to some random project outside of any VPCSC perimeter. Indeed I ran the tests on my own machine before the PR using PROJECT_ID = secure_gcp_test_project_4 and GOOGLE_CLOUD_TESTS_VPCSC_OUTSIDE_PERIMETER_PROJECT my random google cloud project.

This all goes back to the previous comment:

In the code there are two sets of tests. One set is run from inside VPCSC, one set is run from outside of VPCSC. For both sets, one half are sending requests to PROJECT_ID, the other half are sending requests to GOOGLE_CLOUD_TESTS_VPCSC_OUTSIDE_PERIMETER_PROJECT. For all tests, if any of these two env vars is missing, the test will skip.

The first set of tests mentioned above has to be run in VPCSC teams dedicated environment. The second set could be run anywhere, including the pull request checks environment that your team owns, as long as you supply the needed PROJECT_ID and GOOGLE_CLOUD_TESTS_VPCSC_OUTSIDE_PERIMETER_PROJECT to the tests.

So my question is, do you intend to run these tests in the PR presubmit checks?

I feel like we were not on the same page. I hope this comment clarifies things. Feel free to find me on chat if you have any questions or concerns.

@steinwaywhw
Rename to test_vpcsc_v3.py
f94aa2c
@steinwaywhw
Refresh VPCSC tests for monitoring v3
b652b14 
I have tested these locally but I needed to modify `noxfile` in order to pass in the correct environment variables. Let's see how it runs on pull requests.
@steinwaywhw
Copy link
Contributor Author

Sorry, I'm not sure why the diff page does not know it is a git mv even though I did it ...

@busunkim96
Copy link
Contributor

Hi Hanwen - I looked at the project and documentation you mentioned in chat. As the tests are written now, I don't think there's much to gain from running them in our presubmits. The VPC-SC team is already running the tests with projects in VPC, so there's no need to duplicate the same setup here.

If some tests made valid requests to the backend, I would be open to running them here since they would make the tests more robust. But since these tests are exclusively checking that a request is forbidden/permitted via the status code, they wouldn't be able to catch errors that aren't specific to VPC-SC.

Also happy to grab some time next week to chat over GVC if you have other thoughts.

Thanks!

@steinwaywhw
Copy link
Contributor Author

That's ok to me. The code as it is right now in this PR will skip all tests since the CI will pass PROJECT_ID = "". So it is safe to merge them now.

@steinwaywhw
Copy link
Contributor Author

friendly ping

busunkim96
busunkim96 approved these changes Oct 24, 2019
View reviewed changes
Copy link
Contributor

@busunkim96 busunkim96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging now, but will send a follow up PR to tweak some of the setup (it will not impact the actual test runs)

@busunkim96 busunkim96 merged commit 35ba046 into googleapis:master Oct 24, 2019
@steinwaywhw
Copy link
Contributor Author

Thank you, Bu Sun!

@tseaver tseaver mentioned this pull request Oct 29, 2019
Closed
2 tasks
parthea pushed a commit that referenced this pull request Oct 21, 2023
@steinwaywhw @busunkim96
tests(monitoring): refresh VPCSC tests (#9437)
e262172
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tseaver tseaver tseaver left review comments

@busunkim96 busunkim96 busunkim96 approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@steinwaywhw @busunkim96 @tseaver @googlebot