-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
[BUG]: Fixed docker start command bug #1586
Changes from 54 commits
287bec0
9349a79
c2379e8
5f50192
e2af67e
7294389
a834839
215d3cf
4466e0a
ac0e8a3
40516aa
2538a33
f3ddde9
24bf9b6
5df1351
cc4e01f
cc61b00
cf471f4
6db5e69
618b9b0
bdc268a
8f53727
c985555
7242ebe
c52fabf
2192672
632245f
b2e5e03
1213a7d
dd36893
cae36df
eebc436
2ef121b
9a8902f
57b261a
f17e436
02c575c
40e4936
9c3f37a
9971bd1
a7cefef
cd2dc5d
0bc1384
4be8be6
5c33062
bc07952
8c74e63
7856a93
f5fc4cb
57e1847
c6899ee
1d334e2
fb2d240
7c37c68
98d4c43
dcb8ee7
358cefa
0989f0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,12 +69,12 @@ func (a *App) Setup(_ context.Context) error { | |
} | ||
a.docker = d | ||
|
||
if (a.kind == utils.Docker || a.kind == utils.DockerCompose) && IsDetachMode(a.cmd) { | ||
return fmt.Errorf("detach mode is not allowed in Keploy command") | ||
if utils.IsDockerKind(a.kind) && isDetachMode(a.logger, a.cmd, a.kind) { | ||
return fmt.Errorf("application could not be started in detached mode") | ||
} | ||
|
||
switch a.kind { | ||
case utils.Docker: | ||
case utils.DockerRun, utils.DockerStart: | ||
err := a.SetupDocker() | ||
if err != nil { | ||
return err | ||
|
@@ -100,7 +100,9 @@ func (a *App) ContainerIPv4Addr() string { | |
|
||
func (a *App) SetupDocker() error { | ||
var err error | ||
cont, net, err := ParseDockerCmd(a.cmd) | ||
|
||
cont, net, err := ParseDockerCmd(a.cmd, a.kind, a.docker) | ||
|
||
if err != nil { | ||
utils.LogError(a.logger, err, "failed to parse container name from given docker command", zap.String("cmd", a.cmd)) | ||
return err | ||
|
@@ -117,6 +119,16 @@ func (a *App) SetupDocker() error { | |
a.logger.Warn(fmt.Sprintf("given docker network:(%v) is different from parsed docker network:(%v)", a.containerNetwork, net)) | ||
} | ||
|
||
if a.kind == utils.DockerStart { | ||
running, err := a.docker.IsContainerRunning(a.container) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if the given container is different from the parsed container, we should provide the parsed container. i.e
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. corrected |
||
if err != nil { | ||
return err | ||
} | ||
if running { | ||
return fmt.Errorf("docker container is already in running state") | ||
} | ||
} | ||
|
||
//injecting appNetwork to keploy. | ||
err = a.injectNetwork(a.containerNetwork) | ||
if err != nil { | ||
|
@@ -420,7 +432,7 @@ func (a *App) runDocker(ctx context.Context) models.AppError { | |
func (a *App) Run(ctx context.Context, inodeChan chan uint64) models.AppError { | ||
a.inodeChan = inodeChan | ||
|
||
if a.kind == utils.DockerCompose || a.kind == utils.Docker { | ||
if utils.IsDockerKind(a.kind) { | ||
return a.runDocker(ctx) | ||
} | ||
return a.run(ctx) | ||
|
@@ -431,7 +443,7 @@ func (a *App) run(ctx context.Context) models.AppError { | |
userCmd := a.cmd | ||
username := os.Getenv("SUDO_USER") | ||
|
||
if utils.FindDockerCmd(a.cmd) == utils.Docker { | ||
if utils.FindDockerCmd(a.cmd) == utils.DockerRun { | ||
userCmd = utils.EnsureRmBeforeName(userCmd) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,9 +5,14 @@ import ( | |
"os" | ||
"path/filepath" | ||
"regexp" | ||
"slices" | ||
"strconv" | ||
"strings" | ||
"syscall" | ||
|
||
"go.keploy.io/server/v2/pkg/core/app/docker" | ||
"go.keploy.io/server/v2/utils" | ||
"go.uber.org/zap" | ||
) | ||
|
||
func findComposeFile() string { | ||
|
@@ -47,9 +52,17 @@ func modifyDockerComposeCommand(appCmd, newComposeFile string) string { | |
return fmt.Sprintf("%s -f %s", appCmd, newComposeFile) | ||
} | ||
|
||
func ParseDockerCmd(cmd string) (string, string, error) { | ||
func ParseDockerCmd(cmd string, kind utils.CmdType, idc docker.Client) (string, string, error) { | ||
|
||
// Regular expression patterns | ||
containerNamePattern := `--name\s+([^\s]+)` | ||
var containerNamePattern string | ||
switch kind { | ||
case utils.DockerStart: | ||
containerNamePattern = `start\s+(?:-[^\s]+\s+)*([^\s]*)` | ||
default: | ||
containerNamePattern = `--name\s+([^\s]+)` | ||
} | ||
|
||
networkNamePattern := `(--network|--net)\s+([^\s]+)` | ||
|
||
// Extract container name | ||
|
@@ -60,6 +73,17 @@ func ParseDockerCmd(cmd string) (string, string, error) { | |
} | ||
containerName := containerNameMatches[1] | ||
|
||
if kind == utils.DockerStart { | ||
networks, err := idc.ExtractNetworksForContainer(containerName) | ||
if err != nil { | ||
return containerName, "", err | ||
} | ||
for i := range networks { | ||
return containerName, i, nil | ||
} | ||
return containerName, "", fmt.Errorf("failed to parse network name") | ||
} | ||
|
||
// Extract network name | ||
networkNameRegex := regexp.MustCompile(networkNamePattern) | ||
networkNameMatches := networkNameRegex.FindStringSubmatch(cmd) | ||
|
@@ -86,10 +110,24 @@ func getInode(pid int) (uint64, error) { | |
return i, nil | ||
} | ||
|
||
func IsDetachMode(command string) bool { | ||
func isDetachMode(logger *zap.Logger, command string, kind utils.CmdType) bool { | ||
args := strings.Fields(command) | ||
|
||
if kind == utils.DockerStart { | ||
flags := []string{"-a", "--attach", "-i", "--interactive"} | ||
|
||
for _, arg := range args { | ||
if slices.Contains(flags, arg) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I made this error while refactoring changes. Refactored |
||
return false | ||
} | ||
} | ||
utils.LogError(logger, fmt.Errorf("docker start require --attach/-a or --interactive/-i flag"), "failed to start command") | ||
return true | ||
} | ||
|
||
for _, arg := range args { | ||
if arg == "-d" || arg == "--detach" { | ||
utils.LogError(logger, fmt.Errorf("detach mode is not allowed in Keploy command"), "failed to start command") | ||
return true | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -349,7 +349,7 @@ func (r *Recorder) ReRecord(ctx context.Context, appID uint64) error { | |
|
||
} | ||
cmdType := utils.FindDockerCmd(r.config.Command) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here also |
||
if cmdType == utils.Docker || cmdType == utils.DockerCompose { | ||
if utils.IsDockerKind(cmdType) { | ||
host = r.config.ContainerName | ||
} | ||
|
||
|
@@ -360,7 +360,7 @@ func (r *Recorder) ReRecord(ctx context.Context, appID uint64) error { | |
|
||
allTestCasesRecorded := true | ||
for _, tc := range tcs { | ||
if cmdType == utils.Docker || cmdType == utils.DockerCompose { | ||
if utils.IsDockerKind(cmdType) { | ||
|
||
userIP, err := r.instrumentation.GetContainerIP(ctx, appID) | ||
if err != nil { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are you getting
a.container
value, you are not even parsing the docker start command to get the container name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not working for me in both cases. Whether the container is present locally or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User can't give container name in docker start command, they have to use containerName flag for that. So we don't have to parse the command here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keploy cmd would something be like
keployV2 record -c "docker start -a MongoApp" --containerName MongoApp --networkName keploy-network
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why can't we just parse because you have to specify the container name when starting a stopped container? We should parse it by default and also let the user set the value just like how we are doing in the case of docker run command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be working on that then.