-
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
Private CA - Create expander for X509Config #5368
Conversation
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. Thanks for your contribution! A human will be with you soon. @c2thorn, please review this PR or find an appropriate assignee. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 8 files changed, 387 insertions(+), 1179 deletions(-)) |
@@ -54,6 +54,21 @@ overrides: !ruby/object:Overrides::ResourceOverrides | |||
kms_key_name: 'BootstrapKMSKeyWithPurposeInLocation(t, "ASYMMETRIC_SIGN", "us-central1").CryptoKey.Name' | |||
pool_name: 'BootstrapSharedCaPoolInLocation(t, "us-central1")' | |||
pool_location: "\"us-central1\"" | |||
virtual_fields: |
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.
Virtual fields are a possible way to make this work, but it seems preferable to have the fields in the same object as the is_ca and max_path_length fields. Do you think it is better to have them at the top level?
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.
are these top-level fields on the CA resource?
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.
Virtual fields are top-level only. They could be added to the sub object itself if done a slightly different way
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 have an example of how to add this as a virtual-like field as a subject object?
There is not a significant benefit to having these as top-level objects, but it seemed like virtual fields were the simplest approach.
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 don't have an example off hand, but any field that is marked as url_param_only
should not be sent in the request body and not read from an API response
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.
We've updated the PR to use url_param_only
, please take a look!
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.
(left initial feedback on first few files, will look at the rest soon)
@@ -54,6 +54,21 @@ overrides: !ruby/object:Overrides::ResourceOverrides | |||
kms_key_name: 'BootstrapKMSKeyWithPurposeInLocation(t, "ASYMMETRIC_SIGN", "us-central1").CryptoKey.Name' | |||
pool_name: 'BootstrapSharedCaPoolInLocation(t, "us-central1")' | |||
pool_location: "\"us-central1\"" | |||
virtual_fields: |
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.
are these top-level fields on the CA resource?
mmv1/templates/terraform/custom_expand/privateca_certificate_509_config.go.erb
Show resolved
Hide resolved
/gcbrun |
Failed tests from |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 8 files changed, 372 insertions(+), 1179 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 10 files changed, 380 insertions(+), 1179 deletions(-)) |
@@ -54,6 +54,21 @@ overrides: !ruby/object:Overrides::ResourceOverrides | |||
kms_key_name: 'BootstrapKMSKeyWithPurposeInLocation(t, "ASYMMETRIC_SIGN", "us-central1").CryptoKey.Name' | |||
pool_name: 'BootstrapSharedCaPoolInLocation(t, "us-central1")' | |||
pool_location: "\"us-central1\"" | |||
virtual_fields: |
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 don't have an example off hand, but any field that is marked as url_param_only
should not be sent in the request body and not read from an API response
In order to handle the case of optional primitive fields, I've added virtual fields to allow the expander to distinguish between an unset primitive and a primitive set to the default value. Since some Private CA resources do not support updates, I also added support for setting ForceNew in the terraform config.
This prevents incompatiable configs that set the allow* booleans without setting basic constraints, and vice versa.
19b06e6
to
8d5d884
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 10 files changed, 380 insertions(+), 1179 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccAssuredWorkloadsWorkload_FullHandWritten|TestAccAssuredWorkloadsWorkload_BasicHandWritten|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeGlobalForwardingRule_privateServiceConnectGoogleApisExample|TestAccComputeInstanceFromMachineImage_basic|TestAccComputeInstanceFromMachineImage_overrideMetadataDotStartupScript|TestAccComputeInstanceFromMachineImage_diffProject|TestAccComputeRegionHealthCheck_tcp_update|TestAccComputeRegionHealthCheck_typeTransition|TestAccComputeRegionHealthCheck_logConfigDisabled|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccComputeServiceAttachment_serviceAttachmentBasicExampleUpdate|TestAccContainerCluster_withILBSubsetting|TestAccContainerCluster_regionalWithNodePool|TestAccContainerCluster_withNodePoolBasic|TestAccContainerCluster_withNodePoolUpdateVersion|TestAccContainerCluster_withNodePoolResize|TestAccContainerCluster_withNodePoolAutoscaling|TestAccContainerCluster_withNodePoolMultiple|TestAccContainerCluster_withNodePoolNodeConfig|TestAccContainerCluster_withEnableKubernetesAlpha|TestAccContainerNodePool_basic|TestAccContainerNodePool_nodeLocations|TestAccContainerNodePool_basicWithClusterId|TestAccContainerNodePool_maxPodsPerNode|TestAccContainerNodePool_withNodeConfig|TestAccContainerNodePool_withWorkloadIdentityConfig|TestAccContainerNodePool_withSandboxConfig|TestAccContainerNodePool_withKubeletConfig|TestAccContainerNodePool_withLinuxNodeConfig|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccContainerNodePool_withGPU|TestAccContainerNodePool_withManagement|TestAccContainerNodePool_withNodeConfigScopeAlias|TestAccContainerNodePool_regionalAutoscaling|TestAccContainerNodePool_autoscaling|TestAccContainerNodePool_resize|TestAccContainerNodePool_version|TestAccContainerNodePool_regionalClusters|TestAccContainerNodePool_012_ConfigModeAttr|TestAccContainerNodePool_EmptyGuestAccelerator|TestAccContainerNodePool_ephemeralStorageConfig|TestAccDataprocWorkflowTemplate_basic|TestAccEventarcTrigger_BasicHandWritten|TestAccMonitoringMonitoredProject_BasicMonitoredProject|TestAccNetworkServicesEdgeCacheOrigin_networkServicesEdgeCacheOriginBasicExample|TestAccNetworkServicesEdgeCacheOrigin_networkServicesEdgeCacheOriginAdvancedExample|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceAdvancedExample|TestAccOrgPolicyPolicy_EnforcePolicy|TestAccOrgPolicyPolicy_FolderPolicy|TestAccOrgPolicyPolicy_ProjectPolicy|TestAccPrivatecaCaPool_privatecaCapoolAllFieldsExample|TestAccPrivatecaCaPool_privatecaCapoolUpdate|TestAccPrivatecaCaPool_privatecaCapoolEmptyBaseline|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthorityBasicExample|TestAccPrivatecaCertificate_privatecaCertificateConfigExample|TestAccPrivatecaCertificateAuthority_privatecaCertificateAuthoritySubordinateExample|TestAccPrivatecaCertificate_privatecaCertificateCsrExample|TestAccPrivatecaCertificate_privatecaCertificateNoAuthorityExample|TestAccPrivatecaCertificate_privatecaCertificateWithTemplateExample|TestAccPrivatecaCertificateTemplate_BasicCertificateTemplate|TestAccSqlDatabaseInstance_withPrivateNetwork|TestAccSqlUser_postgresIAM You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=218760 |
* Use url_param_only instead of virtual field * Rename new field includeMaxPathLength to includeMaxIssuerPathLength for local consistence
flattenPrivatecaCertificateConfigX509ConfigCaOptionsIsCa(val, d, config) | ||
// CertificateAuthority does not fields `include_is_ca`. | ||
// Expecting non-CertificateAuthority resource does not have field `certificate_authority_id` at top level. | ||
if exists && d.Get("certificate_authority_id") == nil { |
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.
@slevenick Do you have any suggestions for another way of checking if include_is_ca
is present in the schema before setting it?
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 would recommend splitting the flatten methods if the schemas no longer match.
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.
This has been solved by adding include_is_ca
to the other schema.
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 10 files changed, 337 insertions(+), 1179 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 10 files changed, 337 insertions(+), 1179 deletions(-)) |
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.
Functionality seems to work correctly. Just need to fix the descriptions on the fields that will appear in docs
mmv1/products/privateca/api.yaml
Outdated
name: 'nonCa' | ||
url_param_only: true | ||
description: | | ||
Must be set to true when is_ca is set to false. |
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.
These descriptions are incorrect now. It would be helpful to include information about why a user would want to use these fields, and what it actually does on the resource it creates.
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.
Hey @slevenick , descriptions are updated to improve the usability.
In order to handle the case of optional primitive fields, I've added virtual fields to allow the expander to distinguish between an unset primitive and a primitive set to the default value. Since some Private CA resources do not support updates, I also added support for setting ForceNew in the terraform config.
This prevents incompatiable configs that set the allow* booleans without setting basic constraints, and vice versa.
`include_max_issuer_path_path` to reflect its current functionality * Add field `include_is_ca` to CertificateAuthority to avoding checking the existence of this field in flattener. * Update examples with new fields like `include_is_ca`, `include_max_issuer_path_length`.
changes. * User `nonCa`, `zeroMaxIssuerPathLength` instead of `includeIsCa` `includeMaxIssuerPathLength` * Update test cases.
Run rebase to pull changes from upstream
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 10 files changed, 463 insertions(+), 1183 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccContainerClusterDatasource_zonal|TestAccContainerClusterDatasource_regional|TestAccApigeeEnvironmentIamBindingGenerated|TestAccApigeeEnvironmentIamMemberGenerated|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccContainerCluster_basic|TestAccContainerCluster_misc|TestAccContainerCluster_withAddons|TestAccContainerCluster_withILBSubsetting|TestAccContainerCluster_withConfidentialNodes|TestAccContainerCluster_withNotificationConfig|TestAccContainerCluster_withReleaseChannelEnabled|TestAccContainerCluster_withNetworkPolicyEnabled|TestAccContainerCluster_withTelemetryEnabled|TestAccContainerCluster_withMasterAuthorizedNetworksConfig|TestAccContainerCluster_withReleaseChannelEnabledDefaultVersion|TestAccContainerCluster_regionalWithNodePool|TestAccContainerCluster_withIntraNodeVisibility|TestAccContainerCluster_withVersion|TestAccContainerCluster_updateVersion|TestAccContainerCluster_network|TestAccContainerCluster_withNodePoolBasic|TestAccContainerCluster_backend|TestAccContainerCluster_withNodePoolResize|TestAccContainerCluster_withNodePoolUpdateVersion|TestAccContainerCluster_withNodePoolAutoscaling|TestAccContainerCluster_withNodePoolMultiple|TestAccContainerCluster_withNodePoolNodeConfig|TestAccContainerCluster_withRecurringMaintenanceWindow|TestAccContainerCluster_withMaintenanceWindow|TestAccContainerCluster_withMaintenanceExclusionWindow|TestAccContainerCluster_nodeAutoprovisioning|TestAccContainerCluster_withShieldedNodes|TestAccContainerCluster_errorAutopilotLocation|TestAccContainerCluster_nodeAutoprovisioningDefaults|TestAccContainerCluster_withWorkloadIdentityConfig|TestAccContainerCluster_withAutoscalingProfile|TestAccContainerCluster_nodeAutoprovisioningDefaultsMinCpuPlatform|TestAccContainerCluster_withBinaryAuthorization|TestAccContainerCluster_errorNoClusterCreated|TestAccContainerCluster_withDatabaseEncryption|TestAccContainerCluster_withResourceUsageExportConfig|TestAccContainerCluster_withEnableKubernetesAlpha|TestAccContainerNodePool_basicWithClusterId|TestAccContainerNodePool_basic|TestAccContainerNodePool_withNodeConfig|TestAccContainerNodePool_withWorkloadIdentityConfig|TestAccContainerNodePool_withSandboxConfig|TestAccContainerNodePool_withKubeletConfig|TestAccContainerNodePool_withLinuxNodeConfig|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccContainerNodePool_withManagement|TestAccContainerNodePool_withNodeConfigScopeAlias|TestAccContainerNodePool_withGPU|TestAccContainerNodePool_regionalAutoscaling|TestAccContainerNodePool_autoscaling|TestAccContainerNodePool_resize|TestAccContainerNodePool_regionalClusters|TestAccContainerNodePool_version|TestAccContainerNodePool_EmptyGuestAccelerator|TestAccContainerNodePool_shieldedInstanceConfig|TestAccContainerNodePool_012_ConfigModeAttr|TestAccContainerNodePool_ephemeralStorageConfig|TestAccGKEHubMembership_gkehubMembershipIssuerExample|TestAccGKEHubMembership_gkehubMembershipBasicExample|TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccPrivatecaCertificateTemplate_BasicCertificateTemplate|TestAccPrivatecaCertificate_privatecaCertificateNoAuthorityExample You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=228374 |
Tests failed during RECORDING mode: TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccContainerCluster_backend|TestAccContainerCluster_errorAutopilotLocation|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccPrivatecaCertificate_privatecaCertificateNoAuthorityExample Please fix these to complete your PR |
Update doc-string for fields in CaOptions
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 10 files changed, 506 insertions(+), 1220 deletions(-)) |
The failed test was passed when running locally. |
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
* Create custom expander for X509Config In order to handle the case of optional primitive fields, I've added virtual fields to allow the expander to distinguish between an unset primitive and a primitive set to the default value. Since some Private CA resources do not support updates, I also added support for setting ForceNew in the terraform config. * Add error handling for expansion This prevents incompatiable configs that set the allow* booleans without setting basic constraints, and vice versa. * Change virtual field names, and other feedback * Update examples with virtual fields * Use url_param_only instead of virtual field. * Handle CertificateAuthority resource which does not have field include_is_ca * Fix format issue * * Update description for fields `include_is_ca` `include_max_issuer_path_path` to reflect its current functionality * Add field `include_is_ca` to CertificateAuthority to avoding checking the existence of this field in flattener. * Update examples with new fields like `include_is_ca`, `include_max_issuer_path_length`. * Update description; Add test cases for CaOption * fix a typo * remove include_x from template resource * Update semantic meaning for newly added fields to avoid breaking changes. * User `nonCa`, `zeroMaxIssuerPathLength` instead of `includeIsCa` `includeMaxIssuerPathLength` * Update test cases. * Create custom expander for X509Config In order to handle the case of optional primitive fields, I've added virtual fields to allow the expander to distinguish between an unset primitive and a primitive set to the default value. Since some Private CA resources do not support updates, I also added support for setting ForceNew in the terraform config. * Add error handling for expansion This prevents incompatiable configs that set the allow* booleans without setting basic constraints, and vice versa. * Change virtual field names, and other feedback * Update examples with virtual fields * Use url_param_only instead of virtual field. * Handle CertificateAuthority resource which does not have field include_is_ca * Fix format issue * * Update description for fields `include_is_ca` `include_max_issuer_path_path` to reflect its current functionality * Add field `include_is_ca` to CertificateAuthority to avoding checking the existence of this field in flattener. * Update examples with new fields like `include_is_ca`, `include_max_issuer_path_length`. * Update description; Add test cases for CaOption * fix a typo * remove include_x from template resource * Update semantic meaning for newly added fields to avoid breaking changes. * User `nonCa`, `zeroMaxIssuerPathLength` instead of `includeIsCa` `includeMaxIssuerPathLength` * Update test cases. * Update doc-string for fields in CaOptions Co-authored-by: Yong Cao <[email protected]>
In order to handle the case of optional primitive fields, I've added virtual fields to allow the expander to distinguish between an unset primitive and a primitive set to the default value.
Fixes hashicorp/terraform-provider-google#10108
Fixes hashicorp/terraform-provider-google#10239
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)