Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the tool registry in the piped side #5343

Merged
merged 3 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 30 additions & 6 deletions pkg/app/pipedv1/cmd/piped/grpcapi/plugin_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import (
"context"
"fmt"
"os"

"github.com/pipe-cd/pipecd/pkg/app/pipedv1/cmd/piped/service"
"github.com/pipe-cd/pipecd/pkg/app/server/service/pipedservice"
Expand All @@ -34,7 +35,8 @@
cfg *config.PipedSpec
apiClient apiClient

Logger *zap.Logger
toolRegistry *toolRegistry
Logger *zap.Logger
}

type apiClient interface {
Expand All @@ -47,12 +49,18 @@
service.RegisterPluginServiceServer(server, a)
}

func NewPluginAPI(cfg *config.PipedSpec, apiClient apiClient, logger *zap.Logger) *PluginAPI {
return &PluginAPI{
cfg: cfg,
apiClient: apiClient,
Logger: logger.Named("plugin-api"),
func NewPluginAPI(cfg *config.PipedSpec, apiClient apiClient, toolsDir string, logger *zap.Logger) (*PluginAPI, error) {
toolRegistry, err := newToolRegistry(toolsDir, os.TempDir())
if err != nil {
return nil, fmt.Errorf("failed to create tool registry: %w", err)

Check warning on line 55 in pkg/app/pipedv1/cmd/piped/grpcapi/plugin_api.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipedv1/cmd/piped/grpcapi/plugin_api.go#L52-L55

Added lines #L52 - L55 were not covered by tests
}

return &PluginAPI{
cfg: cfg,
apiClient: apiClient,
toolRegistry: toolRegistry,
Logger: logger.Named("plugin-api"),
}, nil

Check warning on line 63 in pkg/app/pipedv1/cmd/piped/grpcapi/plugin_api.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipedv1/cmd/piped/grpcapi/plugin_api.go#L58-L63

Added lines #L58 - L63 were not covered by tests
}

func (a *PluginAPI) DecryptSecret(ctx context.Context, req *service.DecryptSecretRequest) (*service.DecryptSecretResponse, error) {
Expand Down Expand Up @@ -80,6 +88,22 @@
}, nil
}

// InstallTool installs the given tool.
// installed binary's filename becomes `name-version`.
func (a *PluginAPI) InstallTool(ctx context.Context, req *service.InstallToolRequest) (*service.InstallToolResponse, error) {
p, err := a.toolRegistry.InstallTool(ctx, req.GetName(), req.GetVersion(), req.GetInstallScript())
if err != nil {
a.Logger.Error("failed to install tool",
zap.String("name", req.GetName()),
zap.String("version", req.GetVersion()),
zap.Error(err))
return nil, err
}
return &service.InstallToolResponse{
InstalledPath: p,
}, nil

Check warning on line 104 in pkg/app/pipedv1/cmd/piped/grpcapi/plugin_api.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipedv1/cmd/piped/grpcapi/plugin_api.go#L93-L104

Added lines #L93 - L104 were not covered by tests
}

func (a *PluginAPI) ReportStageLogs(ctx context.Context, req *service.ReportStageLogsRequest) (*service.ReportStageLogsResponse, error) {
_, err := a.apiClient.ReportStageLogs(ctx, &pipedservice.ReportStageLogsRequest{
DeploymentId: req.DeploymentId,
Expand Down
133 changes: 133 additions & 0 deletions pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
// Copyright 2024 The PipeCD Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package grpcapi

import (
"bytes"
"context"
"fmt"
"os"
"os/exec"
"path/filepath"
"runtime"
"text/template"

"golang.org/x/sync/singleflight"
)

type toolRegistry struct {
toolsDir string
tmpDir string
group singleflight.Group
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the tool is not plugin-specific, I think we should have some kind of cache to avoid redundant installation. If the name and version are the same, then we should return the installed path immediately instead of running the script, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I'll add the cache like below.

r.mu.RLock()
_, ok := r.versions[name]
r.mu.RUnlock()
if ok {
return path, false, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khanhtc1202
fixed on this commit, thanks
75d7daa


type templateValues struct {
Name string
Version string
OutPath string
TmpDir string
Arch string
Os string
}

func newToolRegistry(toolsDir, tmpDir string) (*toolRegistry, error) {
tmpDir, err := os.MkdirTemp(tmpDir, "tool-registry")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Ask] Is it enough just to create the tmp dir for the tool installation in the newToolRegistry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to call like os.MkdirTemp("", "tool-registry") and do not pass the tmpDir as the argument?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I mean fixing as newToolRegistry(toolsDir string) and call os.MkdirTemp inside it as you said.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got your point! That's enough.
I'll change the codes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed on this commit
f9f7bf0

if err != nil {
return nil, fmt.Errorf("failed to create a temporary directory: %w", err)
}
return &toolRegistry{
toolsDir: toolsDir,
tmpDir: tmpDir,
}, nil

Check warning on line 53 in pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go#L45-L53

Added lines #L45 - L53 were not covered by tests
}

func (r *toolRegistry) newTmpDir() (string, error) {
return os.MkdirTemp(r.tmpDir, "")
}

func (r *toolRegistry) outPath() (string, error) {
target, err := r.newTmpDir()
if err != nil {
return "", err
}

Check warning on line 64 in pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go#L63-L64

Added lines #L63 - L64 were not covered by tests
return filepath.Join(target, "out"), nil
}

func (r *toolRegistry) InstallTool(ctx context.Context, name, version, script string) (string, error) {
out, err, _ := r.group.Do(fmt.Sprintf("%s-%s", name, version), func() (interface{}, error) {
return r.installTool(ctx, name, version, script)
})
if err != nil {
return "", fmt.Errorf("failed to install the tool %s-%s: %w", name, version, err)
}
return out.(string), nil // the result is always string
}

func (r *toolRegistry) installTool(ctx context.Context, name, version, script string) (path string, err error) {
outPath, err := r.outPath()
if err != nil {
return "", err
}

Check warning on line 82 in pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go#L81-L82

Added lines #L81 - L82 were not covered by tests

tmpDir, err := r.newTmpDir()
if err != nil {
return "", err
}

Check warning on line 87 in pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go#L86-L87

Added lines #L86 - L87 were not covered by tests

t, err := template.New("install script").Parse(script)
if err != nil {
return "", err
}

Check warning on line 92 in pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go#L91-L92

Added lines #L91 - L92 were not covered by tests

vars := templateValues{
Name: name,
Version: version,
OutPath: outPath,
TmpDir: tmpDir,
Arch: runtime.GOARCH,
Os: runtime.GOOS,
}
var buf bytes.Buffer
if err := t.Execute(&buf, vars); err != nil {
return "", err
}

Check warning on line 105 in pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go#L104-L105

Added lines #L104 - L105 were not covered by tests

cmd := exec.CommandContext(ctx, "/bin/sh", "-c", buf.String())
if out, err := cmd.CombinedOutput(); err != nil {
return "", fmt.Errorf("failed to execute the install script: %w, output: %s", err, string(out))
}

if err := os.Chmod(outPath, 0o755); err != nil {
return "", err
}

target := filepath.Join(r.toolsDir, fmt.Sprintf("%s-%s", name, version))
if out, err := exec.CommandContext(ctx, "/bin/sh", "-c", "mv "+outPath+" "+target).CombinedOutput(); err != nil {
return "", fmt.Errorf("failed to move the installed binary: %w, output: %s", err, string(out))
}

Check warning on line 119 in pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go#L118-L119

Added lines #L118 - L119 were not covered by tests

if err := os.RemoveAll(tmpDir); err != nil {
return "", err
}

Check warning on line 123 in pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go#L122-L123

Added lines #L122 - L123 were not covered by tests

return target, nil
}

func (r *toolRegistry) Close() error {
if err := os.RemoveAll(r.tmpDir); err != nil {
return err
}
return nil

Check warning on line 132 in pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry.go#L128-L132

Added lines #L128 - L132 were not covered by tests
}
86 changes: 86 additions & 0 deletions pkg/app/pipedv1/cmd/piped/grpcapi/tool_registry_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
// Copyright 2024 The PipeCD Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package grpcapi

import (
"context"
"os"
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestToolRegistry_InstallTool(t *testing.T) {
t.Parallel()

ctx := context.Background()

toolsDir, err := os.MkdirTemp(t.TempDir(), "tools")
require.NoError(t, err)
tmpDir, err := os.MkdirTemp(t.TempDir(), "tmp")
require.NoError(t, err)

registry := &toolRegistry{
toolsDir: toolsDir,
tmpDir: tmpDir,
}

tests := []struct {
name string
toolName string
toolVersion string
script string
wantErr bool
}{
{
name: "valid script",
toolName: "tool-a",
toolVersion: "1.0.0",
script: `touch {{ .OutPath }}`,
wantErr: false,
},
{
name: "output is not found",
toolName: "tool-b",
toolVersion: "1.0.0",
script: "exit 0",
wantErr: true,
},
{
name: "script failed",
toolName: "tool-c",
toolVersion: "1.0.0",
script: "exit 1",
wantErr: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

out, err := registry.InstallTool(ctx, tt.toolName, tt.toolVersion, tt.script)
if tt.wantErr {
require.Error(t, err)
return
}
require.NoError(t, err)
assert.FileExists(t, out)
assert.True(t, strings.HasSuffix(out, tt.toolName+"-"+tt.toolVersion), "output path should have the tool {name}-{version}, got %s", out)
})
}
}
8 changes: 6 additions & 2 deletions pkg/app/pipedv1/cmd/piped/piped.go
Original file line number Diff line number Diff line change
Expand Up @@ -304,15 +304,19 @@
// Start running plugin service server.
{
var (
service = grpcapi.NewPluginAPI(cfg, apiClient, input.Logger)
opts = []rpc.Option{
service, err = grpcapi.NewPluginAPI(cfg, apiClient, p.toolsDir, input.Logger)
opts = []rpc.Option{

Check warning on line 308 in pkg/app/pipedv1/cmd/piped/piped.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipedv1/cmd/piped/piped.go#L307-L308

Added lines #L307 - L308 were not covered by tests
rpc.WithPort(p.pluginServicePort),
rpc.WithGracePeriod(p.gracePeriod),
rpc.WithLogger(input.Logger),
rpc.WithLogUnaryInterceptor(input.Logger),
rpc.WithRequestValidationUnaryInterceptor(),
}
)
if err != nil {
input.Logger.Error("failed to create plugin service", zap.Error(err))
return err
}

Check warning on line 319 in pkg/app/pipedv1/cmd/piped/piped.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/pipedv1/cmd/piped/piped.go#L316-L319

Added lines #L316 - L319 were not covered by tests
// TODO: Ensure piped <-> plugin communication is secure.
server := rpc.NewServer(service, opts...)
group.Go(func() error {
Expand Down
Loading