Merge pull request #4242 from gsquared94/fix-2806-update

Add validations to Control API for Auto Triggers

docs/content/en/docs/design/api.md

@@ -323,12 +323,13 @@ func main() {
     defer ctxCancel()
     // `client` is the gRPC client with connection to localhost:50051.
     _, err = client.AutoBuild(ctx, &pb.TriggerRequest{
-        State: &pb.TriggerState{
-            Enabled:  true,
-        },
-    })
-    if err != nil {
-        log.Fatalf("error when trying to execute phases: %v", err)
+		State: &pb.TriggerState{
+			Val: &pb.TriggerState_Enabled{
+				Enabled: true,
+			},
+		},
+	})    if err != nil {
+        log.Fatalf("error when trying to auto trigger phases: %v", err)
     }
 }
 ```

integration/dev_test.go

@@ -162,7 +162,9 @@ func TestDevAPIAutoTriggers(t *testing.T) {
 	// Enable auto build
 	rpcClient.AutoBuild(context.Background(), &proto.TriggerRequest{
 		State: &proto.TriggerState{
-			Enabled: true,
+			Val: &proto.TriggerState_Enabled{
+				Enabled: true,
+			},
 		},
 	})
 	// Ensure we see a build triggered in the event log
@@ -174,7 +176,9 @@ func TestDevAPIAutoTriggers(t *testing.T) {
 
 	rpcClient.AutoDeploy(context.Background(), &proto.TriggerRequest{
 		State: &proto.TriggerState{
-			Enabled: true,
+			Val: &proto.TriggerState_Enabled{
+				Enabled: true,
+			},
 		},
 	})
 	verifyDeployment(t, entries, client, dep)

integration/sync_test.go

@@ -220,7 +220,9 @@ func TestDevAutoSyncAPITrigger(t *testing.T) {
 
 	rpcClient.AutoSync(context.Background(), &proto.TriggerRequest{
 		State: &proto.TriggerState{
-			Enabled: true,
+			Val: &proto.TriggerState_Enabled{
+				Enabled: true,
+			},
 		},
 	})
 
@@ -233,7 +235,9 @@ func TestDevAutoSyncAPITrigger(t *testing.T) {
 
 	rpcClient.AutoSync(context.Background(), &proto.TriggerRequest{
 		State: &proto.TriggerState{
-			Enabled: false,
+			Val: &proto.TriggerState_Enabled{
+				Enabled: true,
+			},
 		},
 	})
 }

pkg/skaffold/server/endpoints.go

@@ -20,6 +20,8 @@ import (
 	"context"
 
 	"github.com/golang/protobuf/ptypes/empty"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/event"
 	"github.com/GoogleContainerTools/skaffold/proto"
@@ -67,61 +69,41 @@ func (s *server) Execute(ctx context.Context, intent *proto.UserIntentRequest) (
 }
 
 func (s *server) AutoBuild(ctx context.Context, request *proto.TriggerRequest) (res *empty.Empty, err error) {
-	res = &empty.Empty{}
-	autoBuild := request.GetState().Enabled
-	updateAutoBuild, err := event.AutoTriggerDiff("build", autoBuild)
-	if err != nil {
-		return
-	}
-	if !updateAutoBuild {
-		return
-	}
-	event.UpdateStateAutoBuildTrigger(autoBuild)
-	if autoBuild {
-		// reset state only when autoBuild is being set to true
-		event.ResetStateOnBuild()
-	}
-	go func() {
-		s.autoBuildCallback(autoBuild)
-	}()
-	return
+	return executeAutoTrigger("build", request, event.UpdateStateAutoBuildTrigger, event.ResetStateOnBuild, s.autoBuildCallback)
 }
 
 func (s *server) AutoDeploy(ctx context.Context, request *proto.TriggerRequest) (res *empty.Empty, err error) {
-	res = &empty.Empty{}
-	autoDeploy := request.GetState().Enabled
-	updateAutoDeploy, err := event.AutoTriggerDiff("deploy", autoDeploy)
-	if err != nil {
-		return
-	}
-	if !updateAutoDeploy {
-		return
-	}
-
-	event.UpdateStateAutoDeployTrigger(autoDeploy)
-	if autoDeploy {
-		// reset state only when autoDeploy is being set to true
-		event.ResetStateOnDeploy()
-	}
-	go func() {
-		s.autoDeployCallback(autoDeploy)
-	}()
-	return
+	return executeAutoTrigger("deploy", request, event.UpdateStateAutoDeployTrigger, event.ResetStateOnDeploy, s.autoDeployCallback)
 }
 
 func (s *server) AutoSync(ctx context.Context, request *proto.TriggerRequest) (res *empty.Empty, err error) {
+	return executeAutoTrigger("sync", request, event.UpdateStateAutoSyncTrigger, func() {}, s.autoSyncCallback)
+}
+
+func executeAutoTrigger(triggerName string, request *proto.TriggerRequest, updateTriggerStateFunc func(bool), resetPhaseStateFunc func(), serverCallback func(bool)) (res *empty.Empty, err error) {
 	res = &empty.Empty{}
-	autoSync := request.GetState().Enabled
-	updateAutoSync, err := event.AutoTriggerDiff("sync", autoSync)
+	v, ok := request.GetState().GetVal().(*proto.TriggerState_Enabled)
+	if !ok {
+		err = status.Error(codes.InvalidArgument, "missing required boolean parameter 'enabled'")
+		return
+	}
+	trigger := v.Enabled
+	update, err := event.AutoTriggerDiff(triggerName, trigger)
 	if err != nil {
 		return
 	}
-	if !updateAutoSync {
+	if !update {
+		err = status.Errorf(codes.AlreadyExists, "auto %v is already set to %t", triggerName, trigger)
 		return
 	}
-	event.UpdateStateAutoSyncTrigger(autoSync)
+	// update trigger state
+	updateTriggerStateFunc(trigger)
+	if trigger {
+		// reset phase state only when auto trigger is being set to true
+		resetPhaseStateFunc()
+	}
 	go func() {
-		s.autoSyncCallback(autoSync)
+		serverCallback(trigger)
 	}()
 	return
 }

pkg/skaffold/server/server.go

@@ -18,6 +18,7 @@ package server
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"net"
@@ -26,6 +27,7 @@ import (
 	"github.com/grpc-ecosystem/grpc-gateway/runtime"
 	"github.com/sirupsen/logrus"
 	"google.golang.org/grpc"
+	"google.golang.org/grpc/status"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
@@ -159,7 +161,7 @@ func newGRPCServer(port int) (func() error, error) {
 }
 
 func newHTTPServer(port, proxyPort int) (func() error, error) {
-	mux := runtime.NewServeMux()
+	mux := runtime.NewServeMux(runtime.WithProtoErrorHandler(errorHandler))
 	opts := []grpc.DialOption{grpc.WithInsecure()}
 	err := proto.RegisterSkaffoldServiceHandlerFromEndpoint(context.Background(), mux, fmt.Sprintf("%s:%d", util.Loopback, proxyPort), opts)
 	if err != nil {
@@ -176,3 +178,18 @@ func newHTTPServer(port, proxyPort int) (func() error, error) {
 
 	return l.Close, nil
 }
+
+type errResponse struct {
+	Err string `json:"error,omitempty"`
+}
+
+func errorHandler(ctx context.Context, _ *runtime.ServeMux, marshaler runtime.Marshaler, writer http.ResponseWriter, _ *http.Request, err error) {
+	writer.Header().Set("Content-type", marshaler.ContentType())
+	s, _ := status.FromError(err)
+	writer.WriteHeader(runtime.HTTPStatusFromCode(s.Code()))
+	if err := json.NewEncoder(writer).Encode(errResponse{
+		Err: s.Message(),
+	}); err != nil {
+		writer.Write([]byte(`{"error": "failed to marshal error message"}`))
+	}
+}