From 5cbca87add6bb2f0ac84cd007b199c2b7421a1d3 Mon Sep 17 00:00:00 2001 From: Yu-Ju Hong Date: Wed, 18 Oct 2017 16:11:21 -0700 Subject: [PATCH] Address comments, fix lint failures, and bazel complaints --- pkg/kubelet/util/store/BUILD | 2 ++ pkg/kubelet/util/store/doc.go | 2 +- pkg/kubelet/util/store/filestore.go | 13 +++++++++---- pkg/kubelet/util/store/filestore_test.go | 8 ++++---- pkg/kubelet/util/store/store.go | 19 +++++++++++++++---- pkg/kubelet/util/store/store_test.go | 8 ++++++++ 6 files changed, 39 insertions(+), 13 deletions(-) diff --git a/pkg/kubelet/util/store/BUILD b/pkg/kubelet/util/store/BUILD index 33bb61d6c2a..8f4e66a5230 100644 --- a/pkg/kubelet/util/store/BUILD +++ b/pkg/kubelet/util/store/BUILD @@ -7,6 +7,7 @@ go_library( "filestore.go", "store.go", ], + importpath = "k8s.io/kubernetes/pkg/kubelet/util/store", visibility = ["//visibility:public"], deps = ["//pkg/util/filesystem:go_default_library"], ) @@ -17,6 +18,7 @@ go_test( "filestore_test.go", "store_test.go", ], + importpath = "k8s.io/kubernetes/pkg/kubelet/util/store", library = ":go_default_library", deps = [ "//pkg/util/filesystem:go_default_library", diff --git a/pkg/kubelet/util/store/doc.go b/pkg/kubelet/util/store/doc.go index d9f1cedfd8f..b4d8523a097 100644 --- a/pkg/kubelet/util/store/doc.go +++ b/pkg/kubelet/util/store/doc.go @@ -14,5 +14,5 @@ See the License for the specific language governing permissions and limitations under the License. */ -// A store interface. +// Package store hosts a Store interface and its implementations. package store // import "k8s.io/kubernetes/pkg/kubelet/util/store" diff --git a/pkg/kubelet/util/store/filestore.go b/pkg/kubelet/util/store/filestore.go index 94fe222ae32..fa4765fd353 100644 --- a/pkg/kubelet/util/store/filestore.go +++ b/pkg/kubelet/util/store/filestore.go @@ -39,6 +39,7 @@ type FileStore struct { filesystem utilfs.Filesystem } +// NewFileStore returns an instance of FileStore. func NewFileStore(path string, fs utilfs.Filesystem) (Store, error) { if err := ensureDirectory(fs, path); err != nil { return nil, err @@ -46,6 +47,7 @@ func NewFileStore(path string, fs utilfs.Filesystem) (Store, error) { return &FileStore{directoryPath: path, filesystem: fs}, nil } +// Write writes the given data to a file named key. func (f *FileStore) Write(key string, data []byte) error { if err := ValidateKey(key); err != nil { return err @@ -57,17 +59,19 @@ func (f *FileStore) Write(key string, data []byte) error { return writeFile(f.filesystem, f.getPathByKey(key), data) } +// Read reads the data from the file named key. func (f *FileStore) Read(key string) ([]byte, error) { if err := ValidateKey(key); err != nil { return nil, err } bytes, err := f.filesystem.ReadFile(f.getPathByKey(key)) if os.IsNotExist(err) { - return bytes, KeyNotFoundError + return bytes, ErrKeyNotFound } return bytes, err } +// Delete deletes the key file. func (f *FileStore) Delete(key string) error { if err := ValidateKey(key); err != nil { return err @@ -75,6 +79,7 @@ func (f *FileStore) Delete(key string) error { return removePath(f.filesystem, f.getPathByKey(key)) } +// List returns all keys in the store. func (f *FileStore) List() ([]string, error) { keys := make([]string, 0) files, err := f.filesystem.ReadDir(f.directoryPath) @@ -89,6 +94,7 @@ func (f *FileStore) List() ([]string, error) { return keys, nil } +// getPathByKey returns the full path of the file for the key. func (f *FileStore) getPathByKey(key string) string { return filepath.Join(f.directoryPath, key) } @@ -110,6 +116,8 @@ func writeFile(fs utilfs.Filesystem, path string, data []byte) (retErr error) { return err } + tmpPath := tmpFile.Name() + defer func() { // Close the file. if err := tmpFile.Close(); err != nil { @@ -119,10 +127,7 @@ func writeFile(fs utilfs.Filesystem, path string, data []byte) (retErr error) { retErr = fmt.Errorf("failed to close temp file after error %v", retErr) } } - }() - tmpPath := tmpFile.Name() - defer func() { // Clean up the temp file on error. if retErr != nil && tmpPath != "" { if err := removePath(fs, tmpPath); err != nil { diff --git a/pkg/kubelet/util/store/filestore_test.go b/pkg/kubelet/util/store/filestore_test.go index 567fb253d83..f5dcccc5a10 100644 --- a/pkg/kubelet/util/store/filestore_test.go +++ b/pkg/kubelet/util/store/filestore_test.go @@ -31,7 +31,7 @@ func TestFileStore(t *testing.T) { store, err := NewFileStore(path, filesystem.NewFakeFs()) assert.NoError(t, err) - testData := []struct { + testCases := []struct { key string data string expectErr bool @@ -69,7 +69,7 @@ func TestFileStore(t *testing.T) { } // Test add data. - for _, c := range testData { + for _, c := range testCases { _, err = store.Read(c.key) assert.Error(t, err) @@ -94,7 +94,7 @@ func TestFileStore(t *testing.T) { assert.Equal(t, keys, []string{"id1", "id2"}) // Test Delete data - for _, c := range testData { + for _, c := range testCases { if c.expectErr { continue } @@ -102,7 +102,7 @@ func TestFileStore(t *testing.T) { err = store.Delete(c.key) assert.NoError(t, err) _, err = store.Read(c.key) - assert.EqualValues(t, KeyNotFoundError, err) + assert.EqualValues(t, ErrKeyNotFound, err) } // Test delete non-existent key. diff --git a/pkg/kubelet/util/store/store.go b/pkg/kubelet/util/store/store.go index 62072643563..e797fd812b4 100644 --- a/pkg/kubelet/util/store/store.go +++ b/pkg/kubelet/util/store/store.go @@ -21,12 +21,21 @@ import ( "regexp" ) -const keyMaxLength = 250 +const ( + keyMaxLength = 250 -var keyRegex = regexp.MustCompile("^[a-zA-Z0-9]+$") + keyCharFmt string = "[A-Za-z0-9]" + keyExtCharFmt string = "[-A-Za-z0-9_.]" + qualifiedKeyFmt string = "(" + keyCharFmt + keyExtCharFmt + "*)?" + keyCharFmt +) var ( - KeyNotFoundError = fmt.Errorf("key is not found.") + // Key must consist of alphanumeric characters, '-', '_' or '.', and must start + // and end with an alphanumeric character. + keyRegex = regexp.MustCompile("^" + qualifiedKeyFmt + "$") + + // ErrKeyNotFound is the error returned if key is not found in Store. + ErrKeyNotFound = fmt.Errorf("key is not found") ) // Store provides the interface for storing keyed data. @@ -36,7 +45,7 @@ type Store interface { // Write writes data with key. Write(key string, data []byte) error // Read retrieves data with key - // Read must return KeyNotFoundError if key is not found. + // Read must return ErrKeyNotFound if key is not found. Read(key string) ([]byte, error) // Delete deletes data by key // Delete must not return error if key does not exist @@ -45,6 +54,8 @@ type Store interface { List() ([]string, error) } +// ValidateKey returns an error if the given key does not meet the requirement +// of the key format and length. func ValidateKey(key string) error { if len(key) <= keyMaxLength && keyRegex.MatchString(key) { return nil diff --git a/pkg/kubelet/util/store/store_test.go b/pkg/kubelet/util/store/store_test.go index e19afd3ad85..a9c6e9c14b7 100644 --- a/pkg/kubelet/util/store/store_test.go +++ b/pkg/kubelet/util/store/store_test.go @@ -43,6 +43,14 @@ func TestIsValidKey(t *testing.T) { "a78768279290d33d0b82eaea43cb8346f500057cb5bd250e88c97a5585385d66", true, }, + { + "a7.87-6_8", + true, + }, + { + "a7.87-677-", + false, + }, } for _, tc := range testcases {