From 25a187d27008a4154ae366d0bdf221891d784537 Mon Sep 17 00:00:00 2001 From: devinyan Date: Sat, 10 Jun 2017 17:28:02 +0800 Subject: [PATCH 1/5] give an uniform check for release process --- pkg/tiller/release_content.go | 5 +++-- pkg/tiller/release_history.go | 5 +++++ pkg/tiller/release_rollback.go | 10 ++++++---- pkg/tiller/release_server.go | 10 ++++++++++ pkg/tiller/release_server_test.go | 3 ++- pkg/tiller/release_status.go | 5 +++-- pkg/tiller/release_testing.go | 5 +++-- pkg/tiller/release_uninstall.go | 10 +++------- pkg/tiller/release_update.go | 9 +++++---- 9 files changed, 40 insertions(+), 22 deletions(-) diff --git a/pkg/tiller/release_content.go b/pkg/tiller/release_content.go index 7586eb2d8..dc7d86d97 100644 --- a/pkg/tiller/release_content.go +++ b/pkg/tiller/release_content.go @@ -23,8 +23,9 @@ import ( // GetReleaseContent gets all of the stored information for the given release. func (s *ReleaseServer) GetReleaseContent(c ctx.Context, req *services.GetReleaseContentRequest) (*services.GetReleaseContentResponse, error) { - if !ValidName.MatchString(req.Name) { - return nil, errMissingRelease + if err := validateReleaseName(req.Name); err != nil { + s.Log("releaseContent: Release name is invalid: %s", req.Name) + return nil, err } if req.Version <= 0 { diff --git a/pkg/tiller/release_history.go b/pkg/tiller/release_history.go index 09131fad8..0dd525978 100644 --- a/pkg/tiller/release_history.go +++ b/pkg/tiller/release_history.go @@ -25,6 +25,11 @@ import ( // GetHistory gets the history for a given release. func (s *ReleaseServer) GetHistory(ctx context.Context, req *tpb.GetHistoryRequest) (*tpb.GetHistoryResponse, error) { + if err := validateReleaseName(req.Name); err != nil { + s.Log("getHistory: Release name is invalid: %s", req.Name) + return nil, err + } + s.Log("getting history for release %s", req.Name) h, err := s.env.Releases.History(req.Name) if err != nil { diff --git a/pkg/tiller/release_rollback.go b/pkg/tiller/release_rollback.go index 43e06a6b6..5a2f57261 100644 --- a/pkg/tiller/release_rollback.go +++ b/pkg/tiller/release_rollback.go @@ -59,10 +59,12 @@ func (s *ReleaseServer) RollbackRelease(c ctx.Context, req *services.RollbackRel // prepareRollback finds the previous release and prepares a new release object with // the previous release's configuration func (s *ReleaseServer) prepareRollback(req *services.RollbackReleaseRequest) (*release.Release, *release.Release, error) { - switch { - case !ValidName.MatchString(req.Name): - return nil, nil, errMissingRelease - case req.Version < 0: + if err := validateReleaseName(req.Name); err != nil { + s.Log("prepareRollback: Release name is invalid: %s", req.Name) + return nil, err + } + + if req.Version < 0 { return nil, nil, errInvalidRevision } diff --git a/pkg/tiller/release_server.go b/pkg/tiller/release_server.go index 07ea872df..6f691cb39 100644 --- a/pkg/tiller/release_server.go +++ b/pkg/tiller/release_server.go @@ -59,6 +59,8 @@ var ( errMissingRelease = errors.New("no release provided") // errInvalidRevision indicates that an invalid release revision number was provided. errInvalidRevision = errors.New("invalid release revision") + //errInvalidName indicates that an invalid release name was provided + errInvalidName = errors.New("invalid release name") ) // ListDefaultLimit is the default limit for number of items returned in a list. @@ -359,3 +361,11 @@ func validateManifest(c environment.KubeClient, ns string, manifest []byte) erro _, err := c.BuildUnstructured(ns, r) return err } + +func validateReleaseName(releaseName string) error { + if !ValidName.MatchString(releaseName) || (len(releaseName) > releaseNameMaxLen) { + return errInvalidName + } + + return nil +} diff --git a/pkg/tiller/release_server_test.go b/pkg/tiller/release_server_test.go index 8425a58f5..6008a56e0 100644 --- a/pkg/tiller/release_server_test.go +++ b/pkg/tiller/release_server_test.go @@ -197,8 +197,9 @@ func TestValidName(t *testing.T) { " ": false, ".nina.": false, "nina.pinta": true, + "abcdefghi-abcdefghi-abcdefghi-abcdefghi-abcdefghi-abc": false, } { - if valid != ValidName.MatchString(name) { + if valid != validateReleaseName(name) { t.Errorf("Expected %q to be %t", name, valid) } } diff --git a/pkg/tiller/release_status.go b/pkg/tiller/release_status.go index 41eba1174..aa88b8001 100644 --- a/pkg/tiller/release_status.go +++ b/pkg/tiller/release_status.go @@ -26,8 +26,9 @@ import ( // GetReleaseStatus gets the status information for a named release. func (s *ReleaseServer) GetReleaseStatus(c ctx.Context, req *services.GetReleaseStatusRequest) (*services.GetReleaseStatusResponse, error) { - if !ValidName.MatchString(req.Name) { - return nil, errMissingRelease + if err := validateReleaseName(req.Name); err != nil { + s.Log("getStatus: Release name is invalid: %s", req.Name) + return nil, err } var rel *release.Release diff --git a/pkg/tiller/release_testing.go b/pkg/tiller/release_testing.go index 4f9a38a96..bd31241ac 100644 --- a/pkg/tiller/release_testing.go +++ b/pkg/tiller/release_testing.go @@ -25,8 +25,9 @@ import ( // RunReleaseTest runs pre-defined tests stored as hooks on a given release func (s *ReleaseServer) RunReleaseTest(req *services.TestReleaseRequest, stream services.ReleaseService_RunReleaseTestServer) error { - if !ValidName.MatchString(req.Name) { - return errMissingRelease + if err := validateReleaseName(req.Name); err != nil { + s.Log("releaseTest: Release name is invalid: %s", req.Name) + return nil, err } // finds the non-deleted release with the given name diff --git a/pkg/tiller/release_uninstall.go b/pkg/tiller/release_uninstall.go index 54971ee6e..74601add1 100644 --- a/pkg/tiller/release_uninstall.go +++ b/pkg/tiller/release_uninstall.go @@ -30,13 +30,9 @@ import ( // UninstallRelease deletes all of the resources associated with this release, and marks the release DELETED. func (s *ReleaseServer) UninstallRelease(c ctx.Context, req *services.UninstallReleaseRequest) (*services.UninstallReleaseResponse, error) { - if !ValidName.MatchString(req.Name) { - s.Log("uninstall: Release not found: %s", req.Name) - return nil, errMissingRelease - } - - if len(req.Name) > releaseNameMaxLen { - return nil, fmt.Errorf("release name %q exceeds max length of %d", req.Name, releaseNameMaxLen) + if err := validateReleaseName(req.Name); err != nil { + s.Log("uninstallRelease: Release name is invalid: %s", req.Name) + return nil, err } err := s.env.Releases.LockRelease(req.Name) diff --git a/pkg/tiller/release_update.go b/pkg/tiller/release_update.go index fb30d1661..74e8730f9 100644 --- a/pkg/tiller/release_update.go +++ b/pkg/tiller/release_update.go @@ -29,6 +29,11 @@ import ( // UpdateRelease takes an existing release and new information, and upgrades the release. func (s *ReleaseServer) UpdateRelease(c ctx.Context, req *services.UpdateReleaseRequest) (*services.UpdateReleaseResponse, error) { + if err := validateReleaseName(req.Name); err != nil { + s.Log("updateRelease: Release name is invalid: %s", req.Name) + return nil, err + } + err := s.env.Releases.LockRelease(req.Name) if err != nil { return nil, err @@ -59,10 +64,6 @@ func (s *ReleaseServer) UpdateRelease(c ctx.Context, req *services.UpdateRelease // prepareUpdate builds an updated release for an update operation. func (s *ReleaseServer) prepareUpdate(req *services.UpdateReleaseRequest) (*release.Release, *release.Release, error) { - if !ValidName.MatchString(req.Name) { - return nil, nil, errMissingRelease - } - if req.Chart == nil { return nil, nil, errMissingChart } From dc7e2a52a973e194bb4d65553a7b18e11b77483a Mon Sep 17 00:00:00 2001 From: devinyan Date: Sat, 10 Jun 2017 19:32:17 +0800 Subject: [PATCH 2/5] give an uniform check for release process bugfix --- pkg/tiller/release_rollback.go | 2 +- pkg/tiller/release_testing.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/tiller/release_rollback.go b/pkg/tiller/release_rollback.go index 5a2f57261..a676362ab 100644 --- a/pkg/tiller/release_rollback.go +++ b/pkg/tiller/release_rollback.go @@ -61,7 +61,7 @@ func (s *ReleaseServer) RollbackRelease(c ctx.Context, req *services.RollbackRel func (s *ReleaseServer) prepareRollback(req *services.RollbackReleaseRequest) (*release.Release, *release.Release, error) { if err := validateReleaseName(req.Name); err != nil { s.Log("prepareRollback: Release name is invalid: %s", req.Name) - return nil, err + return nil, nil, err } if req.Version < 0 { diff --git a/pkg/tiller/release_testing.go b/pkg/tiller/release_testing.go index bd31241ac..a44b67e6f 100644 --- a/pkg/tiller/release_testing.go +++ b/pkg/tiller/release_testing.go @@ -27,7 +27,7 @@ func (s *ReleaseServer) RunReleaseTest(req *services.TestReleaseRequest, stream if err := validateReleaseName(req.Name); err != nil { s.Log("releaseTest: Release name is invalid: %s", req.Name) - return nil, err + return err } // finds the non-deleted release with the given name From 35b5a6753e0b2fb7817a623446f33a15a975d217 Mon Sep 17 00:00:00 2001 From: devinyan Date: Sat, 10 Jun 2017 19:57:47 +0800 Subject: [PATCH 3/5] give an uniform check for release process bugfix --- pkg/tiller/release_server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/tiller/release_server_test.go b/pkg/tiller/release_server_test.go index 6008a56e0..61c41d6d5 100644 --- a/pkg/tiller/release_server_test.go +++ b/pkg/tiller/release_server_test.go @@ -199,7 +199,7 @@ func TestValidName(t *testing.T) { "nina.pinta": true, "abcdefghi-abcdefghi-abcdefghi-abcdefghi-abcdefghi-abc": false, } { - if valid != validateReleaseName(name) { + if valid != (validateReleaseName(name) == nil) { t.Errorf("Expected %q to be %t", name, valid) } } From 715fdf1d0fac0f003721967a4a733768c8b36834 Mon Sep 17 00:00:00 2001 From: devinyan Date: Sat, 10 Jun 2017 20:33:34 +0800 Subject: [PATCH 4/5] give an uniform check for release process bugfix3 --- pkg/tiller/release_server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/tiller/release_server_test.go b/pkg/tiller/release_server_test.go index 61c41d6d5..f73eb7abf 100644 --- a/pkg/tiller/release_server_test.go +++ b/pkg/tiller/release_server_test.go @@ -197,7 +197,7 @@ func TestValidName(t *testing.T) { " ": false, ".nina.": false, "nina.pinta": true, - "abcdefghi-abcdefghi-abcdefghi-abcdefghi-abcdefghi-abc": false, + "abcdefghi-abcdefghi-abcdefghi-abcdefghi-abcdefghi-abcd": false, } { if valid != (validateReleaseName(name) == nil) { t.Errorf("Expected %q to be %t", name, valid) From 645cc4e8f6e20407081344513fb57cdc4c19a552 Mon Sep 17 00:00:00 2001 From: devinyan Date: Tue, 13 Jun 2017 22:00:45 +0800 Subject: [PATCH 5/5] fixed as the review of adamreese: update the err message when releasename is empty and update the test units. --- pkg/tiller/release_server.go | 4 ++++ pkg/tiller/release_server_test.go | 32 +++++++++++++++---------------- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/pkg/tiller/release_server.go b/pkg/tiller/release_server.go index 6f691cb39..d28d6796e 100644 --- a/pkg/tiller/release_server.go +++ b/pkg/tiller/release_server.go @@ -363,6 +363,10 @@ func validateManifest(c environment.KubeClient, ns string, manifest []byte) erro } func validateReleaseName(releaseName string) error { + if releaseName == "" { + return errMissingRelease + } + if !ValidName.MatchString(releaseName) || (len(releaseName) > releaseNameMaxLen) { return errInvalidName } diff --git a/pkg/tiller/release_server_test.go b/pkg/tiller/release_server_test.go index f73eb7abf..b7b14a4f1 100644 --- a/pkg/tiller/release_server_test.go +++ b/pkg/tiller/release_server_test.go @@ -183,23 +183,23 @@ func upgradeReleaseVersion(rel *release.Release) *release.Release { } func TestValidName(t *testing.T) { - for name, valid := range map[string]bool{ - "nina pinta santa-maria": false, - "nina-pinta-santa-maria": true, - "-nina": false, - "pinta-": false, - "santa-maria": true, - "niña": false, - "...": false, - "pinta...": false, - "santa...maria": true, - "": false, - " ": false, - ".nina.": false, - "nina.pinta": true, - "abcdefghi-abcdefghi-abcdefghi-abcdefghi-abcdefghi-abcd": false, + for name, valid := range map[string]error{ + "nina pinta santa-maria": errInvalidName, + "nina-pinta-santa-maria": nil, + "-nina": errInvalidName, + "pinta-": errInvalidName, + "santa-maria": nil, + "niña": errInvalidName, + "...": errInvalidName, + "pinta...": errInvalidName, + "santa...maria": nil, + "": errMissingRelease, + " ": errInvalidName, + ".nina.": errInvalidName, + "nina.pinta": nil, + "abcdefghi-abcdefghi-abcdefghi-abcdefghi-abcdefghi-abcd": errInvalidName, } { - if valid != (validateReleaseName(name) == nil) { + if valid != validateReleaseName(name) { t.Errorf("Expected %q to be %t", name, valid) } }