-
Notifications
You must be signed in to change notification settings - Fork 64
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
driver: support for credentials helper and auth config file #265
Conversation
0a4a995
to
3efb2c4
Compare
This PR adds support for specifying an external credentials helper and/ or an external "auth.json" credentials file. The plugin configuration now has an "auth" block with fields "helper" and "config" (similar to the docker driver). We also now have an "auth_soft_fail" option in Task config for cases where someone has configured the auth block in plugin config, but has a task that is using a public image with no credentials. In that case setting auth_soft_fail is used to ignore the fact that no credentials will be found for the given public image. (This is also how the docker driver works, I didn't come up with this). Unlike the docker driver, the podman driver still does not support specifying a credentials helper _in_ the external "auth.json" credentials file. If there is demand for that use case we can add it, but in the short term it seems like just the plugin's support for specifying a credentials helper could be sufficient. And it's a lot of code to do the other thing.
Spot check, the e2e suite in the branch above where we try each configuration against a private authenticated registry.
|
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.
LGTM!
I don't know if it would be helpful in your case, but I have a test pattern when I create a proxy between the driver and a registry to manipulate requests
nomad-driver-podman/driver_test.go
Lines 1991 to 2076 in 40566ed
// TestPodmanDriver_Pull_Timeout verifies that the task image_pull_timeout | |
// configuration is respected and that it can be set to higher value than the | |
// driver's client_http_timeout. | |
// | |
// This test starts a proxy on port 5000 and requires the machine running the | |
// test to allow an insecure registry at localhost:5000. To run this test add | |
// the following to /etc/containers/registries.conf: | |
// | |
// [[registry]] | |
// location = "localhost:5000" | |
// insecure = true | |
func TestPodmanDriver_Pull_Timeout(t *testing.T) { | |
ci.Parallel(t) | |
// Check if the machine running the test is properly configured to run this | |
// test. | |
expectedRegistry := `[[registry]] | |
location = "localhost:5000" | |
insecure = true` | |
content, err := os.ReadFile("/etc/containers/registries.conf") | |
must.NoError(t, err) | |
if !strings.Contains(string(content), expectedRegistry) { | |
t.Skip("Skipping test because /etc/containers/registries.conf doesn't have insecure registry config for localhost:5000") | |
} | |
ctx, cancel := context.WithCancel(context.Background()) | |
defer cancel() | |
// Create a listener on port 5000. | |
l, err := net.Listen("tcp", "127.0.0.1:5000") | |
must.NoError(t, err) | |
// Proxy requests to quay.io except when trying to pull a busybox image. | |
parsedURL, err := url.Parse("https://quay.io") | |
must.NoError(t, err) | |
proxy := httputil.NewSingleHostReverseProxy(parsedURL) | |
proxy.ModifyResponse = func(resp *http.Response) error { | |
if strings.Contains(resp.Request.URL.Path, "busybox") { | |
select { | |
case <-ctx.Done(): | |
return nil | |
case <-time.After(5 * time.Second): | |
return fmt.Errorf("expected image pull to timeout") | |
} | |
} | |
return nil | |
} | |
// Create a new HTTP test server but don't start it so we can use our | |
// own listener on port 5000. | |
ts := httptest.NewUnstartedServer(proxy) | |
ts.Listener.Close() | |
ts.Listener = l | |
ts.Start() | |
defer ts.Close() | |
// Create the test harness and a sample task. | |
d := podmanDriverHarness(t, map[string]interface{}{ | |
"ClientHttpTimeout": "1s", | |
}) | |
task := &drivers.TaskConfig{ | |
ID: uuid.Generate(), | |
Name: "test", | |
AllocID: uuid.Generate(), | |
Resources: createBasicResources(), | |
} | |
// Time how long it takes to pull the image. | |
now := time.Now() | |
resultCh := make(chan error) | |
go func() { | |
// Pull image using our proxy. | |
image := "localhost:5000/quay/busybox:latest" | |
_, err = getPodmanDriver(t, d).createImage(image, &AuthConfig{}, true, 3*time.Second, task) | |
resultCh <- err | |
}() | |
err = <-resultCh | |
cancel() | |
// Verify that the timeout happened after client_http_timeout. | |
must.Greater(t, 2*time.Second, time.Since(now)) | |
must.ErrorContains(t, err, "context deadline exceeded") | |
} |
I think you could do something like that to check the auth header is being set properly but it's quite a bit of heavy lifting 😅
if usesAuth { | ||
authHeader, err := NewAuthHeader(auth) | ||
func (c *API) ImagePull(ctx context.Context, pullConfig *registry.PullConfig) (string, error) { | ||
pullConfig.Log(c.logger) |
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.
This reads a bit inverted to me, but something like c.logger.Info("pull config", pullConfig.LogTags()...)
looks a bit weird as well 🤔
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.
Yeah it's a bit unusual but it nicely hides the ~20 lines of gunk specific to the thing being logged
var ( | ||
headers = make(map[string]string) | ||
repository = pullConfig.Image | ||
tlsVerify = pullConfig.TLSVerify | ||
) |
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.
Just some Go curiosity, is there an advantage to declaring the variables like this instead of using the walrus :=
?
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.
Nope none at all; doing it this way just makes gofmt
line them up nicely
registry/authentication.go
Outdated
// todo: trace | ||
logger.Info("pull config", |
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.
// todo: trace | |
logger.Info("pull config", | |
logger.Trace("pull config", |
TODONE 😄
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.
😅
registry/fileconfig.go
Outdated
"os" | ||
) | ||
|
||
// [Glossery] |
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.
// [Glossery] | |
// [Glossary] |
// CredHelpers is currently not supported by the podman task driver | ||
// CredHelpers map[string]string `json:"credHelpers"` |
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.
Just to make sure I understand things, these credential helpers are the same concepts as the plugin config cred helper (a script that is executed to return a username and password), but are different because these are defined in the auth.json
file, so in order to support them we will have to parse them from the JSON file and run something like authFromCredsHelper
on them?
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.
That's exactly correct!
The main advantage to implementing support for specifying helpers in the auth.json
file is that you can directly correlate a registry/image to a specific helper.
However, you could also just implement your own credsHelper wrapper that does the same mapping, and invokes the correct helper given a specific registry.
Just a thought as a way to handle auth.json In the client /etc/nomad.d/nomad.env add REGISTRY_AUTH_FILE=/etc/nomad.d/registry/auth.json That's podman's way of handling an auth file as an environment variable. Just set it in the nomad.env that's already provided and you don't need to set it anywhere else. |
Where can I see an example of how to use it? I have not found any documentation on this |
Oh that's a good point @bru-msholomov we should copy over some of the docs from the docker driver (this mechanism was copied from there and works the same). The docker docs are here: https://developer.hashicorp.com/nomad/docs/drivers/docker#authentication and should get you started. |
This PR adds support for specifying an external credentials helper and/
or an external credentials file (e.g. "auth.json"). The plugin configuration
now has an
auth
block with fieldshelper
andconfig
(similar tothe docker driver). We also now have an
auth_soft_fail
option in Taskconfig for cases where someone has configured the
auth
block in pluginconfig, but has a task that is using a public image with no credentials.
In that case setting
auth_soft_fail
is used to ignore the fact thatno credentials will be found for the given public image. (This is also
how the docker driver works, I didn't come up with this).
Unlike the docker driver, the podman driver still does not support
specifying a credentials helper in the external "auth.json"
credentials file. If there is demand for that use case we can add
it, but in the short term it seems like just the plugin's support for
specifying a credentials helper could be sufficient. And it's a lot of
code to do the other thing.
Closes #132
Closes #174
Note: docs and changelog will be in a follow up PR (along with all the other 0.5.0 work).
e2e tests will be in https://github.com/hashicorp/nomad/commits/e2e-podman-private-registry
(still needs to tweaking to work with the actual e2e runner)