-
Notifications
You must be signed in to change notification settings - Fork 788
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
Added yaml2 sync config #1792
Added yaml2 sync config #1792
Conversation
Signed-off-by: Max Howell <[email protected]>
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.
Thanks!
Just a very quick skim, focusing basically only on the config format semantics.
@@ -50,9 +50,10 @@ type syncOptions struct { | |||
|
|||
// repoDescriptor contains information of a single repository used as a sync source. |
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.
“used as a sync source” is no longer quite true I guess?
ImageRefs []types.ImageReference // List of tagged image found for the repository | ||
Context *types.SystemContext // SystemContext for the sync command | ||
DirBasePath string // base path when source is 'dir' | ||
DestinationRef types.ImageReference // Destination |
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.
types.ImageReference
inherently points at a single image. It should not be abused as a repo/directory value.
One way to do this might be to move the destinationReference
calls into repoDescriptor
construction somehow, and to have repoDescriptor
already contain fully-resolved (source reference, destination reference) pairs.
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 would be nice to try and render everything down to pairs of source and destination references regardless of how sync was called i.e. using specific transports from the command line or using one of the yaml configs. I think this is possible however it could be a rather large change
@@ -74,6 +75,25 @@ type registrySyncConfig struct { | |||
// sourceConfig contains all registries information read from the source YAML file | |||
type sourceConfig map[string]registrySyncConfig | |||
|
|||
// syncConfig contains all registries information read from the source YAML file for source yaml2 | |||
type syncConfig map[string]registrySyncConfigV2 |
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.
-
So is this
syncConfigV2
? -
One of the major lessons from v1 is that having a top-level
map[something]something
structure makes it impossible to add options at that level. So maybe the top-level should be astruct
here as well.
// registrySyncConfigV2 contains all information read from the yaml2 sync config for a registry | ||
type registrySyncConfigV2 struct { | ||
Repos map[string]repoSyncConfig | ||
Credentials types.DockerAuthConfig // Username and password used to authenticate with the 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.
Are credentials really a per-SyncConfig
thing?
Conceptually we need both source and destination credentials. How does that work here?
(I guess credentials should be a separate top-level map but I’m not at all sure about that.)
type registrySyncConfigV2 struct { | ||
Repos map[string]repoSyncConfig | ||
Credentials types.DockerAuthConfig // Username and password used to authenticate with the registry | ||
TLSVerify tlsVerifyConfig `yaml:"tls-verify"` // TLS verification mode (enabled by default) |
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 should be separate for source and destination. (And, like credentials, possibly separate from the repo configurations — it makes no sense for two repo destinations on the same registry to use a different value.)
// It returns a repository descriptors slice with as many elements as the images | ||
// found and any error encountered. Each element of the slice is a list of | ||
// image references, to be used as sync source. | ||
func imagesToCopyFromRegistryV2(registryName string, cfg registrySyncConfigV2, sourceCtx types.SystemContext, destination string) ([]repoDescriptor, error) { |
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.
Would it be practical at all, instead of duplicating this large logic, to instead convert v1 data into the v2 format, and have only one execution path?
logrus.Errorf("destination ref type is unsupported for this sync") | ||
return fmt.Errorf("destination ref type is unsupported for this sync") | ||
} | ||
} else { |
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.
Is it really valuable at all to allow specifying a destination on the command line with YAML2? I imagine it’s always going to be the same for the same YAML file, though I’m not quite sure.
(OTOH if we don’t support this code path, we need to account for directory destinations in the YAML format.)
Thanks for the quick response @mtrmac. You make a good point about the config being a map removing some flexibility, I agree a struct would be better. Considering your comments from above, how about something along the lines of this for new config format: syncs:
- source:
registry: example.docker.io
tls-verify: true
cert-dir: /example/certs
credentials:
username: joeBlogs123
password: password123
destination:
registry: my.other.registry
tls-verify: false
cert-dir: /example/other/certs
credentials:
username: joeBlogs456
password: password456
repos:
- source-path: example/image
destination-path: example/test/image
images:
- v1.0
- v1.1
- v2.0
- sha256:0000000000000000000000000000000011111111111111111111111111111111
images-by-tag-regex: ^v[0-9]\.0$
- source-path: example/image2
destination-path: image2
images:
- "1.0"
- "1.1"
- "2.0"
- sha256:0000000000000000000000000000000011111111111111111111111111111111
images-by-tag-regex: ^[0-9]\.1$ Each element in We could go further and seperate the registries out completly and then use them as either sources or destinations e.g. registries:
- name: reg0
domain: example.docker.io
tls-verify: true
cert-dir: /example/certs
credentials:
username: joeBlogs123
password: password123
- name: reg1
domain: my.other.registry
tls-verify: false
cert-dir: /example/certs
credentials:
username: joeBlogs456
password: password456
- name: reg2
domain: my.backup.registry
tls-verify: false
cert-dir: /example/certs
credentials:
username: joeBlogs789
password: password789
repos:
- source: reg0
destination: reg1
source-path: example/image
destination-path: example/test/image
images:
- v1.0
- v1.1
- v2.0
- sha256:0000000000000000000000000000000011111111111111111111111111111111
images-by-tag-regex: ^v[0-9]\.0$
- source: reg1
destination: reg3
source-path: example/image2
destination-path: image2
images:
- "1.0"
- "1.1"
- "2.0"
- sha256:0000000000000000000000000000000011111111111111111111111111111111
images-by-tag-regex: ^[0-9]\.1$ |
The second version (separate registries configuration from repo sets) seems preferable to me, because I’m guessing that many users have a single private / disconnected mirror that contains copies from several internet repositories; in that case it’s useful to only have to specify the single-mirror credentials once. But that’s a guess without any data. What do actual users of |
|
A friendly reminder that this PR had no activity for 30 days. |
A friendly reminder that this PR had no activity for 30 days. |
I believe the option to abstract registries away could actually complicate things. If all I need is a simple case "paths on the source and destination should be the same", supplying paths and repos for every set of images becomes a task that needs a templating engine on top to actually generate the config. |
A friendly reminder that this PR had no activity for 30 days. |
@mhowell24 Still working on this? |
@rhatdan sorry, I haven't had any time to work on this recently. I might have some some time next weekend but cant promise anything |
A friendly reminder that this PR had no activity for 30 days. |
Adds the yaml2 sync configuration that allows rewriting of the repository path on a repo by repo basis. This largely follows the new yaml format suggested in #1531 and fixes #1072. I have written some testing but need to update the documentation to document the new configuration and its features.
Signed-off-by: Max Howell [email protected]