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.

Sign up for GitHub

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

fix: #9006 - Filter port forwarding resources for docker deploy #9008

Merged
merged 1 commit into from
Aug 14, 2023
Merged

fix: #9006 - Filter port forwarding resources for docker deploy #9008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 15 additions & 1 deletion pkg/skaffold/deploy/docker/deploy.go
View file
Open in desktop
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
Open in desktop
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",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially, I added a test case to the TestDebugBindings function since it was the only function here. However, I did end up adding a separate function TestFilterPortForwardingResources to test specifically that. Let me know if I should revert the changes in this TestDebugBindings function.

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