Skip to content

Conversation

gpontejos
Copy link
Contributor

No description provided.

@gpontejos gpontejos force-pushed the custom-policy-resource branch from 22e6889 to ab438d4 Compare September 12, 2025 16:03
@gpontejos gpontejos force-pushed the custom-policy-resource branch from ab438d4 to 6d6a396 Compare September 23, 2025 20:25

"github.com/crowdstrike/gofalcon/falcon"
cloudcompliance "github.com/crowdstrike/terraform-provider-crowdstrike/internal/cloud_compliance"
"github.com/crowdstrike/terraform-provider-crowdstrike/internal/cloud_posture"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"github.com/crowdstrike/terraform-provider-crowdstrike/internal/cloud_posture"
cloudposture "github.com/crowdstrike/terraform-provider-crowdstrike/internal/cloud_posture"

contentupdatepolicy.NewDefaultContentUpdatePolicyResource,
contentupdatepolicy.NewContentUpdatePolicyPrecedenceResource,
sensorvisibilityexclusion.NewSensorVisibilityExclusionResource,
cloud_posture.NewCloudPostureCustomRuleResource,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cloud_posture.NewCloudPostureCustomRuleResource,
cloudposture.NewCloudPostureCustomRuleResource,

sensorupdatepolicy.NewSensorUpdateBuildsDataSource,
fcs.NewCloudAwsAccountsDataSource,
contentupdatepolicy.NewContentCategoryVersionsDataSource,
cloud_posture.NewCloudPostureRulesDataSource,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cloud_posture.NewCloudPostureRulesDataSource,
cloudposture.NewCloudPostureRulesDataSource,

}

type cloudComplianceFrameworkControlDataSourceModel struct {
Controls []cloudComplianceFrameworkControlModel `tfsdk:"controls"`
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use terraform types for our model struct. Almost every bug I have had to fix have been due to me using go types in my model. A recent example: #136

Suggested change
Controls []cloudComplianceFrameworkControlModel `tfsdk:"controls"`
Controls []types.Set `tfsdk:"controls"`

You can use Schedule in sensor update policy as reference.

data.Controls = types.SetNull(types.ObjectType{AttrTypes: cloudComplianceFrameworkControlModel{}.AttributeTypes()})

Basically I add a AttributeTypes method to help reduce the boilerplate that is required to convert from go to tf types.


# retrieve all controls under a named benchmark
data "crowdstrike_cloud_compliance_framework_controls" "all" {
cloud_provider = "AWS"
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look like a valid option.

Comment on lines +326 to +329
diags.AddError(
"Failed to create rule.",
fmt.Sprintf("Failed to create rule: %s", err),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

this error message needs to be updated

Comment on lines +344 to +350
if len(queryPayload.Resources) == 0 {
diags.AddWarning(
"No Rules Found",
"The query returned no rules. Please check your filter criteria.",
)
return nil, diags
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this warning.

Comment on lines +361 to +364
diags.AddError(
"Failed to get rule",
fmt.Sprintf("Failed to get rule: %s", err),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
diags.AddError(
"Failed to get rule",
fmt.Sprintf("Failed to get rule: %s", err),
)
diags.AddError(
"Failed to get rules",
fmt.Sprintf("Failed to get rules: %s", err),
)

if err = falcon.AssertNoError(getRulesPayload.Errors); err != nil {
diags.AddError(
"Failed to get rules",
fmt.Sprintf("Failed to get rule: %s", err.Error()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fmt.Sprintf("Failed to get rule: %s", err.Error()),
fmt.Sprintf("Failed to get rules: %s", err.Error()),

Comment on lines +414 to +420
rule.AttackTypes = types.SetValueMust(types.StringType, []attr.Value{})
for _, attackType := range resource.AttackTypes {
rule.AttackTypes, diags = types.SetValue(types.StringType, append(rule.AttackTypes.Elements(), types.StringValue(attackType)))
if diags.HasError() {
return nil, diags
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you should be able to do this instead

Suggested change
rule.AttackTypes = types.SetValueMust(types.StringType, []attr.Value{})
for _, attackType := range resource.AttackTypes {
rule.AttackTypes, diags = types.SetValue(types.StringType, append(rule.AttackTypes.Elements(), types.StringValue(attackType)))
if diags.HasError() {
return nil, diags
}
}
attackTypeSet, diags := types.SetValueFrom(ctx, types.StringType, resource.AttackTypes)
if diags.HasError() {
return nil, diags
}

Copy link
Contributor

@ffalor ffalor left a comment

Choose a reason for hiding this comment

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

I've added annotations and code change suggestions

Copy link
Contributor

@ffalor ffalor left a comment

Choose a reason for hiding this comment

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

We need to add pagination support also for these get requests. I implement an example here

func (r *contentUpdatePolicyPrecedenceResource) getContentUpdatePoliciesByPrecedence(

There are a few pagination methods our API's use so implementation varies.

Comment on lines +176 to +181
if config.Benchmark.IsNull() && config.FQL.IsNull() {
resp.Diagnostics.AddError(
"Invalid Configuration",
"At least one of 'benchmark' or 'fql' must be defined",
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible for this endpoint to return everything? if it is then instead of requiring fql or benchmark, we should allow nothing to be provided, and just verify they aren't providing both are the same time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants