From bd3ccf1e3386f94d80c8bcae207c98471e23a249 Mon Sep 17 00:00:00 2001 From: AhmedGrati Date: Fri, 28 Jul 2023 16:40:04 +0100 Subject: [PATCH 1/3] feat: add support for feature files expiration Signed-off-by: AhmedGrati --- source/local/local.go | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/source/local/local.go b/source/local/local.go index 9d1459572..c8e8f1193 100644 --- a/source/local/local.go +++ b/source/local/local.go @@ -23,6 +23,7 @@ import ( "os/exec" "path/filepath" "strings" + "time" "k8s.io/klog/v2" @@ -37,6 +38,10 @@ const Name = "local" // LabelFeature of this feature source const LabelFeature = "label" +// ExpiryDateKey is the key of this feature source indicating +// when features should be removed. +const ExpiryDateKey = "# +expiry-time" + // Config var ( featureFilesDir = "/etc/kubernetes/node-feature-discovery/features.d/" @@ -271,9 +276,24 @@ func getFeaturesFromFiles() (map[string]string, error) { klog.ErrorS(err, "failed to read file", "fileName", fileName) continue } - + // Append features fileFeatures := parseFeatures(lines) + + // Check expiration of file features + if expiryDate, ok := fileFeatures[ExpiryDateKey]; ok { + expiryDate, err := time.Parse(time.RFC3339, expiryDate) + if err != nil { + klog.ErrorS(err, "failed to parse feature file expiry date", "fileName", fileName) + continue + } + + // Features should not be included if they're expired + if expiryDate.Before(time.Now()) { + continue + } + } + klog.V(4).InfoS("feature file read", "fileName", fileName, "features", utils.DelayedDumper(fileFeatures)) for k, v := range fileFeatures { if old, ok := features[k]; ok { From f0edc6532af0f87e6e4a1aa6da3ba1ec00fd83fb Mon Sep 17 00:00:00 2001 From: AhmedGrati Date: Fri, 28 Jul 2023 16:46:17 +0100 Subject: [PATCH 2/3] docs: add the support of the exipration date in the input format of the feature files Signed-off-by: AhmedGrati --- docs/usage/customization-guide.md | 8 +++++++ source/local/local.go | 40 ++++++++++++++++++++++--------- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/docs/usage/customization-guide.md b/docs/usage/customization-guide.md index 773e0dffe..6fe360caf 100644 --- a/docs/usage/customization-guide.md +++ b/docs/usage/customization-guide.md @@ -316,6 +316,14 @@ Label namespace may be specified with `/[=]`. Comment lines (starting with `#`) are ignored. +You can include the following block if you want to define the expiration date +of features that are described in the feature files: + +```plaintext +# +expiry-time: 2023-07-29T11:22:33Z +``` +**Note: The time format that we are supporting is RFC3339.** + ### Mounts The standard NFD deployments contain `hostPath` mounts for diff --git a/source/local/local.go b/source/local/local.go index c8e8f1193..0faeea756 100644 --- a/source/local/local.go +++ b/source/local/local.go @@ -276,22 +276,19 @@ func getFeaturesFromFiles() (map[string]string, error) { klog.ErrorS(err, "failed to read file", "fileName", fileName) continue } - + // Append features fileFeatures := parseFeatures(lines) // Check expiration of file features - if expiryDate, ok := fileFeatures[ExpiryDateKey]; ok { - expiryDate, err := time.Parse(time.RFC3339, expiryDate) - if err != nil { - klog.ErrorS(err, "failed to parse feature file expiry date", "fileName", fileName) - continue - } + expiryDate, err := getExpirationDate(lines) + if err != nil { + klog.ErrorS(err, "failed to parse feature file expiry date", "fileName", fileName) + continue + } - // Features should not be included if they're expired - if expiryDate.Before(time.Now()) { - continue - } + if expiryDate.Before(time.Now()) { + continue } klog.V(4).InfoS("feature file read", "fileName", fileName, "features", utils.DelayedDumper(fileFeatures)) @@ -306,6 +303,27 @@ func getFeaturesFromFiles() (map[string]string, error) { return features, nil } +// Return the expiration date of a feature file +func getExpirationDate(lines [][]byte) (time.Time, error) { + for _, line := range lines { + if len(line) > 0 { + lineSplit := strings.SplitN(string(line), ":", 2) + + key := lineSplit[0] + + if key == ExpiryDateKey { + expiryDate, err := time.Parse(time.RFC3339, lineSplit[1]) + if err != nil { + return time.Now(), err + } + return expiryDate, nil + } + } + } + + return time.Now(), nil +} + // Read one file func getFileContent(fileName string) ([][]byte, error) { var lines [][]byte From 47aec15ea1192aaff89399f1461c05afc6e785f3 Mon Sep 17 00:00:00 2001 From: AhmedGrati Date: Sat, 29 Jul 2023 19:17:08 +0100 Subject: [PATCH 3/3] test: add unit tests for the expiration date function Signed-off-by: AhmedGrati --- docs/usage/customization-guide.md | 32 ++++++- source/local/local.go | 94 +++++++++++-------- source/local/local_test.go | 49 ++++++++++ .../local/testdata/features.d/expired_feature | 2 + .../testdata/features.d/feature_with_comments | 2 + .../features.d/multiple_expiration_dates | 11 +++ .../features.d/unparsable_expiry_comment | 2 + .../local/testdata/features.d/valid_feature | 2 + 8 files changed, 152 insertions(+), 42 deletions(-) create mode 100644 source/local/testdata/features.d/expired_feature create mode 100644 source/local/testdata/features.d/feature_with_comments create mode 100644 source/local/testdata/features.d/multiple_expiration_dates create mode 100644 source/local/testdata/features.d/unparsable_expiry_comment create mode 100644 source/local/testdata/features.d/valid_feature diff --git a/docs/usage/customization-guide.md b/docs/usage/customization-guide.md index 6fe360caf..d49e31892 100644 --- a/docs/usage/customization-guide.md +++ b/docs/usage/customization-guide.md @@ -316,13 +316,37 @@ Label namespace may be specified with `/[=]`. Comment lines (starting with `#`) are ignored. -You can include the following block if you want to define the expiration date -of features that are described in the feature files: +Adding following line anywhere to feature file defines date when +its content expires / is ignored: ```plaintext -# +expiry-time: 2023-07-29T11:22:33Z +# +expiry-time=2023-07-29T11:22:33Z ``` -**Note: The time format that we are supporting is RFC3339.** + +Also, the expiry-time value would stay the same during the processing of the +feature file until another expiry-time directive is encountered. +Considering the following file: + +```plaintext +# +expiry-time=2012-07-28T11:22:33Z +featureKey=featureValue + +# +expiry-time=2080-07-28T11:22:33Z +featureKey2=featureValue2 + +# +expiry-time=2070-07-28T11:22:33Z +featureKey3=featureValue3 + +# +expiry-time=2002-07-28T11:22:33Z +featureKey4=featureValue4 +``` + +After processing the above file, only `featureKey2` and `featureKey3` would be +included in the list of accepted features. + +> **NOTE:** The time format that we are supporting is RFC3339. Also, the `expiry-time` +> tag is only evaluated in each re-discovery period, and the expiration of +> node labels is not tracked. ### Mounts diff --git a/source/local/local.go b/source/local/local.go index 0faeea756..3f2529bcc 100644 --- a/source/local/local.go +++ b/source/local/local.go @@ -38,9 +38,12 @@ const Name = "local" // LabelFeature of this feature source const LabelFeature = "label" -// ExpiryDateKey is the key of this feature source indicating +// ExpiryTimeKey is the key of this feature source indicating // when features should be removed. -const ExpiryDateKey = "# +expiry-time" +const ExpiryTimeKey = "expiry-time" + +// DirectivePrefix defines the prefix of directives that should be parsed +const DirectivePrefix = "# +" // Config var ( @@ -58,6 +61,11 @@ type Config struct { HooksEnabled bool `json:"hooksEnabled,omitempty"` } +// parsingOpts contains options used for directives parsing +type parsingOpts struct { + ExpiryTime time.Time +} + // Singleton source instance var ( src = localSource{config: newDefaultConfig()} @@ -149,14 +157,56 @@ func (s *localSource) GetFeatures() *nfdv1alpha1.Features { return s.features } -func parseFeatures(lines [][]byte) map[string]string { +func parseDirectives(line string, opts *parsingOpts) error { + if !strings.HasPrefix(line, DirectivePrefix) { + return nil + } + + directive := line[len(DirectivePrefix):] + split := strings.SplitN(directive, "=", 2) + key := split[0] + + if len(split) == 1 { + return fmt.Errorf("invalid directive format in %q, should be '# +key=value'", line) + } + value := split[1] + + switch key { + case ExpiryTimeKey: + expiryDate, err := time.Parse(time.RFC3339, strings.TrimSpace(value)) + if err != nil { + return fmt.Errorf("failed to parse expiry-date directive: %w", err) + } + opts.ExpiryTime = expiryDate + default: + return fmt.Errorf("unknown feature file directive %q", key) + } + + return nil +} + +func parseFeatures(lines [][]byte, fileName string) map[string]string { features := make(map[string]string) + now := time.Now() + parsingOpts := &parsingOpts{ + ExpiryTime: now, + } for _, l := range lines { line := strings.TrimSpace(string(l)) if len(line) > 0 { - // Skip comment lines if strings.HasPrefix(line, "#") { + // Parse directives + err := parseDirectives(line, parsingOpts) + if err != nil { + klog.ErrorS(err, "error while parsing directives", "fileName", fileName) + } + + continue + } + + // handle expiration + if parsingOpts.ExpiryTime.Before(now) { continue } @@ -202,7 +252,7 @@ func getFeaturesFromHooks() (map[string]string, error) { } // Append features - fileFeatures := parseFeatures(lines) + fileFeatures := parseFeatures(lines, fileName) klog.V(4).InfoS("hook executed", "fileName", fileName, "features", utils.DelayedDumper(fileFeatures)) for k, v := range fileFeatures { if old, ok := features[k]; ok { @@ -278,18 +328,7 @@ func getFeaturesFromFiles() (map[string]string, error) { } // Append features - fileFeatures := parseFeatures(lines) - - // Check expiration of file features - expiryDate, err := getExpirationDate(lines) - if err != nil { - klog.ErrorS(err, "failed to parse feature file expiry date", "fileName", fileName) - continue - } - - if expiryDate.Before(time.Now()) { - continue - } + fileFeatures := parseFeatures(lines, fileName) klog.V(4).InfoS("feature file read", "fileName", fileName, "features", utils.DelayedDumper(fileFeatures)) for k, v := range fileFeatures { @@ -303,27 +342,6 @@ func getFeaturesFromFiles() (map[string]string, error) { return features, nil } -// Return the expiration date of a feature file -func getExpirationDate(lines [][]byte) (time.Time, error) { - for _, line := range lines { - if len(line) > 0 { - lineSplit := strings.SplitN(string(line), ":", 2) - - key := lineSplit[0] - - if key == ExpiryDateKey { - expiryDate, err := time.Parse(time.RFC3339, lineSplit[1]) - if err != nil { - return time.Now(), err - } - return expiryDate, nil - } - } - } - - return time.Now(), nil -} - // Read one file func getFileContent(fileName string) ([][]byte, error) { var lines [][]byte diff --git a/source/local/local_test.go b/source/local/local_test.go index e83493c63..08d20b77c 100644 --- a/source/local/local_test.go +++ b/source/local/local_test.go @@ -17,7 +17,11 @@ limitations under the License. package local import ( + "fmt" + "os" + "path/filepath" "testing" + "time" "github.com/stretchr/testify/assert" ) @@ -33,3 +37,48 @@ func TestLocalSource(t *testing.T) { assert.Empty(t, l) } + +func TestGetExpirationDate(t *testing.T) { + expectedFeaturesLen := 5 + pwd, _ := os.Getwd() + featureFilesDir = filepath.Join(pwd, "testdata/features.d") + + features, err := getFeaturesFromFiles() + fmt.Println(features) + assert.NoError(t, err) + assert.Equal(t, expectedFeaturesLen, len(features)) +} + +func TestParseDirectives(t *testing.T) { + testCases := []struct { + name string + directive string + wantErr bool + }{ + { + name: "valid directive", + directive: "# +expiry-time=2080-07-28T11:22:33Z", + wantErr: false, + }, + { + name: "invalid directive", + directive: "# +random-key=random-value", + wantErr: true, + }, + { + name: "invalid directive format", + directive: "# + Something", + wantErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + parsingOpts := parsingOpts{ + ExpiryTime: time.Now(), + } + err := parseDirectives(tc.directive, &parsingOpts) + assert.Equal(t, err != nil, tc.wantErr) + }) + } +} diff --git a/source/local/testdata/features.d/expired_feature b/source/local/testdata/features.d/expired_feature new file mode 100644 index 000000000..d92bc7d50 --- /dev/null +++ b/source/local/testdata/features.d/expired_feature @@ -0,0 +1,2 @@ +# +expiry-time=2012-07-28T11:22:33Z +featureKeyExpired=featureValue diff --git a/source/local/testdata/features.d/feature_with_comments b/source/local/testdata/features.d/feature_with_comments new file mode 100644 index 000000000..03dac9adb --- /dev/null +++ b/source/local/testdata/features.d/feature_with_comments @@ -0,0 +1,2 @@ +# Notes: foo bar +featureKeyRandomComment=featureValue diff --git a/source/local/testdata/features.d/multiple_expiration_dates b/source/local/testdata/features.d/multiple_expiration_dates new file mode 100644 index 000000000..e6fd0ba3c --- /dev/null +++ b/source/local/testdata/features.d/multiple_expiration_dates @@ -0,0 +1,11 @@ +# +expiry-time=2012-07-28T11:22:33Z +featureKey=featureValue + +# +expiry-time=2080-07-28T11:22:33Z +featureKey2=featureValue2 + +# +expiry-time=2070-07-28T11:22:33Z +featureKey3=featureValue3 + +# +expiry-time=2002-07-28T11:22:33Z +featureKey4=featureValue4 diff --git a/source/local/testdata/features.d/unparsable_expiry_comment b/source/local/testdata/features.d/unparsable_expiry_comment new file mode 100644 index 000000000..75ec2f272 --- /dev/null +++ b/source/local/testdata/features.d/unparsable_expiry_comment @@ -0,0 +1,2 @@ +# +expiry-time=2080-07-28T11:22:33X +featureKeyUnparsable=featureValue diff --git a/source/local/testdata/features.d/valid_feature b/source/local/testdata/features.d/valid_feature new file mode 100644 index 000000000..9c0fdd307 --- /dev/null +++ b/source/local/testdata/features.d/valid_feature @@ -0,0 +1,2 @@ +# +expiry-time=2080-07-28T11:22:33Z +featureKeyValid=featureValue