-
Notifications
You must be signed in to change notification settings - Fork 527
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
match package with matcher and utils to filter v1.Descriptors; v1.Platform.Equals utility #823
Conversation
Oh, notice with the |
867e826
to
501ae44
Compare
This seems to overlap a bit with #797 -- I don't feel strongly about either approach, but I don't think both should exist. Re: naming, I don't really love |
Hey @imjasonh 👋🏻 @jonjohnsonjr and I had a really long talk yesterday. He pointed out #797, as well as other things, see #821, as well as the ability to get the right image resolved and such for Agreed totally on where to put it. @jonjohnsonjr asked that I just put it up there in some package, and then we can use the PR to discuss where it should do (as we are doing). As for which one? 🤷♂️ This includes For remote registry, my entrypoint is an image ref string, which always returns a descriptor, either an index or manifest. That descriptor is captured as Essentially, For What we are looking for, then, is, something like: p, err := layout.Path(root)
desc, err := p.Get(matcher) // or maybe GetDescriptor(p, matcher)
desc.ImageIndex() // just like remote
desc.Image() // just like remote Does that help? |
I have no idea what that one failed test is complaining about. |
We went from an indirect dependency on opencontainers/image-spec to a direct dependency. If you run |
Got it. Ran and updated and pushed. |
I'm a little torn here because this is useful both as a helper for packages implementing an Given your precedent in #797, I'd lean |
pkg/v1/v1util/matcher.go
Outdated
|
||
// NameMatcher returns a ManifestMatcher that matches on the name tag in | ||
// github.com/opencontainers/image-spec/specs-go/v1.AnnotationRefName | ||
func NameMatcher(name string) ManifestMatcher { |
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.
Started typing out "what if we wanted to match multiple names?" but it might be better to have an API for composing matchers... e.g.:
And(Or(NameMatcher("foo"), NameMatcher("bar")), Or(PlatformMatcher(arm), PlatformMatcher(arm64)))
Which would match anything images with name "foo" or "bar" that also have platform "arm" or "arm64".
func Or(matchers ...ManifestMatcher) ManifestMatcher {
return func(desc v1.Descriptor) bool {
for _, m := range matchers {
if m(desc) {
return true
}
}
return false
}
}
func And(matchers ...ManifestMatcher) ManifestMatcher {
return func(desc v1.Descriptor) bool {
for _, m := range matchers {
if !m(desc) {
return false
}
}
return true
}
}
Now we've got a whole language, and we maybe want a new package for this craziness, e.g.:
match.And(
match.Or(match.Name("foo"), match.Name("bar")),
match.Or(match.Platform(arm), match.Platform(arm64))
)
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 is pretty complex. I suspect for the vast majority of use cases, people will be looking just for one name. This is the closest thing to remote.Get()
I can think of. How about if we start with the simple matchers, and then create composites later?
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.
Agreed, we shouldn't start here, but if we want to end up here, this is a good argument for pulling out matchers into a separate matching-only package so to make names a little less verbose and avoid polluting other package namespaces.
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 that is the approach, I can pull it out into a matcher
package. I am a little concerned about getting into a circular imports loop (matcher importing v1 importing matcher), but I think we will be ok.
Let me know.
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.
matcher importing v1 importing matcher
This is why v1
is (almost) strictly structs and interfaces. There's some stuff for JSON [de]serialization in there as well, but it should be as small as possible and have as few dependencies as possible. Any real behavior should go in a different package.
So let's stay away from it.
Could just go in a utilities package, but isn't that what |
Codecov Report
@@ Coverage Diff @@
## master #823 +/- ##
==========================================
+ Coverage 74.67% 74.85% +0.17%
==========================================
Files 103 105 +2
Lines 4348 4379 +31
==========================================
+ Hits 3247 3278 +31
Misses 619 619
Partials 482 482
Continue to review full report at Codecov.
|
pkg/v1/v1util/matcher.go
Outdated
|
||
// ManifestMatcher function that is given a v1.Descriptor, and returns whether or | ||
// not it matches a given rule. Can match on anything it wants in the Descriptor. | ||
type ManifestMatcher func(desc v1.Descriptor) bool |
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.
ManifestMatcher
as a name is too specific, we can reuse this type to match any descriptor, which can reference arbitrary things. We could just call this a Matcher
.
If we end up in a package called match
or matcher
or whatever, we may want to simply call this thing a Predicate
to avoid stuttering.
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 can reuse this type to match any descriptor, which can reference arbitrary things
That is a good point. Within Index
, we are passing it through the descriptors in the manifests, but it can be for anything.
If we end up in a package called match or matcher or whatever, we may want to simply call this thing a Predicate to avoid stuttering.
Are we moving it to match
or matcher
package? I am happy to do it, we just need to decide.
Predicate
Bit of a mouthful. I am not sure people will understand it at first glance, or know to look for it. It is, well, a "matcher". I know it stutters, but it still is easier to understand than "predicate".
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 we moving it to match or matcher package?
I'd prefer match
, simply because it's monosyllabic and shorter to type, but I'm open to arguments against it.
Bit of a mouthful.
Yeah that's a good point. I'd be fine with match.Matcher
as well... predicate just feels so right, you know? 😉
I'll leave it up to you, since you'll be doing the heavy lifting. Matcher
is two letters shorter as well, so it's roughly a tie in my vague, inexpressible, gut-feeling-based name-scoring belief system.
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.
😂
Those of us with engineering or mathematical training and/or experience tend to belittle those gut feelings; it is a mistake on our part. Guts count.
In line with what you wrote earlier, this really is not a manifest matcher, but a descriptor matcher. So I put it in match
package as Descriptor
, but that feels wrong. It is a Descriptor matcher. But then it gets ugly: match.DescriptorMatcher
? Of course, we might have an IndexMatcher
and a ManifestMatcher
and even BlobMatcher
in the future. Thoughts?
Here's what I'd propose:
I imagine we'll start with something like these, maybe in
And end up with something like this, not sure where:
We may want to just skip to the generic bits and do some type-checking (like in Maybe we should rename |
c589559
to
b0f5678
Compare
Responding to your suggestions:
Done.
Done.
I renamed it to My problem with it is that, on the one hand, it isn't a descriptor, but rather a matcher for a In line with that, the other funcs are now Happy for some better naming suggestions. We could have sub-packages under Actually, this makes me realize that the package probably should be |
You don't like it in partial, @imjasonh doesn't like it in partial, I don't like it in partial... why put it there? |
The functionality isn't quite right for it. "Filter" almost always implies, give in a list, get back a (possibly abbreviated) list of the same kind. Here we are doing more; give in an index, get back a filtered list. It is a convenience function with more than just filter. |
I imagine these make sense in v1util; isn't that what it is there for? Actually, I would be quite happy to get this PR to the right state, merge it in, and then do the others in another run. I only added I don't think the recursive ones work. Image
Index
I am struggling to find the recursive use case. I definitely see it in the other area (outside of this PR), where I want to |
This response got longer than I expected, so it might be somewhat incoherent. If I missed anything or you still disagree about something or have more questions, keep poking at it until we find consensus.
I think this makes sense if there are other things that we intend to match against, but given that Descriptors are intentionally a generic pointer to anything, and used throughout every artifact, I think it's fine to assume we only care about descriptors, especially for the sake of the API being cleaner. Interested in your thoughts here if you have other things in mind that would make sense for matchers.
I really think those two letters will make a difference if we implement the stupid composition DSL I proposed, which I think is very likely something I'll eventually do. I really like how e.g. The stutter on I'm mostly using https://blog.golang.org/package-names as a guide here, which makes me lean towards just
I don't think there's a better place for it right now, but it feels silly to create a new package just yet. Putting this in
Yeah good point, I'd agree with that.
Eh, kind of. If you look at what's in
Yeah, this is definitely my intention. I just want to think forward to other applications so that we can consider this API with more context. I want to merge whatever we're happy with as soon as we're happy with it so we can start using it. I'm happy to merge parts of this we agree on (at this point, I think
In practice that's usually true, but there's really nothing preventing someone from referencing another manifest or index from a Perhaps the behavior when encountering something like that is undefined, but people often [ab]use the registry for all kinds of stuff, so I don't know that it's always safe to assume a
One thing I've suggested to a bunch of people (not sure if anyone actually did it) for air-gapped environments (etc.) was to just pull everything down into an OCI image layout, ship that via physical media, and re-hydrate a destination registry by just uploading the entire index. If you also include the index.json by pushing it as an index to the registry, you get this incredible added benefit of a single reference entrypoint to a DAG that encompasses all of the images in a given release (or what have you). Then you can just think of your entire registry as a single artifact that is portable via a format that is defined by a standards body. It's pretty nice. I suggested this to the CNAB folks early on, and I think they didn't understand what I was saying because they (paraphrasing) suggested I was confused and this was unrelated, but I am pretty sure they have since adopted a similar approach. Also, "never" is a pretty strong word, here's an index I just pushed to make my point:
Certainly this is outside the scope of this PR, but in terms of discussing APIs I think it's an important use case to consider. One thing that jumps to mind is identifying artifacts that are affected by a CVE or something. Imagine that we implement ko-build/ko#221 and add version information as annotations to an image. If there were a bug in Similarly, imagine one of our base images has a CVE, and we want to find any images with a specific bad layer in an OCI layout bundle. A simple Walk with a matcher on a digest would do it in a couple lines. These are a bit contrived but they're not that far-fetched. |
I am not going to try to hit all of this right now. I will do the following:
So I will update this PR to restructure just those, and pull out the |
Is that really legitimate? I don't read the spec that way. A layer is a leaf. Sure, people abuse it, but that is not our problem as an implementation. I don't know of a single registry that would support it, or containerd. Sure, you could place a manifest or index in there, but nothing would read it and walk the tree down. |
No, they aren't that far-fetched. Still and all, I would think you would want something like: Walk(desc v1.Descriptor, Handler handlerFunc) that just walks down the tree. It looks for children on media-types that have children. I can see why you might do the same for layers, but I would just look at something and say, "you are an index, you have manifest children that I need to read; whether or not they have children depends on the manifest's media-type", following by, "you are a manifest, you have children that I need to read, but, no, they don't have children." It certainly is simpler to start, and nothing prevents us from expanding it in the future. |
OK, updated to just include:
Removed the If we are in agreement on the above, I can add some simple unit tests for each of the ~5 funcs we added. We need agreement that, subject to those tests, this is ready. |
Oh we might have just been speaking past each other. I agree we would want a signature like: Walk(desc v1.Descriptor, Handler handlerFunc) error NOT this: Walk(root ThingWithChildren, match.Matcher) []v1.Descriptor
I mostly agree with you. If we ever encountered something I described, we would almost certainly do the wrong thing or just return an error, but in the future that might change if we ever can agree on a better format than gzipped tarballs and json for representing filesystems. |
de753a7
to
0a17cb9
Compare
SGTM. This looks good (assuming tests and happy linters). My last pedantic nit will be that the godoc comments should end with a period/full stop (or really any sentence-terminating punctuation mark, if you're feeling festive). |
8cd45e1
to
6a808e0
Compare
And now you have unit tests for everything added in this PR. |
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.
A few small nits but otherwise lgtm, thanks :)
1a4219f
to
f0f1acb
Compare
Resolved everything except for the test package naming, to be discussed, but not such a big deal. Back at you @jonjohnsonjr |
Code LGTM, do you mind updating the PR and commit titles to reflect that you've added the match package and platform.Equals? Just for my sake in the future when scanning logs. Thanks for sticking through all this back and forth :) |
Sure, doing it right now. 30 seconds and it will be done. |
Urgh, I thought I fixed it, but the wrong changes are on the wrong commits. Time for some git-foo. Hang on another minute. |
Signed-off-by: Avi Deitcher <avi@deitcher.net>
Signed-off-by: Avi Deitcher <avi@deitcher.net>
Now I got it. Back at you @jonjohnsonjr
All good. I ma enjoying the conversations. Once this is in, I will open a PR with one or more of the |
Thanks! |
By the way, if I didn't say it before @jonjohnsonjr, this is pretty nice:
|
Starts to address #821
ManifestMatcher
and utilities that can be used to find manifests that match the provided matcherFindManifests
that takes av1.ImageIndex
and returns the manifests in it that match. This is useful in its own right, but will be even more useful later for tree walking.Some questions and follow-ons:
partial
was suggested but seems, well, off. This is a utilities package, but mostly seems to have internal utils, although it still works better thanpartial
. I think this could handle being renamed toutil
sincev1/v1util
is a mouthful.v1.layout
, index->index->manifest. That can be handled separately.Once this is good, we can start to add the next parts.
cc @jonjohnsonjr