Skip to content

Commit c821a9e

Browse files
nkubalankubala
committed
Use ContainerAdapter to abstract away kubernetes packages from debug package
1 parent c126a0d commit c821a9e

20 files changed

+331
-199
lines changed

‎pkg/skaffold/debug/cnb.go

+4-2
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
"github.com/sirupsen/logrus"
2828

2929
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug/annotations"
30+
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug/types"
3031
)
3132

3233
const (
@@ -104,7 +105,7 @@ func hasCNBLauncherEntrypoint(ic imageConfiguration) bool {
104105
// the default process type. `CNB_PROCESS_TYPE` is ignored in this situation. A different process
105106
// can be used by overriding the image entrypoint. Direct and script launches are supported by
106107
// setting the entrypoint to `/cnb/lifecycle/launcher` and providing the appropriate arguments.
107-
func updateForCNBImage(container *operableContainer, ic imageConfiguration, transformer func(container *operableContainer, ic imageConfiguration) (annotations.ContainerDebugConfiguration, string, error)) (annotations.ContainerDebugConfiguration, string, error) {
108+
func updateForCNBImage(adapter types.ContainerAdapter, ic imageConfiguration, transformer func(adapter types.ContainerAdapter, ic imageConfiguration) (annotations.ContainerDebugConfiguration, string, error)) (annotations.ContainerDebugConfiguration, string, error) {
108109
// buildpacks/lifecycle 0.6.0 embeds the process definitions into a special image label.
109110
// The build metadata isn't absolutely required as the image args could be
110111
// a command line (e.g., `python xxx`) but it likely indicates the
@@ -132,7 +133,7 @@ func updateForCNBImage(container *operableContainer, ic imageConfiguration, tran
132133
ic.workingDir = "/workspace"
133134
}
134135

135-
c, img, err := transformer(container, ic)
136+
c, img, err := transformer(adapter, ic)
136137
if err != nil {
137138
return c, img, err
138139
}
@@ -141,6 +142,7 @@ func updateForCNBImage(container *operableContainer, ic imageConfiguration, tran
141142
c.WorkingDir = ic.workingDir
142143
}
143144

145+
container := adapter.GetContainer()
144146
if container.Args != nil && rewriter != nil {
145147
// Only rewrite the container if the arguments were changed: some transforms only alter
146148
// env vars, and the image's arguments are not changed.

‎pkg/skaffold/debug/cnb_test.go

+14-10
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ import (
2626
v1 "k8s.io/api/core/v1"
2727

2828
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug/annotations"
29+
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug/types"
30+
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/debugging/adapter"
2931
"github.com/GoogleContainerTools/skaffold/testutil"
3032
)
3133

@@ -381,29 +383,31 @@ func TestUpdateForCNBImage(t *testing.T) {
381383
// Test that when a transform modifies the command-line arguments, then
382384
// the changes are reflected to the launcher command-line
383385
testutil.Run(t, test.description+" (args changed)", func(t *testutil.T) {
384-
argsChangedTransform := func(c *operableContainer, ic imageConfiguration) (annotations.ContainerDebugConfiguration, string, error) {
385-
c.Args = ic.arguments
386+
argsChangedTransform := func(a types.ContainerAdapter, ic imageConfiguration) (annotations.ContainerDebugConfiguration, string, error) {
387+
a.GetContainer().Args = ic.arguments
386388
return annotations.ContainerDebugConfiguration{}, "", nil
387389
}
388-
operable := operableContainer{}
389390
container := v1.Container{}
390-
c, _, err := updateForCNBImage(&operable, test.input, argsChangedTransform)
391-
applyFromOperable(&operable, &container)
391+
a := adapter.NewAdapter(&container)
392+
c, _, err := updateForCNBImage(a, test.input, argsChangedTransform)
393+
a.Apply()
392394
t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expected, container)
393395
t.CheckErrorAndDeepEqual(test.shouldErr, err, test.config, c)
394396
})
395397

396398
// Test that when the arguments are left unchanged, that the container is unchanged
397399
testutil.Run(t, test.description+" (args unchanged)", func(t *testutil.T) {
398-
argsUnchangedTransform := func(c *operableContainer, ic imageConfiguration) (annotations.ContainerDebugConfiguration, string, error) {
400+
argsUnchangedTransform := func(_ types.ContainerAdapter, ic imageConfiguration) (annotations.ContainerDebugConfiguration, string, error) {
399401
return annotations.ContainerDebugConfiguration{WorkingDir: ic.workingDir}, "", nil
400402
}
401403

402-
copy := operableContainer{}
403-
_, _, err := updateForCNBImage(&copy, test.input, argsUnchangedTransform)
404+
container := v1.Container{}
405+
a := adapter.NewAdapter(&container)
406+
// copy := operableContainer{}
407+
_, _, err := updateForCNBImage(a, test.input, argsUnchangedTransform)
404408
t.CheckError(test.shouldErr, err)
405-
if copy.Args != nil {
406-
t.Errorf("args not nil: %v", copy.Args)
409+
if container.Args != nil {
410+
t.Errorf("args not nil: %v", container.Args)
407411
}
408412
})
409413
}

‎pkg/skaffold/debug/container.go

+40-113
Original file line numberDiff line numberDiff line change
@@ -1,36 +1,33 @@
1+
/*
2+
Copyright 2021 The Skaffold Authors
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
117
package debug
218

319
import (
420
"fmt"
521

6-
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug/annotations"
722
shell "github.com/kballard/go-shellquote"
823
"github.com/sirupsen/logrus"
924
v1 "k8s.io/api/core/v1"
1025
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
11-
)
12-
13-
type operableContainer struct {
14-
Name string
15-
Command []string
16-
Args []string
17-
Env containerEnv
18-
Ports []containerPort
19-
}
2026

21-
// adapted from github.com/kubernetes/api/core/v1/types.go
22-
type containerPort struct {
23-
Name string
24-
HostPort int32
25-
ContainerPort int32
26-
Protocol string
27-
HostIP string
28-
}
29-
30-
type containerEnv struct {
31-
Order []string
32-
Env map[string]string
33-
}
27+
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug/annotations"
28+
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug/types"
29+
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/debugging/adapter"
30+
)
3431

3532
// containerTransforms are the set of configured transformers
3633
var containerTransforms []containerTransformer
@@ -44,15 +41,17 @@ type containerTransformer interface {
4441
// and required initContainer (an empty string if not required), or return a non-nil error if
4542
// the container could not be transformed. The initContainer image is intended to install any
4643
// required debug support tools.
47-
Apply(container *operableContainer, config imageConfiguration, portAlloc portAllocator, overrideProtocols []string) (annotations.ContainerDebugConfiguration, string, error)
44+
Apply(adapter types.ContainerAdapter, config imageConfiguration, portAlloc portAllocator, overrideProtocols []string) (annotations.ContainerDebugConfiguration, string, error)
4845
}
4946

5047
// transformContainer rewrites the container definition to enable debugging.
5148
// Returns a debugging configuration description with associated language runtime support
5249
// container image, or an error if the rewrite was unsuccessful.
53-
func transformContainer(container *operableContainer, config imageConfiguration, portAlloc portAllocator) (annotations.ContainerDebugConfiguration, string, error) {
50+
func transformContainer(adapter types.ContainerAdapter, config imageConfiguration, portAlloc portAllocator) (annotations.ContainerDebugConfiguration, string, error) {
5451
// Update the image configuration's environment with those set in the k8s manifest.
5552
// (Environment variables in the k8s container's `env` add to the image configuration's `env` settings rather than replace.)
53+
container := adapter.GetContainer()
54+
defer adapter.Apply()
5655
for _, key := range container.Env.Order {
5756
// FIXME handle ValueFrom?
5857
if config.env == nil {
@@ -69,13 +68,13 @@ func transformContainer(container *operableContainer, config imageConfiguration,
6968
}
7069

7170
// Apply command-line unwrapping for buildpack images and images using `sh -c`-style command-lines
72-
next := func(container *operableContainer, config imageConfiguration) (annotations.ContainerDebugConfiguration, string, error) {
73-
return performContainerTransform(container, config, portAlloc)
71+
next := func(adapter types.ContainerAdapter, config imageConfiguration) (annotations.ContainerDebugConfiguration, string, error) {
72+
return performContainerTransform(adapter, config, portAlloc)
7473
}
7574
if isCNBImage(config) {
76-
return updateForCNBImage(container, config, next)
75+
return updateForCNBImage(adapter, config, next)
7776
}
78-
return updateForShDashC(container, config, next)
77+
return updateForShDashC(adapter, config, next)
7978
}
8079

8180
func rewriteContainers(metadata *metav1.ObjectMeta, podSpec *v1.PodSpec, retrieveImageConfiguration configurationRetriever, debugHelpersRegistry string) bool {
@@ -101,16 +100,15 @@ func rewriteContainers(metadata *metav1.ObjectMeta, podSpec *v1.PodSpec, retriev
101100
if err != nil {
102101
continue
103102
}
104-
operable := operableContainerFromK8sContainer(&container)
103+
a := adapter.NewAdapter(&container)
105104
// requiredImage, if not empty, is the image ID providing the debugging support files
106105
// `err != nil` means that the container did not or could not be transformed
107-
if configuration, requiredImage, err := transformContainer(operable, imageConfig, portAlloc); err == nil {
106+
if configuration, requiredImage, err := transformContainer(a, imageConfig, portAlloc); err == nil {
108107
configuration.Artifact = imageConfig.artifact
109108
if configuration.WorkingDir == "" {
110109
configuration.WorkingDir = imageConfig.workingDir
111110
}
112111
configurations[container.Name] = configuration
113-
applyFromOperable(operable, &container)
114112
podSpec.Containers[i] = container // apply any configuration changes
115113
if len(requiredImage) > 0 {
116114
logrus.Infof("%q requires debugging support image %q", container.Name, requiredImage)
@@ -156,7 +154,7 @@ func rewriteContainers(metadata *metav1.ObjectMeta, podSpec *v1.PodSpec, retriev
156154
return false
157155
}
158156

159-
func updateForShDashC(container *operableContainer, ic imageConfiguration, transformer func(*operableContainer, imageConfiguration) (annotations.ContainerDebugConfiguration, string, error)) (annotations.ContainerDebugConfiguration, string, error) {
157+
func updateForShDashC(adapter types.ContainerAdapter, ic imageConfiguration, transformer func(types.ContainerAdapter, imageConfiguration) (annotations.ContainerDebugConfiguration, string, error)) (annotations.ContainerDebugConfiguration, string, error) {
160158
var rewriter func([]string)
161159
copy := ic
162160
switch {
@@ -166,6 +164,7 @@ func updateForShDashC(container *operableContainer, ic imageConfiguration, trans
166164
copy.entrypoint = split
167165
copy.arguments = nil
168166
rewriter = func(rewrite []string) {
167+
container := adapter.GetContainer()
169168
container.Command = nil // inherit from container
170169
container.Args = append([]string{shJoin(rewrite)}, ic.arguments[1:]...)
171170
}
@@ -177,6 +176,7 @@ func updateForShDashC(container *operableContainer, ic imageConfiguration, trans
177176
copy.entrypoint = split
178177
copy.arguments = nil
179178
rewriter = func(rewrite []string) {
179+
container := adapter.GetContainer()
180180
container.Command = append([]string{ic.entrypoint[0], ic.entrypoint[1], shJoin(rewrite)}, ic.entrypoint[3:]...)
181181
}
182182
}
@@ -187,13 +187,15 @@ func updateForShDashC(container *operableContainer, ic imageConfiguration, trans
187187
copy.entrypoint = split
188188
copy.arguments = nil
189189
rewriter = func(rewrite []string) {
190+
container := adapter.GetContainer()
190191
container.Command = nil
191192
container.Args = append([]string{ic.arguments[0], ic.arguments[1], shJoin(rewrite)}, ic.arguments[3:]...)
192193
}
193194
}
194195
}
195196

196-
c, image, err := transformer(container, copy)
197+
c, image, err := transformer(adapter, copy)
198+
container := adapter.GetContainer()
197199
if err == nil && rewriter != nil && container.Command != nil {
198200
rewriter(container.Command)
199201
}
@@ -204,87 +206,12 @@ func isShDashC(cmd, arg string) bool {
204206
return (cmd == "/bin/sh" || cmd == "/bin/bash") && arg == "-c"
205207
}
206208

207-
func performContainerTransform(container *operableContainer, config imageConfiguration, portAlloc portAllocator) (annotations.ContainerDebugConfiguration, string, error) {
208-
logrus.Tracef("Examining container %q with config %v", container.Name, config)
209+
func performContainerTransform(adapter types.ContainerAdapter, config imageConfiguration, portAlloc portAllocator) (annotations.ContainerDebugConfiguration, string, error) {
210+
logrus.Tracef("Examining container %q with config %v", adapter.GetContainer().Name, config)
209211
for _, transform := range containerTransforms {
210212
if transform.IsApplicable(config) {
211-
return transform.Apply(container, config, portAlloc, Protocols)
213+
return transform.Apply(adapter, config, portAlloc, Protocols)
212214
}
213215
}
214-
return annotations.ContainerDebugConfiguration{}, "", fmt.Errorf("unable to determine runtime for %q", container.Name)
215-
}
216-
217-
// operableContainerFromK8sContainer creates an instance of an operableContainer
218-
// from a v1.Container reference. This object will be passed around to accept
219-
// transforms, and will eventually overwrite fields from the creating v1.Container
220-
// in the manifest-under-transformation's pod spec.
221-
func operableContainerFromK8sContainer(c *v1.Container) *operableContainer {
222-
return &operableContainer{
223-
Command: c.Command,
224-
Args: c.Args,
225-
Env: k8sEnvToContainerEnv(c.Env),
226-
Ports: k8sPortsToContainerPorts(c.Ports),
227-
}
228-
}
229-
230-
func k8sEnvToContainerEnv(k8sEnv []v1.EnvVar) containerEnv {
231-
// TODO(nkubala): ValueFrom is ignored. Do we care?
232-
env := make(map[string]string, len(k8sEnv))
233-
var order []string
234-
for _, entry := range k8sEnv {
235-
order = append(order, entry.Name)
236-
env[entry.Name] = entry.Value
237-
}
238-
return containerEnv{
239-
Order: order,
240-
Env: env,
241-
}
242-
}
243-
244-
func containerEnvToK8sEnv(env containerEnv) []v1.EnvVar {
245-
var k8sEnv []v1.EnvVar
246-
for _, k := range env.Order {
247-
k8sEnv = append(k8sEnv, v1.EnvVar{
248-
Name: k,
249-
Value: env.Env[k],
250-
})
251-
}
252-
return k8sEnv
253-
}
254-
255-
func k8sPortsToContainerPorts(k8sPorts []v1.ContainerPort) []containerPort {
256-
var containerPorts []containerPort
257-
for _, port := range k8sPorts {
258-
containerPorts = append(containerPorts, containerPort{
259-
Name: port.Name,
260-
HostPort: port.HostPort,
261-
ContainerPort: port.ContainerPort,
262-
Protocol: string(port.Protocol),
263-
HostIP: port.HostIP,
264-
})
265-
}
266-
return containerPorts
267-
}
268-
269-
func containerPortsToK8sPorts(containerPorts []containerPort) []v1.ContainerPort {
270-
var k8sPorts []v1.ContainerPort
271-
for _, port := range containerPorts {
272-
k8sPorts = append(k8sPorts, v1.ContainerPort{
273-
Name: port.Name,
274-
HostPort: port.HostPort,
275-
ContainerPort: port.ContainerPort,
276-
Protocol: v1.Protocol(port.Protocol),
277-
HostIP: port.HostIP,
278-
})
279-
}
280-
return k8sPorts
281-
}
282-
283-
// applyFromOperable takes the relevant fields from the operable container
284-
// and applies them to the referenced v1.Container from the manifest's pod spec
285-
func applyFromOperable(o *operableContainer, c *v1.Container) {
286-
c.Args = o.Args
287-
c.Command = o.Command
288-
c.Env = containerEnvToK8sEnv(o.Env)
289-
c.Ports = containerPortsToK8sPorts(o.Ports)
216+
return annotations.ContainerDebugConfiguration{}, "", fmt.Errorf("unable to determine runtime for %q", adapter.GetContainer().Name)
290217
}

‎pkg/skaffold/debug/debug_test.go

+15-3
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2626

2727
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug/annotations"
28+
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug/types"
2829
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/graph"
2930
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/manifest"
3031
"github.com/GoogleContainerTools/skaffold/testutil"
@@ -113,11 +114,12 @@ func (t testTransformer) IsApplicable(config imageConfiguration) bool {
113114
return true
114115
}
115116

116-
func (t testTransformer) Apply(container *operableContainer, config imageConfiguration, portAlloc portAllocator, overrideProtocols []string) (annotations.ContainerDebugConfiguration, string, error) {
117+
func (t testTransformer) Apply(adapter types.ContainerAdapter, config imageConfiguration, portAlloc portAllocator, overrideProtocols []string) (annotations.ContainerDebugConfiguration, string, error) {
117118
port := portAlloc(9999)
118-
container.Ports = append(container.Ports, containerPort{Name: "test", ContainerPort: port})
119+
container := adapter.GetContainer()
120+
container.Ports = append(container.Ports, types.ContainerPort{Name: "test", ContainerPort: port})
119121

120-
testEnv := containerEnv{Order: []string{"KEY"}, Env: map[string]string{"KEY": "value"}}
122+
testEnv := types.ContainerEnv{Order: []string{"KEY"}, Env: map[string]string{"KEY": "value"}}
121123
container.Env = testEnv
122124

123125
return annotations.ContainerDebugConfiguration{Runtime: "test"}, "", nil
@@ -660,3 +662,13 @@ func TestSkipAnnotatedPodSpec(t *testing.T) {
660662
testutil.CheckDeepEqual(t, false, result)
661663
testutil.CheckDeepEqual(t, copy, pod) // should be unchanged
662664
}
665+
666+
type testAdapter struct {
667+
executable *types.ExecutableContainer
668+
}
669+
670+
func (t *testAdapter) GetContainer() *types.ExecutableContainer {
671+
return t.executable
672+
}
673+
674+
func (t *testAdapter) Apply() {}

0 commit comments

Comments
 (0)