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 <[email protected]>

* Added test for invalid value for retention bytes

* Minor code update

Co-authored-by: Matt Hughes <[email protected]>

* Minor code update

---------

Co-authored-by: Matt Hughes <[email protected]>

Author: sbuliarca

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,16 +138,22 @@ func (r *MSKTopicConfigCommentsRule) validateConfigValuesInComments(
 			return err
 		}
 	}
+	for _, configValueInfo := range configByteValueCommentInfos {
+		if err := r.validateByteConfigValue(runner, configKeyToPairMap, configValueInfo); err != nil {
+			return err
+		}
+	}
 
 	return nil
 }
 
 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,39 +166,70 @@ 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
 	}
 
 	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 {

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"
   }
 }