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

feat(securitycenter): Add Resource SCC Org Mgmt API SHA Custom Modules (GetEff, ListEff, ListDesc, Simulate) #13023

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

vijaykanthm
Copy link
Contributor

Description

Fixes b/347347705, b/347347992, b/347347268, b/347348030
This PR adds SCC Managament API Org SHA Custom Module Samples for GetEffective, ListEffective, ListDescendant & Simulate.

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

Checklist

@vijaykanthm vijaykanthm requested review from a team as code owners December 30, 2024 04:03
Copy link

snippet-bot bot commented Dec 30, 2024

Here is the summary of changes.

You are about to add 4 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@product-auto-label product-auto-label bot added api: securitycenter Issues related to the Security Command Center API. samples Issues that are directly related to samples. labels Dec 30, 2024
Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

Hi team,

I'm currently reviewing this pull request and will post my detailed review in a few minutes. In the meantime, here's a quick summary to help you and other reviewers get up to speed:

This pull request adds several new Python samples for interacting with the Security Health Analytics Custom Modules API. The intent is to provide comprehensive examples for common operations, addressing issues b/347347705, b/347347992, b/347347268, and b/347348030.

Here's a breakdown of the changes:

  • New files added:

    • securitycenter/snippets_management_api/noxfile_config.py: This file configures the testing environment for the new samples, specifying environment variables and dependencies.
    • securitycenter/snippets_management_api/requirements-test.txt and securitycenter/snippets_management_api/requirements.txt: These files list the necessary Python packages for testing and running the samples, respectively.
    • securitycenter/snippets_management_api/security_health_analytics_custom_modules.py: This file contains the core Python code for the samples, implementing functions for:
      • get_effective_security_health_analytics_custom_module
      • list_descendant_security_health_analytics_custom_module
      • list_effective_security_health_analytics_custom_module
      • simulate_security_health_analytics_custom_module
    • securitycenter/snippets_management_api/security_health_analytics_custom_modules_test.py: This file contains unit tests to verify the functionality of the samples.
  • Key Changes: The samples cover four main functions: getting, listing descendants, listing effective modules, and simulating custom modules. The code uses the google-cloud-securitycentermanagement library. The tests are set up to use a specific organization ID (defined in environment variables), and include robust error handling and retry mechanisms using backoff.

I'll be checking the code for clarity, efficiency, and adherence to best practices in my full review. I'll also verify that the tests are comprehensive and cover all edge cases.


And now, a little haiku to lighten the mood:

Code flows like a stream,
Tests catch bugs before they swim,
Smooth merge, all is well.

Thanks,
Gemini

Copy link

Gemini encountered an error creating the review. You can try again by commenting @code-review-assist review.

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

Hi team,

I'm currently reviewing this pull request and will post my detailed review in a few minutes. In the meantime, here's a quick summary to help you and other reviewers get up to speed:

This pull request adds several new Python samples for interacting with the Security Health Analytics custom modules API. The intent is to provide comprehensive examples for common tasks, addressing several internal Google bugs (b/347347705, b/347347992, b/347347268, b/347348030).

Here's a breakdown of the changes:

  • New files added:

    • securitycenter/snippets_management_api/noxfile_config.py: This file configures the testing environment for the new samples, specifying required environment variables and dependencies.
    • securitycenter/snippets_management_api/requirements-test.txt and securitycenter/snippets_management_api/requirements.txt: These files list the necessary Python packages for testing and running the samples, respectively. Noteworthy additions include google-cloud-securitycentermanagement and pytest.
    • securitycenter/snippets_management_api/security_health_analytics_custom_modules.py: This file contains the core sample code, implementing functions for:
      • get_effective_security_health_analytics_custom_module
      • list_descendant_security_health_analytics_custom_modules
      • list_effective_security_health_analytics_custom_modules
      • simulate_security_health_analytics_custom_module
    • securitycenter/snippets_management_api/security_health_analytics_custom_modules_test.py: This file includes unit tests to verify the functionality of the samples.
  • Key changes: The samples demonstrate how to get, list, and simulate Security Health Analytics custom modules, covering various scenarios and use cases. The tests are designed to ensure the samples function correctly.

I'll provide a more detailed review shortly, focusing on code style, error handling, and test coverage. In the meantime, please take a look at this summary and let me know if you have any questions.

And now, a little haiku to lighten the mood:

Code flows like a stream,
Tests run, bugs swim far away,
Clarity's sweet reward.

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

This pull request adds Security Health Analytics custom module samples. The code is well-structured and easy to follow. The use of try-except blocks for error handling is good. A few minor improvements are suggested below.

I've referenced the following elements from the Google Python Style Guide:

  • Function and method arguments should be documented using type annotations.
  • Docstrings should follow the three-part "Arrange, Act, Assert" structure.
  • Use descriptive names for functions and variables.
  • Use commonly used acronyms, or define acronyms where appropriate.


TEST_CONFIG_OVERRIDE = {
# You can opt out from the test for specific Python versions.
"ignored_versions": ["2.7", "3.7", "3.9", "3.10", "3.11"],

Choose a reason for hiding this comment

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

medium

Per the provided style guide, Python 3.9 should be tested. Is there a reason it's being skipped?

Suggested change
"ignored_versions": ["2.7", "3.7", "3.9", "3.10", "3.11"],
"ignored_versions": ["2.7", "3.7", "3.10", "3.11"],

# [START securitycenter_get_effective_security_health_analytics_custom_module]


def get_effective_security_health_analytics_custom_module(parent: str, module_id: str):

Choose a reason for hiding this comment

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

low

Per the Google Python Style Guide, consider using a more descriptive function name, even if it becomes long. How about get_effective_sha_custom_module?

Suggested change
def get_effective_security_health_analytics_custom_module(parent: str, module_id: str):
def get_effective_sha_custom_module(parent: str, module_id: str):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessary

Comment on lines 24 to 35
"""
Retrieves a Security Health Analytics custom module.
Args:
parent: Use any one of the following options:
- organizations/{organization_id}/locations/{location_id}
- folders/{folder_id}/locations/{location_id}
- projects/{project_id}/locations/{location_id}
Returns:
The retrieved Security Health Analytics custom module.
Raises:
NotFound: If the specified custom module does not exist.
"""

Choose a reason for hiding this comment

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

low

The docstring could be improved by following the "Arrange, Act, Assert" structure from the style guide. Consider adding a more detailed description of the function's purpose, arguments, return value, and potential exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

print(f"Retrieved Effective Security Health Analytics Custom Module: {response.name}")
return response
except NotFound as e:
print(f"Custom Module not found: {response.name}")

Choose a reason for hiding this comment

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

high

The variable response appears to be out of scope here. Did you mean to reference e instead?

Suggested change
print(f"Custom Module not found: {response.name}")
print(f"Custom Module not found: {e}")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

# [START securitycenter_list_descendant_security_health_analytics_custom_module]


def list_descendant_security_health_analytics_custom_module(parent: str):

Choose a reason for hiding this comment

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

low

Consider using a more descriptive function name, such as list_descendant_sha_custom_modules.

Suggested change
def list_descendant_security_health_analytics_custom_module(parent: str):
def list_descendant_sha_custom_modules(parent: str):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessary.

"display_name": display_name,
"enablement_state": "ENABLED",
"custom_config": {
"description": "Sample custom module for testing purpose. Please do not delete.",

Choose a reason for hiding this comment

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

Typo: "purpose" should be "purposes".

Suggested change
"description": "Sample custom module for testing purpose. Please do not delete.",
"description": "Sample custom module for testing purposes. Please do not delete.",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

Comment on lines 92 to 93
"title": "GCE Instance High Severity",
"description": "Custom module to detect high severity issues on GCE instances.",

Choose a reason for hiding this comment

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

medium

The title and description here seem to be related to GCE instances, but the rest of the configuration is for Cloud KMS. Should these be updated to reflect the actual resource type being evaluated?

Suggested change
"title": "GCE Instance High Severity",
"description": "Custom module to detect high severity issues on GCE instances.",
"title": "Cloud KMS CryptoKey Rotation Period",
"description": "Custom module to detect CryptoKeys with rotation period greater than 30 days.",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

"title": "GCE Instance High Severity",
"description": "Custom module to detect high severity issues on GCE instances.",
},
"recommendation": "Ensure proper security configurations on GCE instances.",

Choose a reason for hiding this comment

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

low

The recommendation should be more specific to Cloud KMS. Consider rephrasing it to something like: "Review and adjust the rotation period for Cloud KMS CryptoKeys."

Suggested change
"recommendation": "Ensure proper security configurations on GCE instances.",
"recommendation": "Review and adjust the rotation period for Cloud KMS CryptoKeys.",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

{
"name": "example_property",
"value_expression": {
"description": "The name of the instance",

Choose a reason for hiding this comment

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

low

The description should reflect Cloud KMS, not instances. Perhaps: "The resource name of the CryptoKey."

Suggested change
"description": "The name of the instance",
"description": "The resource name of the CryptoKey.",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

"description": "The name of the instance",
"expression": "resource.name",
"location": "global",
"title": "Instance Name",

Choose a reason for hiding this comment

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

low

The title should also be updated to reflect Cloud KMS. How about: "CryptoKey Resource Name"

Suggested change
"title": "Instance Name",
"title": "CryptoKey Resource Name",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

Hi team,

I'm currently reviewing this pull request and will post my detailed review in a few minutes. In the meantime, here's a quick summary to help you and other reviewers get up to speed:

This pull request, authored by vijaykanthm, adds several new Python samples for interacting with the Security Health Analytics Custom Modules API. The intent is to provide comprehensive examples for common operations, addressing several internal Google bugs (b/347347705, b/347347992, b/347347268, b/347348030).

Here's a breakdown of the changes:

  • New files added:
    • securitycenter/snippets_management_api/noxfile_config.py: Configuration file for testing.
    • securitycenter/snippets_management_api/requirements-test.txt: Test dependencies.
    • securitycenter/snippets_management_api/requirements.txt: Project dependencies.
    • securitycenter/snippets_management_api/security_health_analytics_custom_modules.py: Contains the core functions for interacting with the API (GetEffective, ListEffective, ListDescendant, Simulate).
    • securitycenter/snippets_management_api/security_health_analytics_custom_modules_test.py: Unit tests for the functions.
  • Key functionality: The new samples demonstrate how to:
    • Retrieve a single Security Health Analytics custom module using get_effective_security_health_analytics_custom_module.
    • List all descendant custom modules using list_descendant_security_health_analytics_custom_module.
    • List all effective custom modules (resident and inherited) using list_effective_security_health_analytics_custom_module.
    • Simulate the results of running a custom module against a resource using simulate_security_health_analytics_custom_module.

The changes include adding new functions, comprehensive docstrings, and unit tests to ensure code quality and functionality. The noxfile_config.py file includes specific environment variable configurations for the tests. The requirements.txt and requirements-test.txt files list the necessary dependencies.

I'll be checking for correctness, readability, and test coverage in my full review.


And now, a little haiku to brighten your day:

Code flows like a stream,
Tests ensure the path is clear,
A bug-free release.

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

This pull request adds Security Health Analytics custom module samples. The code is well-structured and easy to follow. The use of try-except blocks for error handling is good. A few minor improvements are suggested below.

I've referenced the following elements from the Google Python Style Guide:

  • Function docstrings should follow the Google Python Style Guide format.
  • Module level docstrings should be placed at the top of the file.
  • Use descriptive variable and function names.
  • Use commonly used variable names where appropriate (e.g. client for client objects).


# Default TEST_CONFIG_OVERRIDE for python repos.

# You can copy this file into your directory, then it will be inported from

Choose a reason for hiding this comment

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

Typo: "inported" should be "imported".

Suggested change
# You can copy this file into your directory, then it will be inported from
# You can copy this file into your directory, then it will be imported from

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

# [START securitycenter_get_effective_security_health_analytics_custom_module]


def get_effective_security_health_analytics_custom_module(parent: str, module_id: str):

Choose a reason for hiding this comment

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

low

The docstring could be improved by following the Google Python Style Guide format. It should include an Args section and a Returns section. Consider adding a Raises section as well.

Suggested change
def get_effective_security_health_analytics_custom_module(parent: str, module_id: str):
def get_effective_security_health_analytics_custom_module(parent: str, module_id: str):
"""
Retrieves a Security Health Analytics custom module.
Args:
parent (str): Resource name of the parent to retrieve the effective custom module from.
module_id (str): The unique identifier of the custom module.
Returns:
EffectiveSecurityHealthAnalyticsCustomModule: The retrieved effective custom module.
Raises:
google.api_core.exceptions.NotFound: If the custom module is not found.
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

Raises:
NotFound: If the specified custom module does not exist.
"""
client = securitycentermanagement_v1.SecurityCenterManagementClient()

Choose a reason for hiding this comment

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

Per the Google style guide, prefer using client as the variable name for client objects, where appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already doing it.

print(f"Retrieved Effective Security Health Analytics Custom Module: {response.name}")
return response
except NotFound as e:
print(f"Custom Module not found: {response.name}")

Choose a reason for hiding this comment

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

medium

The variable response is not defined within the except block. It should be defined in the try block before being used in the except block. It's better to print the exception message e instead.

Suggested change
print(f"Custom Module not found: {response.name}")
print(f"Custom Module not found: {e}")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

# [START securitycenter_list_descendant_security_health_analytics_custom_module]


def list_descendant_security_health_analytics_custom_module(parent: str):

Choose a reason for hiding this comment

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

low

The docstring could be improved by following the Google Python Style Guide format. It should include an Args section and a Returns section. Consider adding a Raises section as well.

Suggested change
def list_descendant_security_health_analytics_custom_module(parent: str):
def list_descendant_security_health_analytics_custom_module(parent: str):
"""
Retrieves a list of all descendant Security Health Analytics custom modules.
Args:
parent (str): Resource name of the parent to retrieve the descendant custom modules from.
Returns:
List[SecurityHealthAnalyticsCustomModule]: A list of descendant custom modules.
Raises:
google.api_core.exceptions.NotFound: If the parent resource is not found.
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

raise


def add_custom_module(org_id: str):

Choose a reason for hiding this comment

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

low

This function is missing a docstring explaining its purpose and parameters.

Suggested change
def add_custom_module(org_id: str):
def add_custom_module(org_id: str):
"""
Adds a new SHA custom module.
Args:
org_id (str): The organization ID.
Returns:
Tuple[str, str]: The name and ID of the created custom module.
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

@backoff.on_exception(
backoff.expo, (InternalServerError, ServiceUnavailable, NotFound), max_tries=3
)
def test_get_effective_security_health_analytics_custom_module():

Choose a reason for hiding this comment

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

low

This function is missing a docstring explaining its purpose.

Suggested change
def test_get_effective_security_health_analytics_custom_module():
def test_get_effective_security_health_analytics_custom_module():
"""Tests getting an effective SHA custom module."""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

@backoff.on_exception(
backoff.expo, (InternalServerError, ServiceUnavailable, NotFound), max_tries=3
)
def test_list_descendant_security_health_analytics_custom_module():

Choose a reason for hiding this comment

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

low

This function is missing a docstring explaining its purpose.

Suggested change
def test_list_descendant_security_health_analytics_custom_module():
def test_list_descendant_security_health_analytics_custom_module():
"""Tests listing descendant SHA custom modules."""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

@backoff.on_exception(
backoff.expo, (InternalServerError, ServiceUnavailable, NotFound), max_tries=3
)
def test_list_effective_security_health_analytics_custom_module():

Choose a reason for hiding this comment

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

low

This function is missing a docstring explaining its purpose.

Suggested change
def test_list_effective_security_health_analytics_custom_module():
def test_list_effective_security_health_analytics_custom_module():
"""Tests listing effective SHA custom modules."""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

@backoff.on_exception(
backoff.expo, (InternalServerError, ServiceUnavailable, NotFound), max_tries=3
)
def test_simulate_security_health_analytics_custom_module():

Choose a reason for hiding this comment

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

low

This function is missing a docstring explaining its purpose.

Suggested change
def test_simulate_security_health_analytics_custom_module():
def test_simulate_security_health_analytics_custom_module():
"""Tests simulating an SHA custom module."""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

Hi team,

I'm currently reviewing this pull request and will post my detailed review in a few minutes. In the meantime, here's a quick summary to help you and other reviewers get up to speed:

This pull request adds several new Python samples for interacting with the Security Health Analytics Custom Modules API. The goal is to provide comprehensive examples for common operations, addressing issues b/347347705, b/347347992, b/347347268, and b/347348030.

Here's a breakdown of the changes:

  • New files:

    • securitycenter/snippets_management_api/noxfile_config.py: Added configuration for testing the samples. Lines 1-41 define the test configuration override, specifying ignored Python versions, environment variables, and project details.
    • securitycenter/snippets_management_api/requirements-test.txt and securitycenter/snippets_management_api/requirements.txt: Added necessary dependencies for testing and running the samples. Lines 1-4 in requirements-test.txt and lines 1-4 in requirements.txt specify the packages.
    • securitycenter/snippets_management_api/security_health_analytics_custom_modules.py: Contains the core sample code. Lines 1-200 implement functions for get_effective, list_descendant, list_effective, and simulate operations on Security Health Analytics custom modules.
    • securitycenter/snippets_management_api/security_health_analytics_custom_modules_test.py: Added comprehensive unit tests for the new samples. Lines 1-206 implement the test functions, including setup and cleanup, and using backoff for retry logic.
  • Key improvements: The samples cover a range of functionalities, including retrieving, listing, and simulating custom modules. They are well-documented and include comprehensive error handling.

Let me know if you have any questions.

And now, a little haiku to lighten the mood:

Code flows like a stream,
Tests run, bugs flee in the night,
Clean code, joy is found.

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

This pull request adds Security Health Analytics custom module samples. Overall, the code is well-structured and easy to understand. The addition of type hints and docstrings improves readability and maintainability. The inclusion of tests is also a good practice. Here are a few suggestions based on PEP 8 and Google Python Style Guide that could further enhance the code:

  • In noxfile_config.py, the GCLOUD_ORGANIZATION should not be hardcoded. It's better to retrieve it from the environment or use a placeholder value for testing.
  • The try...except blocks in the list_descendant_security_health_analytics_custom_module and list_effective_security_health_analytics_custom_module functions are too broad. It's better to catch specific exceptions and handle them accordingly.
  • The resource_type in the simulate_security_health_analytics_custom_module function should be parameterized or retrieved from the custom module configuration.
  • The checklist in the pull request description is not fully completed. Please review and complete the checklist before merging.
  • It is good practice to delete any resources created during testing in the teardown method of the test file. This can be done by calling the appropriate delete methods for each resource in the cleanup_existing_custom_modules function. This ensures that resources are not left behind after the tests are completed and helps to avoid unexpected charges.


# Default TEST_CONFIG_OVERRIDE for python repos.

# You can copy this file into your directory, then it will be inported from

Choose a reason for hiding this comment

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

Typo: "inported" should be "imported".

Suggested change
# You can copy this file into your directory, then it will be inported from
# the noxfile.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

# A dictionary you want to inject into your test. Don't put any
# secrets here. These values will override predefined values.
"envs": {
"GCLOUD_ORGANIZATION": "1081635000895",

Choose a reason for hiding this comment

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

medium

Per the Google Python Style Guide, it's recommended to not hardcode sensitive information like organization IDs directly in the code. Consider retrieving this value from the environment or using a placeholder for testing purposes. Refer to Secrets in AUTHORING_GUIDE.md for more details on how to handle secrets.

Suggested change
"GCLOUD_ORGANIZATION": "1081635000895",
"GCLOUD_ORGANIZATION": os.environ.get("GCLOUD_ORGANIZATION", "YOUR_ORGANIZATION_ID"),

print(f"Retrieved Effective Security Health Analytics Custom Module: {response.name}")
return response
except NotFound as e:
print(f"Custom Module not found: {response.name}")

Choose a reason for hiding this comment

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

medium

The variable response might not be defined within this except block if the NotFound exception is raised before the API call. Consider using a more specific try...except block around the API call itself, or handling the exception differently. According to PEP 8, it's a good practice to handle exceptions as close to the source of the error as possible.

Suggested change
print(f"Custom Module not found: {response.name}")
print(f"Custom Module not found: {e.message}")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

Comment on lines +85 to +86
except Exception as e:
print(f"An error occurred while listing custom modules: {e}")

Choose a reason for hiding this comment

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

medium

Catching a generic Exception can mask unexpected errors. PEP 8 recommends catching specific exceptions whenever possible. Could you specify the type of exceptions you expect in this block, or provide more context? This will make debugging easier and improve the overall robustness of the code.

Suggested change
except Exception as e:
print(f"An error occurred while listing custom modules: {e}")
print(f"An error occurred while listing custom modules: {e}")
raise # Re-raise the exception after printing the message

Comment on lines +127 to +128
except Exception as e:
print(f"An error occurred while listing custom modules: {e}")

Choose a reason for hiding this comment

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

medium

Catching a generic Exception can mask unexpected errors. PEP 8 recommends catching specific exceptions whenever possible. Could you specify the type of exceptions you expect in this block, or provide more context? This practice enhances code clarity and aids in debugging.

Suggested change
except Exception as e:
print(f"An error occurred while listing custom modules: {e}")
print(f"An error occurred while listing custom modules: {e}")
raise # Re-raise the exception after printing the message


# Initialize request argument(s)
resource = securitycentermanagement_v1.types.SimulateSecurityHealthAnalyticsCustomModuleRequest.SimulatedResource()
resource.resource_type = "cloudkms.googleapis.com/CryptoKey" # Replace with the correct resource type

Choose a reason for hiding this comment

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

medium

The resource type is hardcoded here. Consider making this a parameter to the function or retrieving it from the custom_config. This would make the function more flexible and reusable. For example, you could add a resource_type field to the custom_config dictionary and retrieve it within the function.

Suggested change
resource.resource_type = "cloudkms.googleapis.com/CryptoKey" # Replace with the correct resource type
resource.resource_type = custom_config.get("resource_selector", {}).get("resource_types", ["YOUR_RESOURCE_TYPE"])[0]

pytest.fail("GCLOUD_ORGANIZATION environment variable is not set.")

print(f"Cleaning up existing custom modules for organization: {ORGANIZATION_ID}")
cleanup_existing_custom_modules(ORGANIZATION_ID)

Choose a reason for hiding this comment

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

medium

It's a good practice to clean up any resources created during testing. Consider adding a finalizer or using try...finally to ensure cleanup happens even if tests fail. This can prevent orphaned resources and reduce costs. You can delete the created custom module in the cleanup_existing_custom_modules function.

Suggested change
cleanup_existing_custom_modules(ORGANIZATION_ID)
cleanup_existing_custom_modules(ORGANIZATION_ID)

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

Hi team,

I'm currently reviewing this pull request and will post my detailed review in a few minutes. In the meantime, here's a quick summary to help you and other reviewers get up to speed:

This pull request adds Security Health Analytics custom module samples for the Security Command Center (SCC) Management API. The intent is to provide code examples for four key operations:

  • GetEffective: Retrieve a specific Security Health Analytics custom module.
  • ListEffective: List all Security Health Analytics custom modules (resident and inherited).
  • ListDescendant: List all resident custom modules and their descendants.
  • Simulate: Simulate the result of running a custom module against a resource.

The changes include:

  • Addition of a new securitycenter/snippets_management_api directory containing the sample code.
  • Updates to noxfile_config.py to configure the testing environment (lines 1-41).
  • Addition of necessary library dependencies in requirements.txt and requirements-test.txt (lines 1-4 and 1-4 respectively).
  • Creation of security_health_analytics_custom_modules.py with the core functions (lines 1-200).
  • Implementation of comprehensive unit tests in security_health_analytics_custom_modules_test.py to ensure functionality (lines 1-206).

The PR addresses several internal Google bugs (b/347347705, b/347347992, b/347347268, b/347348030).

I'll provide a more detailed review shortly, but this should give you a good overview in the meantime.

And now, a little haiku to brighten your day:

Code flows like a stream,
Tests run, bugs flee in the night,
Clean code, joy is found.

Best,
Gemini

Copy link

Gemini encountered an error creating the review. You can try again by commenting @code-review-assist review.

@rsamborski rsamborski added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 7, 2025
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 7, 2025
@vijaykanthm vijaykanthm added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 13, 2025
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 13, 2025
@vijaykanthm vijaykanthm added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 13, 2025
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: securitycenter Issues related to the Security Command Center API. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants