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

Add support for AWS::IoT::SecurityProfile #19

Merged
merged 8 commits into from
Dec 16, 2020

Conversation

anton-aws
Copy link
Contributor

Notes

Security Profiles are used to configure Device Defender Detect. API documentation can be found at https://docs.aws.amazon.com/iot/latest/apireference/API_CreateSecurityProfile.html. Main highlights for the resource from CloudFormation perspective:

  1. We're supporting target attachments with the "TargetArns" field. Attachments are managed with Attach/DetachSecurity Profile APIs.
  2. Behavior::Criteria::MetricValue is of type Long in the API, but CFN's json-schema allows only integer&double, neither of which provide enough space. This is a known issue and some existing services are already using Strings as the workaround. For example, AWS::ElasticLoadBalancing::LoadBalancer HealthCheck https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-elb-health-check.html. We're doing the same.
  3. SecurityProfile has several optional List/Map fields, and we needed to handle them carefully to differentiate between null & empty. Refer to the code & comments in the Translator class.

Testing

  • Unit tests - 80%+ coverage.
  • Contract tests passed - except for the ones that failed due to known issues in the contract tests library.
  • Ran pre-commit run --all-files, mvn verify successfully.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

"permissions": [
"iot:CreateSecurityProfile",
"iot:AttachSecurityProfile",
"iam:PassRole"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the permission for iam:PassRole for create and update calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed if the customer specifies an AlertTarget. Device Defender (as all other AWS services) requires customers to have a permission for passing the role.

// SDK converts nulls from Describe API to empty DefaultSdkAutoConstructLists,
// so if we simply translate without the .has* check, nulls will turn into empty lists.
if (iotMetricValue.hasCidrs()) {
metricValueBuilder.cidrs(new HashSet<>(iotMetricValue.cidrs()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be Arrays, since we defining the type as Arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Discussed offline) We've modeled Cidrs as well as other lists like Behaviors to have "insertionOrder=false", because the order of the elements doesn't matter to the service. That's why the generated Java objects have Sets not Lists.

Comment on lines 260 to 271
"AdditionalMetricsToRetain": {
"description": "This parameter has been deprecated, please use AdditionalMetricsToRetainV2.",
"type": "array",
"uniqueItems": true,
"insertionOrder": false,
"items": {
"type": "string",
"pattern": "[a-zA-Z0-9:_-]+",
"minLength": 1,
"maxLength": 128
}
},

Choose a reason for hiding this comment

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

Do we need to support this in Cloudformation? All resources created by CFN can be on-boarded to V2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, all the security profiles being added to CFN templates would be newly created, and the field has been deprecated for a while. Removing it from the CFN model.


ResourceModel model = request.getDesiredResourceState();
if (!StringUtils.isEmpty(model.getSecurityProfileArn())) {
logger.log(String.format("Arn is read-only, but the caller passed %s.", model.getSecurityProfileArn()));

Choose a reason for hiding this comment

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

Does CFN logger not support error/info log levels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't, there's an open issue for it aws-cloudformation/cloudformation-cli-java-plugin#225

@anton-aws anton-aws merged commit 735eac8 into aws-cloudformation:master Dec 16, 2020
Comment on lines +240 to +243
"Behaviors": {
"description": "Specifies the behaviors that, when violated by a device (thing), cause an alert.",
"type": "array",
"maxLength": 100,
Copy link

@PatMyron PatMyron Mar 12, 2021

Choose a reason for hiding this comment

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

don't think minLength/maxLength can be applied to array types:
aws-cloudformation/cloudformation-cli#414
https://json-schema.org/understanding-json-schema/reference/array.html#length
(caught by running cfn validate)

Comment on lines +108 to +111
"Count": {
"description": "If the ComparisonOperator calls for a numeric value, use this to specify that (integer) numeric value to be compared with the metric.",
"type": "string",
"minimum": 0

Choose a reason for hiding this comment

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

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.

5 participants