fix: search $DOCKER_CONFIG if no base64 config is provided
#398
Conversation
mafredri
force-pushed
the
mafredri/fix-docker-config-path
branch
from
b34692e
to
147ba65
Compare
mafredri
force-pushed
the
mafredri/fix-docker-config-path
branch
from
147ba65
to
8c0198f
Compare
|
CI seems to have issue with privilege and remounting, not sure why it applies to my test and not others. Looking into it. Also realized we'll have to copy over |
| // Remove the Docker config secret file! | ||
| if err := cleanupDockerConfigOverride(); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Review: Moved this higher up so that when we reset DOCKER_CONFIG, we don't interfere with envbuilder trying to set envs from the build/devcontainer.
mafredri
force-pushed
the
mafredri/fix-docker-config-path
branch
5 times, most recently
from
2f1992e
to
54fe86d
Compare
mafredri
force-pushed
the
mafredri/fix-docker-config-path
branch
from
54fe86d
to
5da2aaa
Compare
integration/integration_test.go
Outdated
| require.NoError(t, err) | ||
|
|
||
| require.Contains(t, logbuf.String(), "Set DOCKER_CONFIG to /.envbuilder") | ||
| require.NotContains(t, logbuf.String(), "Using DOCKER_CONFIG at ") |
There was a problem hiding this comment.
Do we generally trust log output for test assertions if we aren't testing logs?
If this log line ever drifts, this test will become a silent false positive. Is there a practical way to assert against the actual behaviour of the system? Could we copy the written docker config secret file so that it makes its way into the container and then assert against its contents?
Would that approach be worth it?
There was a problem hiding this comment.
We could modify ENVBUILDER_INIT_SCRIPT to write the content of $DOCKER_CONFIG and check the value from inside the container
There was a problem hiding this comment.
@SasSwart good points, I agree, in general it’s best to test behavior and not output. @johnstcn I wanted to do that here but we can’t do it in the init script since it runs after cleanup.
Not sure if it’ll work but we could try to copy it as part of Dockerfile build instructions, will look into that.
There was a problem hiding this comment.
Went back to the drawing board with the implementation, changed the behavior for user-provided config files and converted the integration tests to primarily testing behavior.
mafredri
force-pushed
the
mafredri/fix-docker-config-path
branch
from
78d72f3
to
86db5a5
Compare
| require.Equal(t, want, got) | ||
| } | ||
|
|
||
| // Ensure that a warning message is printed if config secrets |
There was a problem hiding this comment.
Why do we only warn on this? Users may have a reason to use a DOCKER_CONFIG at build time and they may have a reason to use one at run time. But those are separate use cases as far as I can tell. Should we allow the line between the two to blur?
Would it be more secure and user friendly to treat build time DOCKER_CONFIG like a build secret; ensuring that it's always cleared?
Runtime secrets could then be provided using a runtime mechanism? Right users are mounting it in, but to me this would be treated just like any other runtime secret.
There was a problem hiding this comment.
Why do we only warn on this? Users may have a reason to use a DOCKER_CONFIG at build time and they may have a reason to use one at run time. But those are separate use cases as far as I can tell. Should we allow the line between the two to blur?
That's a valid question. We can't determine the user's intent here. I'd say it's more likely that a given DOCKER_CONFIG is intended for build time since there's no guarantee Docker will be available in the resulting devcontainer.
Adding to this, if the devcontainer should have a DOCKER_CONFIG set at runtime, that should be done via containerEnv in devcontainer.json, not on the envbuilder container.
Would it be more secure and user friendly to treat build time DOCKER_CONFIG like a build secret; ensuring that it's always cleared?
Perhaps, but this ties into why we "only warn". We can't guarantee that we'll be able to wipe the secrets if the user has mounted them in, possibly as read-only, and the container doesn't have unmount permissions (or, will remounting be possible if there's sudo?). That's why we only warn and don't attempt to clear. If we attempted something else, we may risk introducing behavior that varies depending on the environment it's run in. It's better if the user knows what to expect.
Also, since we restore the original DOCKER_CONFIG, we're allowing that config location to be used at runtime, enabling use-cases where you have a separate build and runtime config. You set the build config via base64 env (takes precedence), and after the build, the location will be used at runtime. As a side-effect, the same config can be used for both build and runtime, which might not be ideal, but what's the alternative?
Runtime secrets could then be provided using a runtime mechanism? Right users are mounting it in, but to me this would be treated just like any other runtime secret.
Not sure what you're referring to by runtime secret and runtime mechanism? Could you expand on this?
| continue | ||
| } | ||
|
|
||
| logf(log.LevelWarn, "Found Docker config at %s, this file will remain after the build", path) |
| // Always verify that the Docker configuration secret file is removed. | ||
| output := execContainer(t, ctrID, "stat "+configJSONContainerPath) | ||
| require.Contains(t, output, "No such file or directory") | ||
| }) | ||
| } |
mafredri
changed the title
$DOCKER_CONFIG if no base64 config is provided
(cherry picked from commit c4b082e)
Fixes #392