From 64e26f83cd0162e432e0ec34718b4b374cc1e390 Mon Sep 17 00:00:00 2001 From: sbuliarca <8848332+sbuliarca@users.noreply.github.com> Date: Wed, 6 Nov 2024 08:28:14 +0200 Subject: [PATCH] DENA-671: comments validation for properties with bytes (#32) * Extracted method for reporting the human-readable comment * Implemented handling comments for byte values * Added more tests * Added handling of retention.bytes * Updated docs * Use binary prefixes * Update rules/msk_topic_config_comments.md Co-authored-by: Matt Hughes <128392218+matthewhughes-uw@users.noreply.github.com> * Added test for invalid value for retention bytes * Minor code update Co-authored-by: Matt Hughes <128392218+matthewhughes-uw@users.noreply.github.com> * Minor code update --------- Co-authored-by: Matt Hughes <128392218+matthewhughes-uw@users.noreply.github.com> --- rules/msk_topic_config_comments.go | 149 +++++++++++++++++++--- rules/msk_topic_config_comments.md | 9 +- rules/msk_topic_config_comments_test.go | 162 +++++++++++++++++++++++- 3 files changed, 298 insertions(+), 22 deletions(-) diff --git a/rules/msk_topic_config_comments.go b/rules/msk_topic_config_comments.go index 89d7ebb..1d78f14 100644 --- a/rules/msk_topic_config_comments.go +++ b/rules/msk_topic_config_comments.go @@ -86,14 +86,14 @@ func (r *MSKTopicConfigCommentsRule) validateTopicConfigComments(runner tflint.R return nil } -type configTimeValueCommentInfo struct { +type configValueCommentInfo struct { key string infiniteValue string baseComment string issueWhenInvalid bool } -var configTimeValueCommentInfos = []configTimeValueCommentInfo{ +var configTimeValueCommentInfos = []configValueCommentInfo{ { key: retentionTimeAttr, infiniteValue: "-1", @@ -114,6 +114,21 @@ var configTimeValueCommentInfos = []configTimeValueCommentInfo{ }, } +var configByteValueCommentInfos = []configValueCommentInfo{ + { + key: "max.message.bytes", + infiniteValue: "", + baseComment: "allow for a batch of records maximum", + issueWhenInvalid: true, + }, + { + key: "retention.bytes", + infiniteValue: "-1", + baseComment: "keep on each partition", + issueWhenInvalid: true, + }, +} + func (r *MSKTopicConfigCommentsRule) validateConfigValuesInComments( runner tflint.Runner, configKeyToPairMap map[string]hcl.KeyValuePair, @@ -123,6 +138,11 @@ func (r *MSKTopicConfigCommentsRule) validateConfigValuesInComments( return err } } + for _, configValueInfo := range configByteValueCommentInfos { + if err := r.validateByteConfigValue(runner, configKeyToPairMap, configValueInfo); err != nil { + return err + } + } return nil } @@ -130,9 +150,10 @@ func (r *MSKTopicConfigCommentsRule) validateConfigValuesInComments( func (r *MSKTopicConfigCommentsRule) validateTimeConfigValue( runner tflint.Runner, configKeyToPairMap map[string]hcl.KeyValuePair, - configValueInfo configTimeValueCommentInfo, + configValueInfo configValueCommentInfo, ) error { - timePair, hasConfig := configKeyToPairMap[configValueInfo.key] + key := configValueInfo.key + timePair, hasConfig := configKeyToPairMap[key] if !hasConfig { return nil } @@ -145,7 +166,38 @@ func (r *MSKTopicConfigCommentsRule) validateTimeConfigValue( return nil } - comment, err := r.getExistingComment(runner, timePair) + return r.reportHumanReadableComment(runner, timePair, key, msg) +} + +func (r *MSKTopicConfigCommentsRule) validateByteConfigValue( + runner tflint.Runner, + configKeyToPairMap map[string]hcl.KeyValuePair, + configValueInfo configValueCommentInfo, +) error { + key := configValueInfo.key + dataPair, hasConfig := configKeyToPairMap[key] + if !hasConfig { + return nil + } + + msg, err := r.buildDataSizeComment(runner, dataPair, configValueInfo) + if err != nil { + return err + } + if msg == "" { + return nil + } + + return r.reportHumanReadableComment(runner, dataPair, key, msg) +} + +func (r *MSKTopicConfigCommentsRule) reportHumanReadableComment( + runner tflint.Runner, + keyValuePair hcl.KeyValuePair, + key string, + commentMsg string, +) error { + comment, err := r.getExistingComment(runner, keyValuePair) if err != nil { return err } @@ -153,31 +205,31 @@ func (r *MSKTopicConfigCommentsRule) validateTimeConfigValue( if comment == nil { err := runner.EmitIssueWithFix( r, - fmt.Sprintf("%s must have a comment with the human readable value: adding it ...", configValueInfo.key), - timePair.Key.Range(), + fmt.Sprintf("%s must have a comment with the human readable value: adding it ...", key), + keyValuePair.Key.Range(), func(f tflint.Fixer) error { - return f.InsertTextAfter(timePair.Value.Range(), msg) + return f.InsertTextAfter(keyValuePair.Value.Range(), commentMsg) }, ) if err != nil { - return fmt.Errorf("emitting issue: no comment for time value: %w", err) + return fmt.Errorf("emitting issue: no comment for human readable value: %w", err) } return nil } commentTxt := strings.TrimSpace(string(comment.Bytes)) - if commentTxt != msg { + if commentTxt != commentMsg { issueMsg := fmt.Sprintf( "%s value doesn't correspond to the human readable value in the comment: fixing it ...", - configValueInfo.key, + key, ) err := runner.EmitIssueWithFix(r, issueMsg, comment.Range, func(f tflint.Fixer) error { - return f.ReplaceText(comment.Range, msg+"\n") + return f.ReplaceText(comment.Range, commentMsg+"\n") }, ) if err != nil { - return fmt.Errorf("emitting issue: wrong comment for time value: %w", err) + return fmt.Errorf("emitting issue: wrong comment for human readable value: %w", err) } } return nil @@ -243,7 +295,7 @@ func isNotComment(token hclsyntax.Token) bool { func (r *MSKTopicConfigCommentsRule) buildDurationComment( runner tflint.Runner, timePair hcl.KeyValuePair, - configValueInfo configTimeValueCommentInfo, + configValueInfo configValueCommentInfo, ) (string, error) { var timeVal string diags := gohcl.DecodeExpression(timePair.Value, nil, &timeVal) @@ -271,10 +323,73 @@ func (r *MSKTopicConfigCommentsRule) buildDurationComment( return "", nil } - baseComment := configValueInfo.baseComment + return buildCommentForMillis(timeMillis, configValueInfo.baseComment), nil +} + +func (r *MSKTopicConfigCommentsRule) buildDataSizeComment( + runner tflint.Runner, + dataPair hcl.KeyValuePair, + configValueInfo configValueCommentInfo, +) (string, error) { + var dataVal string + diags := gohcl.DecodeExpression(dataPair.Value, nil, &dataVal) + if diags.HasErrors() { + return "", diags + } + + if dataVal == configValueInfo.infiniteValue { + return fmt.Sprintf("# %s unlimited data", configValueInfo.baseComment), nil + } - msg := buildCommentForMillis(timeMillis, baseComment) - return msg, nil + byteVal, err := strconv.Atoi(dataVal) + if err != nil { + if configValueInfo.issueWhenInvalid { + issueMsg := fmt.Sprintf( + "%s must have a valid integer value expressed in bytes", + configValueInfo.key, + ) + err := runner.EmitIssue(r, issueMsg, dataPair.Value.Range()) + if err != nil { + return "", fmt.Errorf("emitting issue: invalid data value: %w", err) + } + } + + return "", nil + } + + return buildCommentForBytes(byteVal, configValueInfo.baseComment), nil +} + +func buildCommentForBytes(bytes int, baseComment string) string { + byteUnits, unit := determineByteUnits(bytes) + + byteUnitsStr := strconv.FormatFloat(byteUnits, 'f', -1, 64) + return fmt.Sprintf("# %s %s%s", baseComment, byteUnitsStr, unit) +} + +const ( + bytesInOneKiB = 1024 + bytesInOneMiB = 1024 * bytesInOneKiB + bytesInOneGiB = 1024 * bytesInOneMiB +) + +func determineByteUnits(bytes int) (float64, string) { + floatBytes := float64(bytes) + gbs := round(floatBytes / bytesInOneGiB) + if gbs >= 1 { + return gbs, "GiB" + } + + mbs := round(floatBytes / bytesInOneMiB) + if mbs >= 1 { + return mbs, "MiB" + } + + kbs := round(floatBytes / bytesInOneKiB) + if kbs >= 1 { + return kbs, "KiB" + } + return floatBytes, "B" } func buildCommentForMillis(timeMillis int, baseComment string) string { diff --git a/rules/msk_topic_config_comments.md b/rules/msk_topic_config_comments.md index 0e47f24..e713f60 100644 --- a/rules/msk_topic_config_comments.md +++ b/rules/msk_topic_config_comments.md @@ -2,7 +2,7 @@ ## Requirements -Topic configurations expressed in milliseconds must have comments explaining the property and including the human-readable value. +Topic configurations expressed in milliseconds and bytes must have comments explaining the property and including the human-readable value. The comments can be placed after the property definition on the same line or on the line before the definition. For computing the human-readable values it considers the following: @@ -13,7 +13,8 @@ It currently checks the properties: - retention.ms: explanation must start with `keep data` - local.retention.ms: explanation must start with `keep data in primary storage` - max.compaction.lag.ms: explanation must start with `allow not compacted keys maximum` - +- retention.bytes: explanation must start with `keep on each partition` +- max.message.bytes: explanation must start with `allow for a batch of records maximum` ## Example ### Good example @@ -28,7 +29,9 @@ resource "kafka_topic" "good_topic" { "cleanup.policy" = "delete" # keep data in primary storage for 1 day "local.retention.ms" = "86400000" - "retention.ms" = "2592000000" # keep data for 1 month + "retention.ms" = "2592000000" # keep data for 1 month + "max.message.bytes" = "3145728" # allow for a batch of records maximum 3MiB + "retention.bytes" = "1610612736" # keep on each partition 1.5GiB "compression.type" = "zstd" } } diff --git a/rules/msk_topic_config_comments_test.go b/rules/msk_topic_config_comments_test.go index 94a86b1..dfa4a92 100644 --- a/rules/msk_topic_config_comments_test.go +++ b/rules/msk_topic_config_comments_test.go @@ -9,7 +9,7 @@ import ( "github.com/terraform-linters/tflint-plugin-sdk/helper" ) -var configValueCommentsTests = []topicConfigTestCase{ +var configTimeCommentsTests = []topicConfigTestCase{ { name: "retention time without comment", input: ` @@ -313,9 +313,167 @@ resource "kafka_topic" "topic_def" { }, } +var configByteCommentsTests = []topicConfigTestCase{ + { + name: "max message bytes without a comment", + input: ` +resource "kafka_topic" "topic_def" { + name = "topic-def" + config = { + "max.message.bytes" = "3145728" + } +}`, fixed: ` +resource "kafka_topic" "topic_def" { + name = "topic-def" + config = { + "max.message.bytes" = "3145728" # allow for a batch of records maximum 3MiB + } +}`, + expected: []*helper.Issue{ + { + Message: "max.message.bytes must have a comment with the human readable value: adding it ...", + Range: hcl.Range{ + Filename: fileName, + Start: hcl.Pos{Line: 5, Column: 5}, + End: hcl.Pos{Line: 5, Column: 24}, + }, + }, + }, + }, + { + name: "max message bytes with value in gigabytes", + input: ` +resource "kafka_topic" "topic_def" { + name = "topic-def" + config = { + "max.message.bytes" = "4509715661" # allow for a batch of records maximum 4.2GiB + } +}`, + expected: []*helper.Issue{}, + }, + { + name: "max message bytes with value in kilos", + input: ` +resource "kafka_topic" "topic_def" { + name = "topic-def" + config = { + "max.message.bytes" = "204800" # allow for a batch of records maximum 200KiB + } +}`, + expected: []*helper.Issue{}, + }, + { + name: "max message bytes with value in bytes", + input: ` +resource "kafka_topic" "topic_def" { + name = "topic-def" + config = { + "max.message.bytes" = "100" # allow for a batch of records maximum 100B + } +}`, + expected: []*helper.Issue{}, + }, + { + name: "max message bytes invalid", + input: ` +resource "kafka_topic" "topic_def" { + name = "topic-def" + config = { + "max.message.bytes" = "invalid-val" + } +}`, + expected: []*helper.Issue{ + { + Message: "max.message.bytes must have a valid integer value expressed in bytes", + Range: hcl.Range{ + Filename: fileName, + Start: hcl.Pos{Line: 5, Column: 27}, + End: hcl.Pos{Line: 5, Column: 40}, + }, + }, + }, + }, + { + name: "retention bytes without a comment", + input: ` +resource "kafka_topic" "topic_def" { + name = "topic-def" + config = { + "retention.bytes" = "1610612736" + } +}`, fixed: ` +resource "kafka_topic" "topic_def" { + name = "topic-def" + config = { + "retention.bytes" = "1610612736" # keep on each partition 1.5GiB + } +}`, + expected: []*helper.Issue{ + { + Message: "retention.bytes must have a comment with the human readable value: adding it ...", + Range: hcl.Range{ + Filename: fileName, + Start: hcl.Pos{Line: 5, Column: 5}, + End: hcl.Pos{Line: 5, Column: 22}, + }, + }, + }, + }, + { + name: "retention bytes with outdated value", + input: ` +resource "kafka_topic" "topic_def" { + name = "topic-def" + config = { + "retention.bytes" = "-1" # keep on each partition 3MiB + } +}`, fixed: ` +resource "kafka_topic" "topic_def" { + name = "topic-def" + config = { + "retention.bytes" = "-1" # keep on each partition unlimited data + } +}`, + expected: []*helper.Issue{ + { + Message: "retention.bytes value doesn't correspond to the human readable value in the comment: fixing it ...", + Range: hcl.Range{ + Filename: fileName, + Start: hcl.Pos{Line: 5, Column: 30}, + End: hcl.Pos{Line: 6, Column: 1}, + }, + }, + }, + }, + { + name: "retention bytes invalid", + input: ` +resource "kafka_topic" "topic_def" { + name = "topic-def" + config = { + "retention.bytes" = "invalid-val" + } +}`, + expected: []*helper.Issue{ + { + Message: "retention.bytes must have a valid integer value expressed in bytes", + Range: hcl.Range{ + Filename: fileName, + Start: hcl.Pos{Line: 5, Column: 25}, + End: hcl.Pos{Line: 5, Column: 38}, + }, + }, + }, + }, +} + func Test_MSKTopicConfigCommentsRule(t *testing.T) { rule := &MSKTopicConfigCommentsRule{} - for _, tc := range configValueCommentsTests { + var allTests []topicConfigTestCase + allTests = append(allTests, configTimeCommentsTests...) + allTests = append(allTests, configByteCommentsTests...) + + for _, tc := range allTests { t.Run(tc.name, func(t *testing.T) { runner := helper.TestRunner(t, map[string]string{fileName: tc.input}) require.NoError(t, rule.Check(runner))