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 QuickSyncPlan for k8s plugin #5020

Merged
merged 8 commits into from
Jul 12, 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
54 changes: 52 additions & 2 deletions pkg/app/pipedv1/plugin/platform/kubernetes/planner/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,27 @@ package planner
import (
"context"
"fmt"
"io"
"os"
"time"

"github.com/pipe-cd/pipecd/pkg/app/pipedv1/deploysource"
"github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/sourcecloner"
"github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1/platform"
"github.com/pipe-cd/pipecd/pkg/regexpool"

"go.uber.org/zap"
"google.golang.org/grpc"
)

type secretDecrypter interface {
Decrypt(string) (string, error)
}

type PlannerService struct {
platform.UnimplementedPlannerServiceServer

Decrypter secretDecrypter
RegexPool *regexpool.Pool
Logger *zap.Logger
}
Expand All @@ -38,8 +48,12 @@ func (a *PlannerService) Register(server *grpc.Server) {
}

// NewPlannerService creates a new planService.
func NewPlannerService(logger *zap.Logger) *PlannerService {
func NewPlannerService(
decrypter secretDecrypter,
logger *zap.Logger,
) *PlannerService {
return &PlannerService{
Decrypter: decrypter,
RegexPool: regexpool.DefaultPool(),
Logger: logger.Named("planner"),
}
Expand All @@ -63,7 +77,43 @@ func (ps *PlannerService) DetermineStrategy(ctx context.Context, in *platform.De
}

func (ps *PlannerService) QuickSyncPlan(ctx context.Context, in *platform.QuickSyncPlanRequest) (*platform.QuickSyncPlanResponse, error) {
return nil, fmt.Errorf("not implemented yet")
now := time.Now()

cloner, err := sourcecloner.NewCloner(
in.GetInput().GetSourceRemoteUrl(),
in.GetInput().GetDeployment().GetGitPath().GetRepo().GetBranch(),
"target",
)
if err != nil {
return nil, fmt.Errorf("failed to create git client (%w)", err)
}

d, err := os.MkdirTemp("", "") // TODO
if err != nil {
return nil, fmt.Errorf("failed to prepare temporary directory (%w)", err)
}
defer os.RemoveAll(d)
Comment on lines +87 to +91
Copy link
Member

Choose a reason for hiding this comment

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

@Warashi Question:
Should we add workingDir as PlanPluginInput? 🤔 Currently, the planner in the controller uses workingDir passed at the time controller.planner object is created, which mean we have 1:1 for relationship between workingDir & planner which is used to build plan for a deployment
ref:

// Ensure the existence of the working directory for the deployment.
workingDir, err := os.MkdirTemp(c.workspaceDir, d.Id+"-planner-*")
if err != nil {
logger.Error("failed to create working directory for planner", zap.Error(err))
return nil, err
}

message PlanPluginInput {

Copy link
Member

Choose a reason for hiding this comment

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

If we make it that way, then the piped.planner object will create the workingDir then clone/pull the deploy source, then we pass the workingDir (which contains cloned source) to the planner plugin via PlanPluginInput.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds nice.
In this PR, we clone the deploy source manually.
However, it's needed for each plugin implementation, so it's simpler if we handle it at piped and only use the prepared source in the plugin.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let address it by another PR 👍


p := deploysource.NewProvider(
d,
cloner,
*in.GetInput().GetDeployment().GetGitPath(),
ps.Decrypter,
)

ds, err := p.GetReadOnly(ctx, io.Discard /* TODO */)
if err != nil {
return nil, err
}

Copy link
Member

@khanhtc1202 khanhtc1202 Jul 11, 2024

Choose a reason for hiding this comment

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

nits

	cfg := ds.ApplicationConfig.KubernetesApplicationSpec
	if cfg == nil {
		return nil, fmt.Errorf("missing KubernetesApplicationSpec in application configuration")
	}

Should check spec available to avoid nil pointer 👀

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 fixed it on this commit.
d845f7a

autoRollback := false
if a := ds.ApplicationConfig.KubernetesApplicationSpec.Input.AutoRollback; a != nil {
autoRollback = *a
}
Copy link
Member

Choose a reason for hiding this comment

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

nits, we have a default value set as true, so no need to initialize autoRollback var as false here, since the ds.ApplicationConfig.KubernetesApplicationSpec.Input.AutoRollback is true as init
ref: ds.ApplicationConfig.KubernetesApplicationSpec.Input.AutoRollback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you.
I noticed that I can view the document here.
https://pkg.go.dev/github.com/pipe-cd/pipecd/pkg/config#KubernetesDeploymentInput

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 fixed it on this commit.
d845f7a


return &platform.QuickSyncPlanResponse{
Stages: buildQuickSyncPipeline(autoRollback, now),
}, nil
}

func (ps *PlannerService) PipelineSyncPlan(ctx context.Context, in *platform.PipelineSyncPlanRequest) (*platform.PipelineSyncPlanResponse, error) {
Expand Down
34 changes: 34 additions & 0 deletions pkg/app/pipedv1/plugin/secrets/decrypter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// 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 secrets

import (
"context"
"fmt"

"github.com/pipe-cd/pipecd/pkg/app/pipedv1/cmd/piped/service"
)

type Decrypter struct {
client service.PluginServiceClient
}

func (d *Decrypter) Decrypt(src string) (string, error) {
r, err := d.client.DecryptSecret(context.TODO(), &service.DecryptSecretRequest{Secret: src})
if err != nil {
return "", fmt.Errorf("failed to decrypt secret: %w", err)
}
return r.GetDecryptedSecret(), nil
}
82 changes: 82 additions & 0 deletions pkg/app/pipedv1/plugin/sourcecloner/cloner.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
// 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 sourcecloner

import (
"context"
"os"
"os/exec"
"sync"

"github.com/pipe-cd/pipecd/pkg/app/piped/deploysource"
"github.com/pipe-cd/pipecd/pkg/git"
)

var _ deploysource.SourceCloner = (*Cloner)(nil)

var gitPath = sync.OnceValues(func() (string, error) {
path, err := exec.LookPath("git")
if err != nil {
return "", err
}
return path, nil
})

// Cloner is a source cloner for git repositories.
type Cloner struct {
gitPath string
remotePath string
revision string
revisionName string
}
Copy link
Member

Choose a reason for hiding this comment

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

@Warashi I have an idea with this sourcecloner package 🤔
Basically, our plan is to use the piped/deploysource as the portal for all git resource operations (indeed, the name is deploysource and it emits the internal using git stub).
I think this newly created package sourcecloner should follow that approach and more. What package dose is to fetch the already cloned source (prepared by deploysource), then I think we should emits the internal using git as same as we do in deploysource.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of a Clone interface, which could lead to confusion (if the reviewer doesn't know we don't clone or any, just fetch the cloned local source prepared by deploysource), how about to reuse the SourceCloner interface. aka this: https://github.com/pipe-cd/pipecd/blob/master/pkg/app/piped/deploysource/sourcecloner.go#L24-L28
I can see we have some implementation of this interface looks pretty fits our use case here:
https://github.com/pipe-cd/pipecd/blob/master/pkg/app/piped/deploysource/sourcecloner.go#L69-L94

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I missed the localSourceCloner. Thank you!
I think we can use this, but I wonder why this Clone implementation only invokes the Copy method of the internal git repo.
If the internal git repo's current HEAD doesn't match localSourceCloner's revision, this Clone method will give the wrong results.
Or have I misunderstood the Revision() of SourceCloner?

Copy link
Member

Choose a reason for hiding this comment

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

@Warashi That's a good catch 👍 I think up to now, the localSourceCloner not yet raised any issues because its caller doesn't require any other revision than the HEAD 🤔 So, basically I think it's a bug potential logic and should be fixed, but as far as I investigated, the only place which we use this localSourceCloner is planpreview/builder (ref: this code: https://github.com/pipe-cd/pipecd/blob/master/pkg/app/piped/planpreview/builder.go#L228), which basically only request for the HEAD of the current branch - due to the feature plan preview logic.
So, my point is to fix this localSourceCloner and use it for whatever "NOT git action pull directly but pull/fetch from a local source". WDYT? 🤔

Copy link
Member

@khanhtc1202 khanhtc1202 Jul 8, 2024

Choose a reason for hiding this comment

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

My point is: we should not create too many interfaces/packages that do similar tasks (in this case: clone source from git), or it will lead to confusion for the caller side. In this case, it would be better to have only one SourceCloner interface, one pull/fetch to the outer git, one pull/fetch to the local repo that was pulled by the previous implementation.

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 with you.
I'll fix the localSourceCloner and use it.


// NewCloner creates a new Cloner instance.
// if revision is empty, it will checkout defualt branch.
func NewCloner(remotePath, revision, revisionName string) (*Cloner, error) {
gitPath, err := gitPath()
if err != nil {
return nil, err
}

return &Cloner{
gitPath: gitPath,
remotePath: remotePath,
revision: revision,
revisionName: revisionName,
}, nil
}

// Clone implements deploysource.SourceCloner.
func (c *Cloner) Clone(ctx context.Context, dest string) error {
if _, err := git.RunGitCommand(ctx, c.gitPath, "", os.Environ(), "clone", c.remotePath, dest); err != nil {
return err
}
if c.revision == "" {
if _, err := git.RunGitCommand(ctx, c.gitPath, "", os.Environ(), "-C", dest, "checkout", c.revision); err != nil {
return err
}
}
return 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.

I want to write tests for this method with git.faker.
However, the PR will become larger if I make git.faker public.
So, I decide not to write the test in this PR.
I'll make git.faker public later, and then write the test.


// Revision implements deploysource.SourceCloner.
func (c *Cloner) Revision() string {
return c.revision
}

// RevisionName implements deploysource.SourceCloner.
func (c *Cloner) RevisionName() string {
return c.revisionName
}
5 changes: 5 additions & 0 deletions pkg/git/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,11 @@ func (c *client) envsForRepo(remote string) []string {
return append(envs, c.gitEnvs...)
}

// RunGitCommand runs a git command with the given arguments.
Copy link
Member

Choose a reason for hiding this comment

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

we don't use this anymore 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I removed it on this commit
355ce05

func RunGitCommand(ctx context.Context, execPath, dir string, envs []string, args ...string) ([]byte, error) {
return runGitCommand(ctx, execPath, dir, envs, args...)
}

func runGitCommand(ctx context.Context, execPath, dir string, envs []string, args ...string) ([]byte, error) {
cmd := exec.CommandContext(ctx, execPath, args...)
cmd.Dir = dir
Expand Down
Loading