-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add oc image append
which adds layers to a schema1/2 image
#20027
Conversation
oc image append
which adds layers to a schema1/2 imageoc image append
which adds layers to a schema1/2 image
@bparees this is most of the logic from the previous "add layer via image API", but modeled as a CLI command, sharing code with mirror, and much simpler (since the surface area is just taking docker images from a registry and slicing them). It does not support v1 push or read. It handles mounting and reuse of layers, lets you almost completely rewrite the image metadata, downconversion and up conversion from schema1/2, and add arbitrary layers. I would imagine having this as a utility command, and then refactoring the core and reusing with a builder step that would interact with container-snapshot. But this is useful on its own. |
/test all |
1293ecd
to
f22d059
Compare
/retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smarterclayton The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
oc image append
which adds layers to a schema1/2 imageoc image append
which adds layers to a schema1/2 image
/retest |
1 similar comment
/retest |
a8014a7
to
5d58c39
Compare
/retest |
11a0b76
to
c319b26
Compare
Author: in.Author, | ||
Architecture: in.Architecture, | ||
Size: in.Size, | ||
OS: "linux", |
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.
i feel like this is going to bite us as soon as we try to do multiarch... do we not have the image's architecture info anywhere? should this at least be based on runtime.GOARCH like we do on manifestlist conversion?
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.
If you don't have this data, the only thing it supports was linux (docker schema changed to allow this, but older images had no metadata).
|
||
Layers may be provided as arguments to the command and must each be a gzipped tar archive | ||
representing a filesystem overlay to the inherited images. The archive may contain a "whiteout" | ||
file (the prefix '.wh.' and the filename) which will hide files in the lower layers. All |
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 this your convention or docker's? is there an escape mechanism if i really want to prefix a file with .wh
?
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 is docker's (and I think was also one of the kernel filesystem's conventions).
|
||
example = templates.Examples(` | ||
# Remove the entrypoint on the mysql:latest image | ||
%[1]s --from mysql:latest --to myregistry.com/myimage:latest --image {"Entrypoint":null} |
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.
does this get merged w/ the other metadata fields? or the whole config becomes empty except for Entrypoint?
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.
nm looks like it's a patch per the help below.
} | ||
|
||
flag := cmd.Flags() | ||
flag.BoolVar(&o.DryRun, "dry-run", o.DryRun, "Print the actions that would be taken and exit without writing to the destinations.") |
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.
should be destination (singular)? Doesn't look like you're allowing multiple destinations?
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, copy pasta
} | ||
|
||
// schema2ManifestOnly specifically requests a manifest list first | ||
var schema2ManifestOnly = distribution.WithManifestMediaTypes([]string{ |
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.
schema2ManifestListOnly? schema2Only?
) | ||
|
||
// TDOO: remove when quay.io switches to v2 schema | ||
func loadPrivateKey() (libtrust.PrivateKey, 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.
this can't be shared w/ the implementation in append?
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 probably can. I didn't want it to be too generic at this point.
"github.com/golang/glog" | ||
) | ||
|
||
type workQueue struct { |
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.
can't share w/ the the append workqueue?
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.
I plan to refactor soon - it's a little too specialized, so I was concerned about generalizing too early.
test/extended/images/append.go
Outdated
// best effort to get the format string for the release | ||
router, err := cli.AdminAppsClient().Apps().DeploymentConfigs("default").Get("router", metav1.GetOptions{}) | ||
if err != nil { | ||
g.Skip(fmt.Sprintf("Unable to find router in order to query format string: %v", err)) |
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 seems fragile and likely to lead to this test being skipped w/o anyone noticing.
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.
We do this in other tests, I agree it's fragile, but we also need the e2e tests to work against a wider variety of servers. I could fail it, I guess.
VolumeMounts: []kapiv1.VolumeMount{ | ||
{ | ||
Name: "pull-secret", | ||
MountPath: "/secret/.dockercfg", |
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.
.docker/config.json
? (just to avoid confusion) or is our SA secret really still in .dockercfg
format?
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.
Still in that format.
ec8f573
to
6bb87b3
Compare
/retest |
2 similar comments
/retest |
/retest |
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.
minor questions, depending on the answers, lgtm.
pkg/image/dockerlayer/add/add.go
Outdated
return manifest, 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 dead is this commented out code? should it be removed or TODOed?
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.
Totally, dead, accident. Will remove and repush
flag.StringVar(&o.FilterByOS, "filter-by-os", o.FilterByOS, "A regular expression to control which images are mirrored. Images will be passed as '<platform>/<architecture>[/<variant>]'.") | ||
|
||
flag.StringVar(&o.From, "from", o.From, "The image to use as a base. If empty, a new scratch image is created.") | ||
flag.StringVar(&o.To, "to", o.To, "The Docker repository tag to upload the appended image to.") |
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.
thoughts on whether this should support imagestreamtags as a "to" target in the future? (either using the --to flag, or introducing an new flag like --to-imagestreamtag ?)
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.
(also --from for that matter)
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 for both mirror and append I was thinking no, because all clusters should be exposing a public registry URL via the router, and we have oc registry login. So you should be able to easily get the external name and then use it.
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.
Also, you need registry api access anyway to use these commands, so not a burden
Regressed when we started using our own logic.
Unify location of some base layer constants
These files will be copied for now and then refactored later into reusable packages.
system:serviceaccount:blah:blah is incorrectly encoded into base64 for authorization, causing the successive login to fail. Instead, encode it as system-serviceaccount-namespace-name which is ignored by the registry. Also add an insecure flag to skip-check that will bypass validating TLS certs during the skip-check.
These are moves of existing code in the same set, or new, simple dependency packages that bring in no major subtrees.
This command can take zero or more gzipped layer tars (in Docker layer format) and append them to an existing image or a scratch image and then push the new image to a registry. Layers in the existing image are pushed as well. The caller can mutate the provided config as it goes.
/retest Please review the full test history for this PR and help us cut down flakes. |
@smarterclayton: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/retest |
This command can take zero or more gzipped layer tars (in Docker layer
format) and append them to an existing image or a scratch image and then
push the new image to a registry. Layers in the existing image are pushed
as well. The caller can mutate the provided config as it goes.
Pushing will fall back to schema1 if schema2 is not available, but it will always prefer that form.
Command is experimental for 3.11. Will be considered as part of CI use cases for creating new images, potentially building release layers, and completing content.