Skip to content

Commit

Permalink
DENA-671: comments validation for properties with millis (#29)
Browse files Browse the repository at this point in the history
topic comments validation for properties with millis
  • Loading branch information
sbuliarca authored Nov 1, 2024
1 parent 0ef48eb commit 4cb119f
Show file tree
Hide file tree
Showing 7 changed files with 728 additions and 33 deletions.
15 changes: 8 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,14 @@ plugin "uw-kafka-config" {

## Rules

| Name | Description |
|-----------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------|
| [`msk_module_backend`](rules/msk_module_backend.md) | Requires an S3 backend to be defined, with a key that has as suffix the name of the team (taken from the current directory name) |
| [`msk_app_topics`](rules/msk_app_topics.md) | Requires apps consume from and produce to only topics define in their module. |
| [`msk_topic_name`](rules/msk_topic_name.md) | Requires defined topics in a module to belong to that team. |
| [`msk_topic_config`](rules/msk_topic_config.md) | Checks the configuration for MSK topics |
| [`msk_unique_app_names`](rules/msk_unique_app_names.md) | Checks that TLS app names are unique |
| Name | Description |
|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------|
| [`msk_module_backend`](rules/msk_module_backend.md) | Requires an S3 backend to be defined, with a key that has as suffix the name of the team (taken from the current directory name) |
| [`msk_app_topics`](rules/msk_app_topics.md) | Requires apps consume from and produce to only topics define in their module. |
| [`msk_topic_name`](rules/msk_topic_name.md) | Requires defined topics in a module to belong to that team. |
| [`msk_topic_config`](rules/msk_topic_config.md) | Checks the configuration for MSK topics |
| [`msk_topic_config_comments`](rules/msk_topic_config_comments.md) | Checks the comments for topic configurations expressed in millis |
| [`msk_unique_app_names`](rules/msk_unique_app_names.md) | Checks that TLS app names are unique |


## Building the plugin
Expand Down
2 changes: 2 additions & 0 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ func main() {
&rules.MSKAppTopicsRule{},
&rules.MSKTopicNameRule{},
&rules.MSKTopicConfigRule{},
// keep the comments rule after the config one, as the config one might remove some properties checked by the comments one
&rules.MSKTopicConfigCommentsRule{},
&rules.MSKUniqueAppNamesRule{},
},
},
Expand Down
21 changes: 13 additions & 8 deletions rules/msk_topic_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -341,25 +341,30 @@ func (r *MSKTopicConfigRule) getAndValidateCleanupPolicyValue(

const (
retentionTimeAttr = "retention.ms"
millisInOneDay = 1 * 24 * 60 * 60 * 1000
millisInOneHour = 60 * 60 * 1000
millisInOneDay = 24 * millisInOneHour
millisInOneMonth = 30 * millisInOneDay
millisInOneYear = 365 * millisInOneDay
// The threshold on retention time when remote storage is supported.
tieredStorageThresholdInDays = 3
tieredStorageEnableAttr = "remote.storage.enable"
tieredStorageEnabledValue = "true"
localRetentionTimeAttr = "local.retention.ms"
localRetentionTimeInDaysDefault = 1
localRetentionTimeMillisDefault = 1 * millisInOneDay
localRetentionTimeCommentBase = "keep data in primary storage"
)

/* Putting an invalid value by default to force users to put a valid value */
var (
retentionTimeDefTemplate = fmt.Sprintf(`"%s" = "???"`, retentionTimeAttr)
enableTieredStorage = fmt.Sprintf(`"%s" = "%s"`, tieredStorageEnableAttr, tieredStorageEnabledValue)
localRetentionTimeFix = fmt.Sprintf(
`# keep data in hot storage for %d day
"%s" = "%d"`,
localRetentionTimeInDaysDefault,
/* putting the comment after the property definition. */
localRetentionTimeFix = fmt.Sprintf(
`"%s" = "%d" %s`,
localRetentionTimeAttr,
localRetentionTimeInDaysDefault*millisInOneDay)
localRetentionTimeMillisDefault,
buildCommentForMillis(localRetentionTimeMillisDefault, localRetentionTimeCommentBase),
)
)

func (r *MSKTopicConfigRule) validateRetentionForDeletePolicy(
Expand Down Expand Up @@ -412,7 +417,7 @@ func (r *MSKTopicConfigRule) validateLocalRetentionDefined(
msg := fmt.Sprintf(
"missing %s when tiered storage is enabled: using default '%d'",
localRetentionTimeAttr,
localRetentionTimeInDaysDefault*millisInOneDay,
localRetentionTimeMillisDefault,
)
err := runner.EmitIssueWithFix(r, msg, config.Range,
func(f tflint.Fixer) error {
Expand Down
317 changes: 317 additions & 0 deletions rules/msk_topic_config_comments.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,317 @@
package rules

import (
"fmt"
"slices"
"strconv"
"strings"

"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/hcl/v2/gohcl"
"github.com/hashicorp/hcl/v2/hclsyntax"
"github.com/terraform-linters/tflint-plugin-sdk/hclext"
"github.com/terraform-linters/tflint-plugin-sdk/logger"
"github.com/terraform-linters/tflint-plugin-sdk/tflint"
)

// MSKTopicConfigCommentsRule checks comments on time and bytes values.
type MSKTopicConfigCommentsRule struct {
tflint.DefaultRule
}

func (r *MSKTopicConfigCommentsRule) Name() string {
return "msk_topic_config_comments"
}

func (r *MSKTopicConfigCommentsRule) Enabled() bool {
return true
}

func (r *MSKTopicConfigCommentsRule) Link() string {
return ReferenceLink(r.Name())
}

func (r *MSKTopicConfigCommentsRule) Severity() tflint.Severity {
return tflint.ERROR
}

func (r *MSKTopicConfigCommentsRule) Check(runner tflint.Runner) error {
isRoot, err := isRootModule(runner)
if err != nil {
return err
}
if !isRoot {
logger.Debug("skipping child module")
return nil
}

resourceContents, err := runner.GetResourceContent(
"kafka_topic",
&hclext.BodySchema{
Attributes: []hclext.AttributeSchema{
{Name: "config"},
},
},
nil,
)
if err != nil {
return fmt.Errorf("getting kafka_topic contents: %w", err)
}

for _, topicResource := range resourceContents.Blocks {
if err := r.validateTopicConfigComments(runner, topicResource); err != nil {
return err
}
}

return nil
}

func (r *MSKTopicConfigCommentsRule) validateTopicConfigComments(runner tflint.Runner, topic *hclext.Block) error {
configAttr, hasConfig := topic.Body.Attributes["config"]
if !hasConfig {
return nil
}

/* construct a mapping between the config key and the config KeyPair. This helps in both checking if a key is defined and to propose fixes to the values*/
configKeyToPairMap, err := constructConfigKeyToPairMap(configAttr)
if err != nil {
return err
}

if err = r.validateConfigValuesInComments(runner, configKeyToPairMap); err != nil {
return err
}
return nil
}

type configTimeValueCommentInfo struct {
key string
infiniteValue string
baseComment string
issueWhenInvalid bool
}

var configTimeValueCommentInfos = []configTimeValueCommentInfo{
{
key: retentionTimeAttr,
infiniteValue: "-1",
baseComment: "keep data",
issueWhenInvalid: false,
},
{
key: localRetentionTimeAttr,
infiniteValue: "-2",
baseComment: localRetentionTimeCommentBase,
issueWhenInvalid: false,
},
{
key: "max.compaction.lag.ms",
infiniteValue: "",
baseComment: "allow not compacted keys maximum",
issueWhenInvalid: true,
},
}

func (r *MSKTopicConfigCommentsRule) validateConfigValuesInComments(
runner tflint.Runner,
configKeyToPairMap map[string]hcl.KeyValuePair,
) error {
for _, configValueInfo := range configTimeValueCommentInfos {
if err := r.validateTimeConfigValue(runner, configKeyToPairMap, configValueInfo); err != nil {
return err
}
}

return nil
}

func (r *MSKTopicConfigCommentsRule) validateTimeConfigValue(
runner tflint.Runner,
configKeyToPairMap map[string]hcl.KeyValuePair,
configValueInfo configTimeValueCommentInfo,
) error {
timePair, hasConfig := configKeyToPairMap[configValueInfo.key]
if !hasConfig {
return nil
}

msg, err := r.buildDurationComment(runner, timePair, configValueInfo)
if err != nil {
return err
}
if msg == "" {
return nil
}

comment, err := r.getExistingComment(runner, timePair)
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(),
func(f tflint.Fixer) error {
return f.InsertTextAfter(timePair.Value.Range(), msg)
},
)
if err != nil {
return fmt.Errorf("emitting issue: no comment for time value: %w", err)
}
return nil
}

commentTxt := strings.TrimSpace(string(comment.Bytes))
if commentTxt != msg {
issueMsg := fmt.Sprintf(
"%s value doesn't correspond to the human readable value in the comment: fixing it ...",
configValueInfo.key,
)
err := runner.EmitIssueWithFix(r, issueMsg, comment.Range,
func(f tflint.Fixer) error {
return f.ReplaceText(comment.Range, msg+"\n")
},
)
if err != nil {
return fmt.Errorf("emitting issue: wrong comment for time value: %w", err)
}
}
return nil
}

func (r *MSKTopicConfigCommentsRule) getExistingComment(
runner tflint.Runner,
pair hcl.KeyValuePair,
) (*hclsyntax.Token, error) {
comments, err := r.getCommentsForFile(runner, pair.Key.Range().Filename)
if err != nil {
return nil, err
}

// first look for the comment on the same line, after the property definition.
// Example: "retention.ms" = "2629800000" # keep data for 30 days
afterPropertyIdx := slices.IndexFunc(comments, func(comment hclsyntax.Token) bool {
return comment.Range.Start.Line == pair.Key.Range().Start.Line &&
comment.Range.Start.Column > pair.Value.Range().End.Column
})

if afterPropertyIdx >= 0 {
return &comments[afterPropertyIdx], nil
}

/* second, look for the comment on the previous line, before the property definition. Example:
# keep data for 30 days
"retention.ms" = "2629800000"
*/
beforePropertyIdx := slices.IndexFunc(comments, func(comment hclsyntax.Token) bool {
return comment.Range.Start.Line == pair.Key.Range().Start.Line-1 &&
comment.Range.End.Line == pair.Key.Range().Start.Line
})
if beforePropertyIdx >= 0 {
return &comments[beforePropertyIdx], nil
}

return nil, nil
}

func (r *MSKTopicConfigCommentsRule) getCommentsForFile(
runner tflint.Runner,
filename string,
) (hclsyntax.Tokens, error) {
// we need to parse the file every time, otherwise keeping a cache per file doesn't work
file, err := runner.GetFile(filename)
if err != nil {
return nil, fmt.Errorf("getting hcl file %s for reading comments: %w", filename, err)
}

tokens, diags := hclsyntax.LexConfig(file.Bytes, filename, hcl.InitialPos)
if diags != nil {
return nil, diags
}

return slices.DeleteFunc(tokens, isNotComment), nil
}

func isNotComment(token hclsyntax.Token) bool {
return token.Type != hclsyntax.TokenComment
}

func (r *MSKTopicConfigCommentsRule) buildDurationComment(
runner tflint.Runner,
timePair hcl.KeyValuePair,
configValueInfo configTimeValueCommentInfo,
) (string, error) {
var timeVal string
diags := gohcl.DecodeExpression(timePair.Value, nil, &timeVal)
if diags.HasErrors() {
return "", diags
}

if timeVal == configValueInfo.infiniteValue {
return fmt.Sprintf("# %s forever", configValueInfo.baseComment), nil
}

timeMillis, err := strconv.Atoi(timeVal)
if err != nil {
if configValueInfo.issueWhenInvalid {
issueMsg := fmt.Sprintf(
"%s must have a valid integer value expressed in milliseconds",
configValueInfo.key,
)
err := runner.EmitIssue(r, issueMsg, timePair.Value.Range())
if err != nil {
return "", fmt.Errorf("emitting issue: invalid time value: %w", err)
}
}

return "", nil
}

baseComment := configValueInfo.baseComment

msg := buildCommentForMillis(timeMillis, baseComment)
return msg, nil
}

func buildCommentForMillis(timeMillis int, baseComment string) string {
timeUnits, unit := determineTimeUnits(timeMillis)

msg := fmt.Sprintf("# %s for %d %s", baseComment, timeUnits, unit)
return msg
}

func determineTimeUnits(millis int) (int, string) {
// todo: this is not really perfect, as if the time is not exact in millis we'll output a partial number
timeInYears := millis / millisInOneYear
if timeInYears > 0 {
if timeInYears == 1 {
return 1, "year"
}
return timeInYears, "years"
}

timeInMonths := millis / millisInOneMonth
if timeInMonths > 0 {
if timeInMonths == 1 {
return 1, "month"
}
return timeInMonths, "months"
}

timeInDays := millis / millisInOneDay
if timeInDays > 0 {
if timeInDays == 1 {
return 1, "day"
}
return timeInDays, "days"
}

timeInHours := millis / millisInOneHour
if timeInHours == 1 {
return 1, "hour"
}
return timeInHours, "hours"
}
Loading

0 comments on commit 4cb119f

Please sign in to comment.