-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Support security posture config #8185
Support security posture config #8185
Conversation
Oops! It looks like you're using an unknown release-note type in your changelog entries:
Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md. |
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @zli82016, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 5 files changed, 174 insertions(+), 4 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerCluster_withSecurityPostureConfig|TestAccComputeFirewallPolicyRule_multipleRules|TestAccComputeNetworkEndpoints_networkEndpointsBasic |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
The failed test seems not related to my changes? My changes are tested only in I have also updated the release note. |
Right, the failed test is not related to this PR. |
Thanks @zli82016 Anything else I should do at this point? |
Thanks for working on it. Not now. I will review it and give you update soon. |
Hello, @DDDDarrenWB , I am a little confused about this PR. I may miss something. In the API for the resource cluster, I do not see the two new fields. https://cloud.google.com/kubernetes-engine/docs/reference/rest/v1beta1/projects.locations.clusters Also, terraform-google-modules/terraform-google-kubernetes-engine#1586 is already closed. In this issue, it requested to add |
Hi @zli82016
|
Thanks for the information. |
"security_posture": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
Computed: true, |
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.
I wonder what is the reason to set Computed: true
here? Computed: true
is used in the case that the API will returns default value if no value is specified in the request.
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.
Good question! I couldn't pass the Acceptance test locally if I don't set the Computed:true
. Although, it didn't return error when I did manual test without Computed:true
. I really could use your advice on this.
Could it be the configurations I have in my local development 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.
Did you get any error in the logs when running the acceptance test locally without Computed:true
?
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.
Yes.
Error log like below,
error=
| After applying this test step, the plan was not empty.
| stdout:
|
|
| Terraform used the selected providers to generate the following execution
| plan. Resource actions are indicated with the following symbols:
| ~ update in-place
|
| Terraform will perform the following actions:
|
| # google_container_cluster.with_security_posture_config will be updated in-place
| ~ resource "google_container_cluster" "with_security_posture_config" {
| id = "projects/wangbei-gke-dev/locations/us-central1-c/clusters/tf-test-cluster-6mwho946jg"
| name = "tf-test-cluster-6mwho946jg"
| - workload_vulnerability_scanning = "DISABLED" -> null
| # (25 unchanged attributes hidden)
|
| # (16 unchanged blocks hidden)
| }
|
| Plan: 0 to add, 1 to change, 0 to destroy.
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.
I think the Computed: true
is needed.
I want to make the behavior of terraform matches to how user will use If the rule of Terraform is to make it similar to API, I'll also be happy to change my codes. |
I see. In Terraform, we will make it similar to API. |
"workload_vulnerability_scanning": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
Computed: true, |
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.
I wonder what is the reason to set Computed: true 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.
Computed: true is needed. Sorry for the confusion.
"security_posture": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
Computed: true, |
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.
Did you get any error in the logs when running the acceptance test locally without Computed:true
?
Also, in Terraform, the field name will match the name in the API. For example, the field will look like:
|
Understood. I'll make the change. |
06822b2
to
4401914
Compare
reopen due to false close. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 5 files changed, 174 insertions(+), 2 deletions(-)) |
mmv1/third_party/terraform/services/container/resource_container_cluster.go.erb
Outdated
Show resolved
Hide resolved
Tests analyticsTotal tests: Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerCluster_withSecurityPostureConfig|TestAccComputeNetworkEndpoints_networkEndpointsBasic|TestAccComputeFirewallPolicyRule_multipleRules |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 5 files changed, 174 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeFirewallPolicyRule_multipleRules|TestAccComputeNetworkEndpoints_networkEndpointsBasic |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
I run the test |
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.
LGTM. Thanks.
* support security posture config in the same format as the API * move create code up
* support security posture config in the same format as the API * move create code up
Add support of GKE Security Posture Config.
security_posture_config.mode
field in schema.security_posture_config.vulnerability_mode
field in schema.They both talks to GKE API - Security Posture Config.
The behavior of terraform matches with how user will use
gcloud
to manage our feature.Fixes hashicorp/terraform-provider-google#14973
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
in the generated providers to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)