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

match.Platform but ignore variant #892

Closed
deitch opened this issue Dec 30, 2020 · 7 comments
Closed

match.Platform but ignore variant #892

deitch opened this issue Dec 30, 2020 · 7 comments

Comments

@deitch
Copy link
Collaborator

deitch commented Dec 30, 2020

In order to match on a platform, we have match.Platforms. Most of the time this works well.

However, the variant part of the platform, described in the spec is listed as "OPTIONAL".

The current match.Platforms has no way of indicating, "match my OS/Architecture but ignore the variant". For that matter, I think it may be a broader question of a way to tell the platform-matcher to ignore some fields. E.g. I want all entries that have architecture=amd64, I don't care about the OS, etc.

I am happy to open a PR on this, if we can agree on the right interface.

Options

Some options:

match.Any

Some form of match.Any option, such that you could provide it to any given field. So you would do:

match.Platforms(v1.Platform{Architecture: "amd64", OS: match.Any, Variant: match.Any})

I am not sure how to make that work; some fields in v1.Platform are string, others are []string. Maybe it just works for the string fields?

Wrapper

A different variant might be a wrapper, where we could do:

match.IgnoreBlank(match.Platforms(platforms ...v1.Platform))

The above tells it to reject fields that do not match, but ignore blank. Again, not sure how to make this work. Don't we need to tell match.Platforms to ignore blanks?

Alternate

Another option is to have an alternative matcher:

match.PlatformsWithIgnoreBlank(platforms ...v1.Platform)

I don't like the nomenclature, but the idea isn't bad.

I looked at a parameter, but the platforms arg already is variadic.

Thoughts?

@jonjohnsonjr
Copy link
Collaborator

However, the variant part of the platform, described in the spec is listed as "OPTIONAL".

So this is where things get a little fuzzy. The spec also says:

This OPTIONAL property describes the minimum runtime requirements of the image.

Simiarly, for os.features:

This OPTIONAL property specifies an array of strings, each specifying a mandatory OS feature.

So you want to ignore this stuff, but these are part of the minimum runtime requirements or mandatory OS features. Depending on what you're asking, you absolutely should or should not ignore this field...

When a runtime encounters a set of platform-specific images, it needs to ensure that it can actually run that thing, so it definitely shouldn't ignore the variant or os.features.

When a generic tool encounters a set of platform-specific images, perhaps it doesn't actually care about the variant?

What are you trying to do? It might be easier to support your specific goal than to design a generic solution.

if we can agree on the right interface

Yeah this will be fun. We're going from equality, which is symmetric, to $NEW_TERM, which is asymmetric. Conveying that via names is really important. In the past, I've implemented this two ways:

a.can_run(b) ensures that b is a subset of a.

b.compatible_with(a) == a.can_run(b).

It's obvious how to apply the symmetric thing, but I don't think a single option will actually work here because we could match two ways:

matches = [desc.platform.can_run(p) for desc in descriptors]
matches = [p.can_run(desc.platform) for desc in descriptors]

Your match.Any suggestion is kind of interesting, because we completely sidestep the spec and just let you do whatever you want, but an equivalent for []string would need two options, I think, match.Superset and match.Subset.

If we start going in that direction, you'd probably want something similar for map[string]string to handle annotations.


Re: your suggestions, I think this one is the most interesting:

match.IgnoreBlank(match.Platforms(platforms ...v1.Platform))

Because IgnoreBlank would be:

func IgnoreBlank(Matcher) Matcher

So we could hide a lot of super complex logic in there and apply it to any matcher... but maybe IgnoreBlank isn't right and we need Superset and Subset (or better names if you can think of them). The semantics I'm imagining:

// Superset modifies the matching behavior of a given Matcher:
// For structs, a descriptor will match if the given struct has at least all the set fields.
// For arrays, a descriptor will match if the given array has at least all the entries.
// For maps, a descriptor will match if the given map has at least all the k/v entries.
func Superset(Matcher) Matcher 

// Subset behaves the same way as Superset, but s/at least/at most/g
func Subset(Matcher) Matcher 

So we'd need to somehow pass these down through to the leaf matchers (if we end up with composition via match.And etc) but that seems super doable. We may need to change the signature of Matcher to MatcherFunc and have a Matcher be some interface to make this work. We also probably couldn't do this for a hand-rolled Matcher, only built-ins... 🤔

Also I think match.Subset(match.Superset(m)) would cancel out and be equivalent to m, which is neat.

Anyway, lots of food for thought. Not sure what we actually want to do here, but let me know what you think.

@deitch
Copy link
Collaborator Author

deitch commented Dec 31, 2020

When a runtime encounters a set of platform-specific images, it needs to ensure that it can actually run that thing, so it definitely shouldn't ignore the variant or os.features.

When a generic tool encounters a set of platform-specific images, perhaps it doesn't actually care about the variant?

Definitely agree. Which is why I suggest that this should be left up to the user. By default, match.Platforms matches precisely what is provided. Are there other real-world options? Sure. But let the user pick those.

What are you trying to do? It might be easier to support your specific goal than to design a generic solution.

Pretty much that. Stumbled across this when looking at images of type linux/arm64. The default for many such images on the hub is to put the variant in, but sometimes the consuming platform just doesn't care. See, e.g. docker.io/library/alpine:3.11, which has the index for linux/arm64 as linux/arm64/v8, even though most consuming platforms are just linux/arm64. Actually, I don't even know if runtime.GOARCH has support for those variants.

Either way, it is perfectly valid for a consumer to say, "I want the image for linux/arm64, and I have no clue about variants. Once it matches linux/arm64 (or darwin/arm64, now that it exists), it is good enough for me."

FWIW, for now, I just wrote a custom match.Matcher, so I am hardly blocked, but I would like to help do the right thing for this lib.

a.can_run(b) ensures that b is a subset of a.

b.compatible_with(a) == a.can_run(b).

Back in my days of browser stuff, there was a noticeable shift at some point from listing what browser versions one had and checking for it in a piece of JS compat list, to just seeing capabilities, "can this do X?"

In theory, it is nice. In practice, I am hesitant to use that here, because I don't think we want to get into the business of deciding compatibility. Certain code might be ok on any variant of the same arch; others might be on some variants but not all. I would prefer to kick that up a level and let whoever is consuming the library decide.

Your match.Any suggestion is kind of interesting, because we completely sidestep the spec and just let you do whatever you want, but an equivalent for []string would need two options, I think, match.Superset and match.Subset.

If we start going in that direction, you'd probably want something similar for map[string]string to handle annotations.

Good point. Either way, the big appeal of match.Any is that it is explicit and clear. The matcher will match everything that is a precise match, except where you use match.Any, in which case it will accept anything in that field.

We could extend it slightly with match.AnyString, match.AnyStringSlice, match.AnyMap or similar. That avoids the multiple types issue (string vs []string vs map[string]string), but is a bit messy/verbose.

I do wish we could have match.Any which just matches any value of any type, essentially wildcard. If we could make match.Any a function, such that the filter could call it, and it could intelligently know the type, that would make it easier.

The problem is that v1.Platform expects a string or []string in various places, not a func.

I think this one is the most interesting:

match.IgnoreBlank(match.Platforms(platforms ...v1.Platform))

Essentially we are composing our match.Matcher. It definitely has some possibilities.

I am not sure I get your point about superset vs subset.

  • superset: if the matcher value has 3 fields set, and the rest defaults, then any passed value that has those 3 fields set and matched passes, no matter what the other fields are. Equivalent of match.Any on all default fields.
  • subset: how is this different than the default behaviour?

The other challenge is how to implement this? Taking a returned match.Matcher func and know how to modify it on the fly is messy. I am not sure how.

@jonjohnsonjr
Copy link
Collaborator

Actually, I don't even know if runtime.GOARCH has support for those variants.

I somewhat recently learned about GOARM: ko-build/ko#239

FWIW, for now, I just wrote a custom match.Matcher, so I am hardly blocked, but I would like to help do the right thing for this lib.

Definitely. I like that match is flexible enough to allow this, but I think the default behavior of match.Platforms might be surprising, so we should probably do something.

I don't think we want to get into the business of deciding compatibility

I get what you're saying, but I think we can rely on the spec here, usually... but that is only if someone is asking the question "can A run B?", which often isn't relevant, e.g. for your use case.

The problem is that v1.Platform expects a string or []string in various places, not a func.

I'm not sure how to work around this :/

subset: how is this different than the default behaviour?

Equals is "all fields must match".

Superset is "these values I care about here must match, anything else is gravy".

Subset is "these values I care about here are the only things allowed, anything else is forbidden".

Subset makes more sense thinking about a []string. If I know my capabilities, I want to match that against my requirements:

capability := []string{"foo", "bar"}

laxRequirement := []string{"bar"}
stringentRequirement := []string{"foo", "bar", "baz"}
exactRequirement := []string{"foo", "bar"}

// true
Subset(capability).Match(laxRequirement)

// false
Subset(capability).Match(stringentRequirement)

// true
Subset(capability).Match(exactRequirement)

Sometimes you know your requirements and want to match against a list of capabilities, in which case Superset makes more sense.

The other challenge is how to implement this? Taking a returned match.Matcher func and know how to modify it on the fly is messy. I am not sure how.

Agreed, maybe too messy.

@deitch
Copy link
Collaborator Author

deitch commented Jan 6, 2021

Definitely. I like that match is flexible enough to allow this, but I think the default behavior of match.Platforms might be surprising, so we should probably do something

Yes. Looks like we did a good flexible job with match.Matcher (glad the effort brought a good result), but leaving match.Platforms as is does violate the principle of least surprise.

Superset is "these values I care about here must match, anything else is gravy".
Subset is "these values I care about here are the only things allowed, anything else is forbidden".
Subset makes more sense thinking about a []string. If I know my capabilities, I want to match that against my requirements:

So Superset is "match at least all of these", and Subset is "match at least some of these"?

  • Exact - accepts only if all value fields match comparator fields precisely, no more, no less
  • Superset - accepts if all value fields match comparator fields precisely, but also if some value fields exist that are undefined in comparator fields
  • Subset - accepts if all value fields match comparator fields precisely, but also if some value fields are blank that are defined in comparator fields

Still somewhat confusing, but ok.

Either way, how do we implement that?

@jonjohnsonjr
Copy link
Collaborator

Still somewhat confusing, but ok.

It's basically just the complement of Superset so that you can flip the lhs and rhs. When thinking about platforms:

func (runtime Platform) CanRun(requirement Platform) {
  return runtime.Superset(requirement)
}

func (requirement Platform) CanBeRunBy(runtime Platform) {
  return requirement.Subset(runtime)
}
Subset == Equal || !Superset

Superset == Equal || !Subset

Either way, how do we implement that?

Maybe we just don't? In #907 (comment) there's a need to expose the platform matching logic somewhere. That would make it slightly easier to write a custom matcher to do what you need.

If we did expose those two methods on Platform (not in terms of {super,sub}set, just in terms of platform fields), your custom matcher could just look like:

func matchPlatforms(platforms ...v1.Platform) match.Matcher {
	return func(desc v1.Descriptor) bool {
		for _, platform := range platforms {
			if desc.Platform.CanRun(platform) {
                                return true
                        }
		}
		return false
	}
}

@deitch
Copy link
Collaborator Author

deitch commented Jan 20, 2021

That pretty much is what I did, but instead of CanRun, I just did something that compared the fields I cared about and ignore the rest, the effective equivalent of Any for the other fields (if Any existed).

So will we do anything with this?

@github-actions
Copy link

This issue is stale because it has been open for 90 days with no
activity. It will automatically close after 30 more days of
inactivity. Reopen the issue with /reopen. Mark the issue as
fresh by adding the comment /remove-lifecycle stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants