feat: support new options in terminal #6805
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add `ExperimentalPrivilegedNesting` args in `DefaultTerminalCmdOpts` and `TerminalOpts`. Add `InsecureRootCapabilities` args in `DefaultTerminalCmdOpts` and `TerminalOpts`. Signed-off-by: Tom Chauveau <tom@epitech.eu>
added 2 commits
March 4, 2024 16:45
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
vito
approved these changes
Mar 4, 2024
There was a problem hiding this comment.
Approving in principle, will need a regen after the suggested docs tweaks though.
I know @shykes was flirting with the idea of enabling nesting by default, but this also solves for the insecure/privileged flag and otherwise maintains consistency with the current nesting flag, so it seems orthogonal to that decision.
core/schema/container.go
Outdated
| ArgDoc("args", `The args of the command.`), | ||
| ArgDoc("args", `The args of the command.`). | ||
| ArgDoc("experimentalPrivilegedNesting", | ||
| `Provides dagger access to the executed command.`, |
There was a problem hiding this comment.
Suggested change
| `Provides dagger access to the executed command.`, | |
| `Provides Dagger access to the executed command.`, |
There was a problem hiding this comment.
It's written without cap on withExec
dagger/core/schema/container.go
Lines 297 to 298 in 583944d
I did that to keep the consistency, should I update it too in withExec then?
core/schema/container.go
Outdated
| ArgDoc("cmd", `If set, override the container's default terminal command and invoke these command arguments instead.`), | ||
| ArgDoc("cmd", `If set, override the container's default terminal command and invoke these command arguments instead.`). | ||
| ArgDoc("experimentalPrivilegedNesting", | ||
| `Provides dagger access to the executed command.`, |
There was a problem hiding this comment.
Suggested change
| `Provides dagger access to the executed command.`, | |
| `Provides Dagger access to the executed command.`, |
There was a problem hiding this comment.
It's written without cap on withExec
dagger/core/schema/container.go
Lines 297 to 298 in 583944d
I did that to keep the consistency, should I update it too in withExec then?
There was a problem hiding this comment.
Gotcha makes sense - thanks for updating that too!
Comment on lines
+48
to
+51
| ExperimentalPrivilegedNesting *bool `default:"false"` | ||
|
|
||
| // Grant the process all root capabilities | ||
| InsecureRootCapabilities *bool `default:"false"` |
There was a problem hiding this comment.
Ah fun, I guess these need to be pointers so we can tell if you meant to override back to false.
There was a problem hiding this comment.
Exactly, I was confused at first but then I realised it was the only solution because we're using boolean :/
Signed-off-by: Tom Chauveau <tom@epitech.eu>
Signed-off-by: Tom Chauveau <tom@epitech.eu>
TomChv
deleted the
feat/support-privileges-and-nesting-on-default-terminal-command
branch
jedevc
reviewed
Mar 5, 2024
Comment on lines
+1365
to
+1367
| if args.Cmd == nil || len(args.Cmd) == 0 { | ||
| args.Cmd = ctr.Self.DefaultTerminalCmd.Args | ||
| } |
There was a problem hiding this comment.
Just hit this today - this line can panic if ctr.Self.DefaultTerminalCmd isn't set.
goroutine 457496 [running]:
runtime/debug.Stack()
/usr/local/go/src/runtime/debug/stack.go:24 +0x5e
github.com/dagger/dagger/dagql.(*Server).resolvePath.func1()
/app/dagql/server.go:642 +0x74
panic({0x1bd65a0?, 0x3139130?})
/usr/local/go/src/runtime/panic.go:914 +0x21f
github.com/dagger/dagger/core/schema.(*containerSchema).terminal(0x1d3a040?, {0x2299038, 0xc00330eea0}, {0xc005579280, 0xc00305f340, {0x0, 0x1, 0xc0033daf60, 0xc00394bed8}, 0x0}, ...)
/app/core/schema/container.go:1366 +0xc9
github.com/dagger/dagger/dagql.NodeFunc[...].func1({0xc005579280, 0xc00305f340, {0x0, 0x1, 0xc0033daf60, 0xc00394bed8}, 0x0}, 0xc00330ef00)
/app/dagql/objects.go:355 +0x184
github.com/dagger/dagger/dagql.Class[...].Call(0x22c3500?, {0x2299038?, 0xc00330eea0?}, {0xc005579280, 0xc00305f340, {0x0, 0x1, 0xc0033daf60, 0xc00394bed8}, 0x0}, ...)
/app/dagql/objects.go:202 +0x15d
github.com/dagger/dagger/dagql.Instance[...].Select(0x22b3a20, {0x2299038, 0xc00330eea0}, {{0xc000cd2508, 0x8}, {0x319c680, 0x0, 0x0}, 0x0})
/app/dagql/objects.go:271 +0x2fe
github.com/dagger/dagger/dagql.(*Server).cachedSelect.func1({0x2299038?, 0xc00330eea0?})
/app/dagql/server.go:583 +0x49
github.com/dagger/dagger/tracing.AroundFunc.ProgrockAroundFunc.func1({0x2299038, 0xc00330ed80})
/app/tracing/graphql.go:102 +0x5c4
github.com/dagger/dagger/tracing.AroundFunc.SpanAroundFunc.func2({0x2299038, 0xc003e691a0})
/app/tracing/graphql.go:33 +0xa6
github.com/dagger/dagger/dagql.(*cacheMap[...]).GetOrInitializeOnHit(0x22bf680, {0x2299038, 0xc003e68ea0}, {0xc003775130, 0x47}, 0xc004ccb788, 0xc0031f9b08)
/app/dagql/cachemap.go:85 +0x272
github.com/dagger/dagger/dagql.(*cacheMap[...]).GetOrInitialize(0xc0031f9b48?, {0x2299038?, 0xc003e68ea0?}, {0xc003775130?, 0x10c0f25?}, 0x229c200?)
/app/dagql/cachemap.go:60 +0x45
github.com/dagger/dagger/dagql.(*Server).cachedSelect(0xc003e1a4b0, {0x2299038, 0xc004a82630}, {0x229c200, 0xc002c9ac40}, {{0xc000cd2508, 0x8}, {0x319c680, 0x0, 0x0}, ...})
/app/dagql/server.go:592 +0x203
github.com/dagger/dagger/dagql.(*Server).resolvePath(0x1052c6b?, {0x2299038, 0xc004a82630}, {0x229c200?, 0xc002c9ac40?}, {{0xc000cd2508, 0x8}, {{0xc000cd2508, 0x8}, {0x319c680, ...}, ...}, ...})
/app/dagql/server.go:651 +0x13e
github.com/dagger/dagger/dagql.(*Server).Resolve.func1()
/app/dagql/server.go:405 +0x7f
github.com/dagger/dagger/dagql.(*Server).Resolve.(*ErrorPool).Go.func3()
/go/pkg/mod/github.com/sourcegraph/conc@v0.3.0/pool/error_pool.go:30 +0x22
github.com/sourcegraph/conc/pool.(*Pool).worker(0xc001825858?)
/go/pkg/mod/github.com/sourcegraph/conc@v0.3.0/pool/pool.go:154 +0x6f
github.com/sourcegraph/conc/panics.(*Catcher).Try(0x445edc?, 0x6?)
/go/pkg/mod/github.com/sourcegraph/conc@v0.3.0/panics/panics.go:23 +0x48
github.com/sourcegraph/conc.(*WaitGroup).Go.func1()
/go/pkg/mod/github.com/sourcegraph/conc@v0.3.0/waitgroup.go:32 +0x56
created by github.com/sourcegraph/conc.(*WaitGroup).Go in goroutine 372437
/go/pkg/mod/github.com/sourcegraph/conc@v0.3.0/waitgroup.go:30 +0x73
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add
ExperimentalPrivilegedNestingargs inDefaultTerminalCmdOptsandTerminalOpts.Add
InsecureRootCapabilitiesargs inDefaultTerminalCmdOptsandTerminalOpts.I did some test with a local module I made but I struggle figuring out how to test it in our integration's test.
I know we have
module_terminal_test.gofor terminal tests andcontainer_test.gofor inscure and nesting params but I cannot figure out a nice way to combine both because this requires extra installation and sending lines/checking return etc...I would love to have some ideas
/cc @sipsma @vito
Fixes #6712