Skip to content

Commit

Permalink
allow digested images to be 'run' (knative#2445)
Browse files Browse the repository at this point in the history 
* init fix

Signed-off-by: gauron99 <[email protected]>

* dont override direct deploy tag, more tests

Signed-off-by: gauron99 <[email protected]>

* fix

Signed-off-by: gauron99 <[email protected]>

* dont validate with tagged image, fix comment

Signed-off-by: gauron99 <[email protected]>

* init run fix for --image

Signed-off-by: gauron99 <[email protected]>

* init

Signed-off-by: gauron99 <[email protected]>

* int test, add valid untdigested images to run

Signed-off-by: gauron99 <[email protected]>

* check images passed to runner for func run command

Signed-off-by: gauron99 <[email protected]>

* fix build/deploy image passing bug add test

Signed-off-by: gauron99 <[email protected]>

* fix

Signed-off-by: gauron99 <[email protected]>

* remove extra printing

Signed-off-by: gauron99 <[email protected]>

* merge functions to digested

Signed-off-by: gauron99 <[email protected]>

* misspell

Signed-off-by: gauron99 <[email protected]>

* simplify

Signed-off-by: David Fridrich <[email protected]>

* quick fix

Signed-off-by: David Fridrich <[email protected]>

* remove prints, comment

Signed-off-by: David Fridrich <[email protected]>

---------

Signed-off-by: gauron99 <[email protected]>
Signed-off-by: David Fridrich <[email protected]>
  • Loading branch information
@gauron99
gauron99 authored Aug 28, 2024
1 parent bbb3c47 commit ca61712
Show file tree
Hide file tree
Showing 9 changed files with 525 additions and 95 deletions.
2 changes: 1 addition & 1 deletion cmd/build.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ func runBuild(cmd *cobra.Command, _ []string, newClient ClientFactory) (err erro
return
}
if cfg.Push {
if f, err = client.Push(cmd.Context(), f); err != nil {
if f, _, err = client.Push(cmd.Context(), f); err != nil {
return
}
}
Expand Down
129 changes: 51 additions & 78 deletions cmd/deploy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -298,16 +298,24 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) {
return
}

// Preprocess image name. Validate the image and check whether its digested
// This might alter f.Deploy.Image.
var digested bool
f, digested, err = processImageName(f, cfg.Image)
if err != nil {
return
var (
digested bool
justBuilt bool
justPushed bool
)

// Validate the image and check whether its digested or not
if cfg.Image != "" {
digested, err = isDigested(cfg.Image)
if err != nil {
return
}
// image is valid and undigested
if !digested {
f.Deploy.Image = cfg.Image
}
}

var justBuilt bool

// If user provided --image with digest, they are requesting that specific
// image to be used which means building phase should be skipped and image
// should be deployed as is
Expand All @@ -319,19 +327,18 @@ func runDeploy(cmd *cobra.Command, newClient ClientFactory) (err error) {
return
}
if cfg.Push {
if f, err = client.Push(cmd.Context(), f); err != nil {
if f, justPushed, err = client.Push(cmd.Context(), f); err != nil {
return
}
}
// TODO: gauron99 - temporary fix for undigested image direct deploy (w/out
// build) I think we will be able to remove this after we clean up the
// building process - move the setting of built image in building phase?
if justBuilt && f.Build.Image != "" {
// TODO: gauron99 - temporary fix for undigested image direct deploy
// (w/out build) This might be more complex to do than leaving like this
// image digests are created via the registry on push.
if (justBuilt || justPushed) && f.Build.Image != "" {
// f.Build.Image is set in Push for now, just set it as a deployed image
f.Deploy.Image = f.Build.Image
}
}

if f, err = client.Deploy(cmd.Context(), f, fn.WithDeploySkipBuildCheck(cfg.Build == "false")); err != nil {
return
}
Expand Down Expand Up @@ -372,7 +379,8 @@ func build(cmd *cobra.Command, flag string, f fn.Function, client *fn.Client, bu
}
} else if _, err = strconv.ParseBool(flag); err != nil {
return f, false, fmt.Errorf("--build ($FUNC_BUILD) %q not recognized. Should be 'auto' or a truthy value such as 'true', 'false', '0', or '1'.", flag)

} else if !build {
return f, false, nil
}
return f, true, nil
}
Expand Down Expand Up @@ -671,10 +679,11 @@ func (c deployConfig) Validate(cmd *cobra.Command) (err error) {
}

// Check Image Digest was included
// (will be set on the function during .Configure)
var digest bool
if digest, err = isDigested(c.Image); err != nil {
return
if c.Image != "" {
if digest, err = isDigested(c.Image); err != nil {
return
}
}

// --build can be "auto"|true|false
Expand Down Expand Up @@ -775,44 +784,40 @@ func printDeployMessages(out io.Writer, f fn.Function) {
}
}

// isUndigested returns true if provided image string 'v' has valid tag and false if
// not. It is lenient in validating - does not always throw an error, just
// returning false in some scenarios.
func isUndigested(v string) (validTag bool, err error) {
if strings.Contains(v, "@") {
// digest has been processed separately
return
}
vv := strings.Split(v, ":")
if len(vv) < 2 {
// assume user knows what hes doing
validTag = true
return
} else if len(vv) > 2 {
err = fmt.Errorf("image '%v' contains an invalid tag (extra ':')", v)
return
}
tag := vv[1]
if tag == "" {
err = fmt.Errorf("image '%v' has an empty tag", v)
return
}

validTag = true
return
}

// isDigested returns true if provided image string 'v' has digest and false if not.
// Includes basic validation that a provided digest is correctly formatted.
// Given that image is not digested, image will still be validated and return
// a combination of bool (img has valid digest) and err (img is in valid format)
// Therefore returned combination of [false,nil] means "valid undigested image".
func isDigested(v string) (validDigest bool, err error) {
var digest string
vv := strings.Split(v, "@")
if len(vv) < 2 {
return // has no digest
// image does NOT have a digest, validate further
if v == "" {
err = fmt.Errorf("provided image is empty, cannot validate")
return
}
vvv := strings.Split(v, ":")
if len(vvv) < 2 {
// assume user knows what hes doing
return
} else if len(vvv) > 2 {
err = fmt.Errorf("image '%v' contains an invalid tag (extra ':')", v)
return
}
tag := vvv[1]
if tag == "" {
err = fmt.Errorf("image '%v' has an empty tag", v)
return
}
return
} else if len(vv) > 2 {
// image is invalid
err = fmt.Errorf("image '%v' contains an invalid digest (extra '@')", v)
return
}
// image has a digest, validate further
digest = vv[1]

if !strings.HasPrefix(digest, "sha256:") {
Expand All @@ -827,35 +832,3 @@ func isDigested(v string) (validDigest bool, err error) {
validDigest = true
return
}

// processImageName processes the image name for deployment. It ensures that
// image string is validated if --image was given and ensures that proper
// fields of Function structure are populated if needed.
// Returns a Function structure(1), bool indicating if image was given with
// digest(2) and error(3)
func processImageName(fin fn.Function, configImage string) (f fn.Function, digested bool, err error) {
f = fin
// check if --image was provided with a digest. 'digested' bool indicates if
// image contains a digest or not (image is "digested").
digested, err = isDigested(configImage)
if err != nil {
return
}
// if image is digested, no need to process further
if digested {
return
}
// digested = false here

// valid image can be with/without a tag and might be/not be built next
valid, err := isUndigested(configImage)
if err != nil {
return
}
if valid {
// this can be overridden when build&push=enabled with freshly built
// (digested) image OR directly deployed when build&push=disabled
f.Deploy.Image = configImage
}
return
}
123 changes: 123 additions & 0 deletions cmd/deploy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2017,3 +2017,126 @@ func TestDeploy_WithoutHome(t *testing.T) {
t.Fatal(err)
}
}

// TestDeploy_CorrectImageDeployed ensures that deploying will always pass
// the correct image name to the deployer (populating the f.Deploy.Image value)
// in various scenarios.
func TestDeploy_CorrectImageDeployed(t *testing.T) {
const sha = "sha256:xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
// dataset
tests := []struct {
name string
image string
buildArgs []string
deployArgs []string
shouldFail bool
shouldBuild bool
pusherActive bool
}{
{
name: "basic test to create and deploy",
image: "myimage",
deployArgs: []string{"--image", "myimage"},
},
{
name: "test to deploy with prebuild",
image: "myimage",
buildArgs: []string{
"--image=myimage",
},
deployArgs: []string{
"--build=false",
},
shouldBuild: true,
},
{
name: "test to build and deploy",
image: "myimage",
buildArgs: []string{
"--image=myimage",
},
shouldBuild: true,
},
{
name: "test to deploy without build should fail",
image: "myimage",
deployArgs: []string{
"--build=false",
},
shouldFail: true,
},
{
name: "test to build then deploy with push",
image: "myimage" + "@" + sha,
buildArgs: []string{
"--image=myimage",
},
deployArgs: []string{
"--build=false",
"--push=true",
},
shouldBuild: true,
pusherActive: true,
},
}

// run tests
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
root := FromTempDirectory(t)
f := fn.Function{
Runtime: "go",
Root: root,
}
_, err := fn.New().Init(f)
if err != nil {
t.Fatal(err)
}

// prebuild function if desired
if tt.shouldBuild {
cmd := NewBuildCmd(NewTestClient(fn.WithRegistry(TestRegistry)))
cmd.SetArgs(tt.buildArgs)
if err = cmd.Execute(); err != nil {
t.Fatal(err)
}
}

pusher := mock.NewPusher()
if tt.pusherActive {
pusher.PushFn = func(_ context.Context, _ fn.Function) (string, error) {
return sha, nil
}
}

deployer := mock.NewDeployer()
deployer.DeployFn = func(_ context.Context, f fn.Function) (result fn.DeploymentResult, err error) {
// verify the image passed to the deployer
if f.Deploy.Image != tt.image {
return fn.DeploymentResult{}, fmt.Errorf("image '%v' does not match the expected image '%v'\n", f.Deploy.Image, tt.image)
}
return
}

// Deploy the function
cmd := NewDeployCmd(NewTestClient(
fn.WithDeployer(deployer), //is always specified
fn.WithPusher(pusher))) // if specified, will return sha for testing

cmd.SetArgs(tt.deployArgs)

// assert
err = cmd.Execute()
if tt.shouldFail {
if err == nil {
t.Fatal("expected an error but got none")
}
} else {
// should not fail
if err != nil {
t.Fatal(err)
}
}
})
}
}
37 changes: 35 additions & 2 deletions cmd/run.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,12 +177,45 @@ func runRun(cmd *cobra.Command, newClient ClientFactory) (err error) {
// If requesting to run via the container, build the container if it is
// either out-of-date or a build was explicitly requested.
if cfg.Container {
var digested bool

buildOptions, err := cfg.buildOptions()
if err != nil {
return err
}
if f, _, err = build(cmd, cfg.Build, f, client, buildOptions); err != nil {
return err

// if image was specified, check if its digested and do basic validation
if cfg.Image != "" {
digested, err = isDigested(cfg.Image)
if err != nil {
return err
}
if !digested {
// assign valid undigested image
f.Build.Image = cfg.Image
}
}

if digested {
// run cmd takes f.Build.Image - see newContainerConfig in docker/runner.go
// it doesnt get saved, just runtime image
f.Build.Image = cfg.Image
} else {

if f, _, err = build(cmd, cfg.Build, f, client, buildOptions); err != nil {
return err
}
}
} else {
// dont run digested image without a container
if cfg.Image != "" {
digested, err := isDigested(cfg.Image)
if err != nil {
return err
}
if digested {
return fmt.Errorf("cannot use digested image with --container=false")
}
}
}

Expand Down
Loading

0 comments on commit ca61712

Please sign in to comment.