Skip to content
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

allow using the promoter for diffing GCR contents #134

Merged
merged 5 commits into from Sep 24, 2019

Conversation

@listx
Copy link
Contributor

listx commented Sep 24, 2019

The new flags are -manifest-based-snapshot-of and -minimal-snapshot.

-manifest-based-snapshot-of=<registry>: This is only usable with either a -manifest or -manifest-dir flag. This is like -snapshot, but instead of asking the registry for images information, it only looks at the image information in -manifest/-manifest-dir (the promoter manifest(s)), filtered by <registry>. The use case is to generate a sort of "global" manifest given a set of smaller manifests (case in point: the new staging repos). The YAML output here can be diffed to create a delta with the output of -snapshot for auditing purposes.

-minimal-snapshot: This can be paired with -manifest-based-snapshot-of or -snapshot`; it removes all images in the output YAML that are children of DockerManifestLists. So if a manifest list "X" refers to A, B, and C (and A/B/C are all untagged to purposely discourage architecture-specific images), using this flag removes A/B/C (if they are tagless) from the output of the snapshotting flags.

/assign @justinsb

listx added 3 commits Sep 21, 2019
No functional change.
This is because tagless promotions, by definition, cannot overlap with
each other (as the only "pointer" to them are digests which act as
checksums of the content). So, they should always be included.

Also add unit tests.
This flag can condense all of the manifest information from a
-manifest-dir, and output a complete inventory of all images we would
like to promote (filtered by the given destination registry name). This
is useful if, for example, we would like to know the complete catalogue
of all images that we would like to "exist" at a destination registry,
using the promoter manifests alone as the source of truth.

In a sense, this flag can be thought of as a sort of "manifest"
normalizer that can create a new manifest out of a bunch of smaller
manifest files (at least the `images:` portion of it).

This comes in handy in case we want to cross-reference the output of
-snapshot against a known good set of promoter manifests that are solely
responsible for adding new images to the snapshotted repo, for purposes
of auditing.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Sep 24, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: listx

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@listx

This comment has been minimized.

Copy link
Contributor Author

listx commented Sep 24, 2019

/hold
/cc @ps882

@k8s-ci-robot k8s-ci-robot requested a review from ps882 Sep 24, 2019
@listx

This comment has been minimized.

Copy link
Contributor Author

listx commented Sep 24, 2019

/cc @thockin These flags will make auditing (during disaster recovery backups) more accurate. I intend to just diff the output YAML of -snapshot=<registry> vs -manifest-based-snapshot-of=<registry> and expect them to match.

@listx

This comment has been minimized.

Copy link
Contributor Author

listx commented Sep 24, 2019

The linter is failing because of a new release v1.19.0 of the linter. I'll push up a new commit to fix that.

@@ -89,10 +89,18 @@ func main() {
"read all images in a repository and print to stdout")
snapshotTag := ""
flag.StringVar(&snapshotTag, "snapshot-tag", snapshotTag, "only snapshot images with the given tag")
minimalSnapshotPtr := flag.Bool(
"minimal-snapshot",

This comment has been minimized.

Copy link
@justinsb

justinsb Sep 24, 2019

Contributor

I was thinking about "better" names, but I actually like that this is a statement of intent rather than just being narrowly focused on manifest lists. 👍

cip.go Outdated
ServiceAccount: *snapshotSvcAccPtr,
Src: true,
}
}

This comment has been minimized.

Copy link
@justinsb

justinsb Sep 24, 2019

Contributor

It's surprising (at first glance) that we don't set mfests here, but we do in the *snapshotPtr case. Deliberate?

This comment has been minimized.

Copy link
@listx

listx Sep 24, 2019

Author Contributor

Oops, this is an oversight. Ack

@@ -196,7 +215,7 @@ func main() {
// If there are no images in the manifest, it may be a stub manifest file
// (such as for brand new registries that would be watched by the promoter
// for the very first time).
if len(*snapshotPtr) == 0 {
if doingPromotion && len(*manifestBasedSnapshotOf) == 0 {

This comment has been minimized.

Copy link
@justinsb

justinsb Sep 24, 2019

Contributor

Is doingPromotion is actually doing anything here? This looks like the only place you read the var, and it looks like it must be true when we get here anyway

This comment has been minimized.

Copy link
@listx

listx Sep 24, 2019

Author Contributor

Hmm it's only true if we pass in -manifest or -manifest-dir.

// ConditionFunc type.
if err == nil &&
gManifestList != nil &&
len(gManifestList.Manifests) > 0 {

This comment has been minimized.

Copy link
@justinsb

justinsb Sep 24, 2019

Contributor

Aside: is it possible to have a manifest list with no manifests? Maybe this is what you're hitting on the odd real-world cases.

This comment has been minimized.

Copy link
@listx

listx Sep 24, 2019

Author Contributor

Good point. I wrote down your thoughts in my debugging notebook!


return true, nil
}
return false, nil

This comment has been minimized.

Copy link
@justinsb

justinsb Sep 24, 2019

Contributor

If you get an error, I think you should log it, even if only at klog.V(2)

This comment has been minimized.

Copy link
@listx

listx Sep 24, 2019

Author Contributor

Hmm I do log errors encountered by the child function getGManifestListFrom()... Still, I could log oddities such as not having encountered an error but finding that for example the child manifests list is empty. Ack

for digest, tagSlice := range digestTags {
_, hasParent := sc.ParentDigest[digest]
// If this digest is referenced by a parent ManfestList, skip over
// it (do not add to "filtered") only if it is also tagless.

This comment has been minimized.

Copy link
@justinsb

justinsb Sep 24, 2019

Contributor

You could simplify the language here e.g. "If this image digest is only referenced as part of a parent ManifestList (i.e. not directly tagged), we filter it out"

This comment has been minimized.

Copy link
@listx

listx Sep 24, 2019

Author Contributor

Ack.

httpReq.Header.Add("Accept", "*/*")

if err != nil {
klog.Fatalf(

This comment has been minimized.

Copy link
@justinsb

justinsb Sep 24, 2019

Contributor

Idea for the future: Maybe tackling stream.Producer is a reasonable place to start in terms of making the code simpler - can we just return a ([]byte, error)

This comment has been minimized.

Copy link
@listx

listx Sep 24, 2019

Author Contributor

Ack.

if err == io.EOF {
break
}
fmt.Println("DECODING ERROR:", err)

This comment has been minimized.

Copy link
@justinsb

justinsb Sep 24, 2019

Contributor

Nit: use klog instead of stdout

This comment has been minimized.

Copy link
@listx

listx Sep 24, 2019

Author Contributor

Ack.


rii := make(RegInvImage)

destRegistry = strings.TrimRight(destRegistry, "/")

This comment has been minimized.

Copy link
@justinsb

justinsb Sep 24, 2019

Contributor

Don't you want to consistently add the /, so gcr.io/foo doesn't match gcr.io/foobar ?

This comment has been minimized.

Copy link
@listx

listx Sep 24, 2019

Author Contributor

This is actually sanitizing the *manifestBasedSnapshotOf pointer value as we pass that on directly to this function from cip.go.

@@ -250,6 +251,15 @@ type RegistryContext struct {
Src bool `yaml:"src,omitempty"`
}

// GManifestListContext is used only for reading GManifestList information from

This comment has been minimized.

Copy link
@justinsb

justinsb Sep 24, 2019

Contributor

Nit: GCR might be a better prefix than G

This comment has been minimized.

Copy link
@listx

listx Sep 24, 2019

Author Contributor

Ack

@justinsb

This comment has been minimized.

Copy link
Contributor

justinsb commented Sep 24, 2019

A few nits / queries, but overall LGTM

@justinsb

This comment has been minimized.

Copy link
Contributor

justinsb commented Sep 24, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 24, 2019
@listx listx changed the title additional flags related to -snapshot additional flags related to -snapshot to aid in disaster recovery and auditing Sep 24, 2019
@listx listx changed the title additional flags related to -snapshot to aid in disaster recovery and auditing allow using the promoter for diffing GCR contents Sep 24, 2019
@listx listx force-pushed the listx:master branch from 075b765 to b5cfee5 Sep 24, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 24, 2019
listx added 2 commits Sep 21, 2019
This flag removes all digests that are referenced by a
DockerManifestList. This results in a much more compact output, because
ManifestLists frequently refer to tagless child digests.

The new functions are patterned after the existing ReadRegistries()
function, so there are many parallels.
Ignore overly pedantic linters.
@listx listx force-pushed the listx:master branch from b5cfee5 to fc2e894 Sep 24, 2019
@listx

This comment has been minimized.

Copy link
Contributor Author

listx commented Sep 24, 2019

/test pull-cip-e2e

@justinsb

This comment has been minimized.

Copy link
Contributor

justinsb commented Sep 24, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 24, 2019
@listx

This comment has been minimized.

Copy link
Contributor Author

listx commented Sep 24, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit f4a03d2 into kubernetes-sigs:master Sep 24, 2019
4 of 5 checks passed
4 of 5 checks passed
tide Not mergeable. Should not have do-not-merge/hold label.
Details
cla/linuxfoundation listx authorized
Details
pull-cip-e2e Job succeeded.
Details
pull-cip-lint Job succeeded.
Details
pull-cip-unit-tests Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.