-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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(security-center): Add Resource v2 API Assets Security Marks Samples #9680
base: main
Are you sure you want to change the base?
Conversation
Here is the summary of changes. You are about to add 3 region tags.
This comment is generated by snippet-bot.
|
import com.google.protobuf.FieldMask; | ||
import java.io.IOException; | ||
|
||
//[START securitycenter_add_delete_security_marks_assets_v2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
place region tags to enclose necessary imports and the code sample method. no need to leave space lines between the region tags and the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
public class AddDeleteSecurityMarks { | ||
public static void main(String[] args) throws IOException { | ||
// organizationId: Google Cloud Organization id. | ||
String organizationId = "{google-cloud-organization-id}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please align the style of the ID with other code samples. No need for curly brackets. Most of code samples use capitalized expression like PROJECT_ID or ORGANIZATION_ID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
String organizationId = "{google-cloud-organization-id}"; | ||
|
||
// Specify the finding-id. | ||
String assetId = "{asset-id}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. note that "finding-id" is not used anywhere. consider to refactor the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
// Specify the location. | ||
String location = "global"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does your code sample support other locations? if not, please use this literal inside the code sample method instead of using it as parameter. if a user can use different locations, provide a link to documentation that enumerates these locations in the comment for this argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
// Demonstrates adding/updating at the same time as deleting security | ||
// marks from an asset. | ||
// To add or change security marks, you must have an IAM role that includes permission: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for comments here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
final PrintStream out = System.out; | ||
stdOut = new ByteArrayOutputStream(); | ||
System.setOut(new PrintStream(stdOut)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use return values instead of capturing and parsing stdout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
stdOut = new ByteArrayOutputStream(); | ||
System.setOut(new PrintStream(stdOut)); | ||
|
||
requireEnvVar("GOOGLE_APPLICATION_CREDENTIALS"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you can skip this check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
// Fetch a valid asset ID dynamically | ||
try (SecurityCenterClient client = SecurityCenterClient.create()) { | ||
OrganizationName orgName = OrganizationName.of(ORGANIZATION_ID); | ||
ListAssetsRequest request = | ||
ListAssetsRequest.newBuilder().setParent(orgName.toString()).setPageSize(1).build(); | ||
|
||
Asset asset = client.listAssets(request).iterateAll().iterator().next().getAsset(); | ||
assetName = asset.getName(); // Get the full resource name for the asset | ||
assetId = extractAssetId(assetName); | ||
} catch (InvalidArgumentException e) { | ||
System.err.println("Error retrieving asset ID: " + e.getMessage()); | ||
throw e; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setup method is used to create resources. you cannot assume that a resource exists before the tests are executed. in rare occasions when tests use pre-provisioned resources (e.g. because provisioning a resource takes too long), all required attributes of the resource are hardcoded and expressed via environment variables in the testing environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
|
||
stdOut = null; | ||
System.setOut(out); | ||
TimeUnit.MINUTES.sleep(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this delay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
stdOut = new ByteArrayOutputStream(); | ||
System.setOut(new PrintStream(stdOut)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do not capture stdout stream
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
af64a50
to
e4b8d95
Compare
security-command-center/snippets/src/main/java/vtwo/assets/AddSecurityMarksToAssets.java
Show resolved
Hide resolved
security-command-center/snippets/src/main/java/vtwo/assets/AddDeleteSecurityMarks.java
Show resolved
Hide resolved
security-command-center/snippets/src/main/java/vtwo/assets/DeleteAssetsSecurityMarks.java
Show resolved
Hide resolved
security-command-center/snippets/src/test/java/vtwo/AssetSecurityMarksIT.java
Show resolved
Hide resolved
security-command-center/snippets/src/test/java/vtwo/AssetSecurityMarksIT.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review and address the comments.
Please provide explanation about differences between AddDeleteSecurityMarks
and AddSecurityMarksToAssets
code samples.
// organizationId: Google Cloud Organization id. | ||
String organizationId = "ORGANIZATION_ID"; | ||
|
||
// Specify the asset id. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO (actionable comments) are required here to instruct a reader how to create a working example.
See this main()
method for an example.
import com.google.protobuf.FieldMask; | ||
import java.io.IOException; | ||
|
||
public class AddDeleteSecurityMarks { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example name is confusing. Adding and deleting marks in the same action is contradicting. Consider changing the name to UpdateSecurityMarks
. If you want to demonstrate something else, change the name accordingly.
public class AddSecurityMarksToAssets { | ||
|
||
public static void main(String[] args) throws IOException { | ||
// organizationId: Google Cloud Organization id. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO (actionable comments) are required here to instruct a reader how to create a working example.
String assetName = String.format("organizations/%s/assets/%s", organizationId, assetId); | ||
|
||
// Start setting up a request to add security marks for a finding. | ||
ImmutableMap markMap = ImmutableMap.of("key_a", "value_a", "key_b", "value_b"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: consider passing the collection of key/value strings as an argument in order to make the example more usable.
|
||
@Rule | ||
public final MultipleAttemptsRule multipleAttemptsRule = | ||
new MultipleAttemptsRule(3, 120000); // 2 minutes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 minutes per test is too long. please consider to reduce the maximal time of execution.
@Before | ||
public void beforeEach() { | ||
stdOut = new ByteArrayOutputStream(); | ||
} | ||
|
||
@After | ||
public void afterEach() { | ||
stdOut = null; | ||
System.setOut(null); | ||
} | ||
|
||
@AfterClass | ||
public static void cleanUp() { | ||
System.setOut(System.out); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this code since code samples do not print to stdout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why to store so much information in order to retrieve a (supposedly) fixed asset ID?
Description
This PR adds v2 API Assets Security Marks Java client samples to Add Security Marks, Delete Security Marks, Add Delete Security Marks.
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
pom.xml
parent set to latestshared-configuration
mvn clean verify
requiredmvn -P lint checkstyle:check
requiredmvn -P lint clean compile pmd:cpd-check spotbugs:check
advisory only