Skip to content

Commit

Permalink
fix: Address #9006 (#9008)
Browse files Browse the repository at this point in the history 
Add a filter function to only send port resource resources for that particular container into AllocatePort
  • Loading branch information
@jcscottiii
jcscottiii authored Aug 14, 2023
1 parent 85362a1 commit 59a928e
Show file tree
Hide file tree
Showing 2 changed files with 132 additions and 5 deletions.
16 changes: 15 additions & 1 deletion pkg/skaffold/deploy/docker/deploy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"
"io"
"regexp"
"strings"
"sync"
"time"

Expand Down Expand Up @@ -199,7 +200,10 @@ func (d *Deployer) deploy(ctx context.Context, out io.Writer, artifact graph.Art
opts.Mounts = mounts
}

bindings, err := d.portManager.AllocatePorts(artifact.ImageName, d.resources, containerCfg, debugBindings)
// Filter for the port resource for the given artifact.ImageName
filteredPFResources := d.filterPortForwardingResources(artifact.ImageName)

bindings, err := d.portManager.AllocatePorts(artifact.ImageName, filteredPFResources, containerCfg, debugBindings)
if err != nil {
return err
}
Expand All @@ -213,6 +217,16 @@ func (d *Deployer) deploy(ctx context.Context, out io.Writer, artifact graph.Art
return nil
}

func (d *Deployer) filterPortForwardingResources(imageName string) []*latest.PortForwardResource {
filteredPFResources := []*latest.PortForwardResource{}
for _, p := range d.resources {
if strings.EqualFold(imageName, p.Name) {
filteredPFResources = append(filteredPFResources, p)
}
}
return filteredPFResources
}

// setupDebugging configures the provided artifact's image for debugging (if applicable).
// The provided container configuration receives any relevant modifications (e.g. ENTRYPOINT, CMD),
// and any init containers for populating the shared debug volume will be created.
Expand Down
121 changes: 117 additions & 4 deletions pkg/skaffold/deploy/docker/deploy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ import (
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/deploy/label"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/docker/debugger"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/graph"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/latest"
"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/schema/util"
"github.com/GoogleContainerTools/skaffold/v2/testutil"
)

Expand All @@ -45,8 +47,9 @@ func TestDebugBindings(t *testing.T) {
}

tests := []struct {
name string
artifacts []debugArtifact
name string
artifacts []debugArtifact
portForwardResources []*latest.PortForwardResource
}{
{
name: "one artifact one binding",
Expand All @@ -59,6 +62,7 @@ func TestDebugBindings(t *testing.T) {
},
},
},
portForwardResources: nil,
},
{
name: "two artifacts two bindings",
Expand All @@ -78,6 +82,7 @@ func TestDebugBindings(t *testing.T) {
},
},
},
portForwardResources: nil,
},
{
name: "two artifacts but one not configured for debugging",
Expand All @@ -94,6 +99,7 @@ func TestDebugBindings(t *testing.T) {
debug: false,
},
},
portForwardResources: nil,
},
{
name: "two artifacts with same runtime - port collision",
Expand All @@ -113,6 +119,44 @@ func TestDebugBindings(t *testing.T) {
},
},
},
portForwardResources: nil,
},
{
name: "two artifacts two bindings and two port forward resources",
artifacts: []debugArtifact{
{
image: "go",
debug: true,
expectedBindings: nat.PortMap{
"56268/tcp": {{HostIP: "127.0.0.1", HostPort: "56268"}},
"9000/tcp": nil, // Allow any mapping
},
},
{
image: "nodejs",
debug: true,
expectedBindings: nat.PortMap{
"9229/tcp": {{HostIP: "127.0.0.1", HostPort: "9229"}},
"9090/tcp": nil, // Allow any mapping
},
},
},
portForwardResources: []*latest.PortForwardResource{
{
Name: "go",
Type: "container",
Port: util.IntOrString{
IntVal: 9000,
},
},
{
Name: "nodejs",
Type: "container",
Port: util.IntOrString{
IntVal: 9090,
},
},
},
},
}

Expand All @@ -131,7 +175,7 @@ func TestDebugBindings(t *testing.T) {
return configs, nil, nil
})

d, _ := NewDeployer(context.TODO(), mockConfig{}, &label.DefaultLabeller{}, nil, nil, "default")
d, _ := NewDeployer(context.TODO(), mockConfig{}, &label.DefaultLabeller{}, nil, test.portForwardResources, "default")

for _, a := range test.artifacts {
config := container.Config{
Expand All @@ -147,18 +191,87 @@ func TestDebugBindings(t *testing.T) {
}
testutil.CheckErrorAndFailNow(t, false, err)

bindings, err := d.portManager.AllocatePorts(a.image, d.resources, &config, debugBindings)
filteredPFResources := d.filterPortForwardingResources(a.image)

bindings, err := d.portManager.AllocatePorts(a.image, filteredPFResources, &config, debugBindings)
testutil.CheckErrorAndFailNow(t, false, err)

// CheckDeepEqual unfortunately doesn't work when the map elements are slices
for k, v := range a.expectedBindings {
// TODO: If given nil, assume that any value works.
// Otherwise, perform equality check.
if v == nil {
continue
}
testutil.CheckDeepEqual(t, v, bindings[k])
}
if len(a.expectedBindings) != len(bindings) {
t.Errorf("mismatch number of bindings. Expected number of bindings: %d. Actual number of bindings: %d\n", len(a.expectedBindings), len(bindings))
}
}
})
}
}

func TestFilterPortForwardingResources(t *testing.T) {
resources := []*latest.PortForwardResource{
{
Name: "image1",
Port: util.FromInt(8000),
},
{
Name: "image2",
Port: util.FromInt(8001),
},
{
Name: "image2",
Port: util.FromInt(8002),
},
}
tests := []struct {
name string
imageName string
expectedPortResources []*latest.PortForwardResource
}{
{
name: "image name not in list",
imageName: "image3",
expectedPortResources: []*latest.PortForwardResource{},
},
{
name: "image in list. return one",
imageName: "image1",
expectedPortResources: []*latest.PortForwardResource{
{
Name: "image1",
Port: util.FromInt(8000),
},
},
},
{
name: "image in list. return multiple",
imageName: "image2",
expectedPortResources: []*latest.PortForwardResource{
{
Name: "image2",
Port: util.FromInt(8001),
},
{
Name: "image2",
Port: util.FromInt(8002),
},
},
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
d := Deployer{resources: resources}
pfResources := d.filterPortForwardingResources(test.imageName)
testutil.CheckDeepEqual(t, test.expectedPortResources, pfResources)
})
}
}

type mockConfig struct{}

func (m mockConfig) ContainerDebugging() bool { return true }
Expand Down

0 comments on commit 59a928e

Please sign in to comment.