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

Any plans to add providers for writing rules that build on-top of rules_oci? #279

Open
ensonic opened this issue Jun 22, 2023 · 16 comments
Open
Labels
can close? We'll close this issue if we don't get a new comment in 30 days. enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@ensonic
Copy link

ensonic commented Jun 22, 2023

We're trying to port from rules_docker to rules_oci (see googlecloudrobotics/core#130), but have trouble finding a good replacement for:

load("@io_bazel_rules_docker//container:providers.bzl", "ImageInfo", "ImportInfo")
see https://github.com/googlecloudrobotics/core/blob/main/bazel/app_chart.bzl#L1

@thesayyn
Copy link
Collaborator

Usually, our response to "adding something new" to the ruleset is why do you think you need it to accomplish what you are trying to do?

@ensonic
Copy link
Author

ensonic commented Jun 22, 2023

@thesayyn In order to port our bazel rule, we need access to the digest. From https://github.com/bazel-contrib/rules_oci/blob/main/oci/private/image.bzl#L172 it looks like there are no additonal outputs to obtain it.

@thesayyn
Copy link
Collaborator

Is this what would you like to obtain? #60

@alexeagle
Copy link
Collaborator

I think we should add an OutputGroupInfo so you can request the "digest" output file, but we wouldn't have to create any new Provider types to satisfy this FR. Sound right?

@alexeagle alexeagle added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Jul 12, 2023
@ensonic
Copy link
Author

ensonic commented Jul 12, 2023

Is this what would you like to obtain? #60

Thanks! That seems to work for now.

@thesayyn
Copy link
Collaborator

Does #346 fix this for you?

@thesayyn thesayyn added the can close? We'll close this issue if we don't get a new comment in 30 days. label Sep 17, 2023
@gzm0
Copy link
Contributor

gzm0 commented Oct 18, 2023

Maybe I can chime in my experience here: The additional label with the digest file is going to be very hard to compose for downstream rules (need to recalculate a label, so forced to use a macro, not configurable, etc.).

So far, I've just resorted to read the oci-layout index.json directly. This is through a well specified standard. Downsides are:

  • Needs some json processing
  • Needs validation to check the assumption that there is only one manifest in the output (oci-layout allows for more).

@thesayyn
Copy link
Collaborator

  • Needs validation to check the assumption that there is only one manifest in the output (oci-layout allows for more).

oci_* rules will always have a single image in the index.json so that's not really a concern here.

  • Needs some json processing

I believe it could be gathered with a simple sed command given that there's a single digest property.

@gzm0
Copy link
Contributor

gzm0 commented Oct 19, 2023

That is correct, but only under the assumption that users always pass the correct rules forward: Without a provider, all there is to validate at analysis time is that the output is a single tree artifact.

For any more serious ruleset, I would not be comfortable just assuming the build user has wired everything correctly (for comparison: with a provider, I only need to trust the rule implementor).

@gzm0
Copy link
Contributor

gzm0 commented Oct 19, 2023

Another datapoint: Turns out I needed the config digest, not the manifest digest. IIUC with the OIC layout it is infeasible to predict where it is going to end up (since it is content addressable). So to use it, the call-site needs to handle OIC layout anyways.

@thesayyn
Copy link
Collaborator

I don't see the point of adding a provider, just for the sake of validation. Adding a provider is the wrong tool for validation. A custom rule can still produce an invalid oci-layout and add the provider.

And this is separate concern than adding a provider to rules_oci. That'll probably solved via https://bazel.build/extending/rules#validation_actions. We are just waiting to drop bazel 5 support.

@alexeagle
Copy link
Collaborator

We could add a validation action now, and just create it conditional on using Bazel 6. I think this is now a different feature request?

@gzm0
Copy link
Contributor

gzm0 commented Oct 23, 2023

I think this is now a different feature request?

Agreed.

Regarding provider and validation, I think I wasn't very clear. Apologies. I'll try to elaborate.

Consider the following, broken build.

create_dir(
  # some rule that creates an output directory.
  name = "foo",
  ...
)

oci_image(
  # some arbitrary image.
  name = "bar",
  ...
)

my_rule_using_an_image(
  name = "baz",
  image = "foo", # this should be bar
)

The user made a mistake here and passed the wrong label/target to my_rule_using_an_image.

I was looking at this situation from the perspective of the implementer of my_rule_using_an_image. IIUC, the best that can be done to give feedback to the user changes quite a bit with and without a provider:

With a provider

In the analysis phase, bazel itself will not accept the foo label since the target it refers to doesn't have the appropriate provider. The user will get a message to this extent. Hopefully:

  • The user has seen this with other rules/providers.
  • The provider is named reasonably, so it is clear it refers to OCI images.

Without a provider

Analysis phase

my_rule_using_an_image can check that image refers to a single tree artifact. This would not actually lead to an error in this example (assuming create_dir does what it says).

Execution phase

my_rule_using_an_image can check that the contents of the directory claim to be an OCI layout (check oci-layout and its contents) . This is highly likely to fail for any other directory (produced by an arbitrary rule).

While this gives the user appropriate feedback:

  • It happens fairly late in the flow.
  • It requires a lot of effort from my_rule_using_an_image.

Discussion

If we look at the two scenarios above in the context of simply retrieving the manifest digest, in the second scenario (without a provider), the code required for the retrieval will be overshadowed by the code required to prevent users from making mistakes (which IMHO is paramount).

I guess my informal use of the term "validate" has caused some confusion. IIUC to solve the issue I've outlined, a validation action is not useful (or I have misunderstood something about validation actions 🤷 ).

Whether or not all of this is sufficient reason to actually add a provider to rules_oci, and/or whether a provider is the right tool to solve the outlined issue is not 100% clear to me. I just wanted to make sure we're on the same page RE the use cases I had in mind.

@thesayyn
Copy link
Collaborator

Validation actions do what providers would do for "validation" but at runtime. I'd also argue that adding a provider to get bazel to yell users with a message that looks like "target :x does not provide OCIImage provider." is not as useful as one would think.

Adding a provider for this use would simply prevent anyone from using things like genrule, run_binary. Here's an example i made a year later and it still stupidly simple because we don't have to deal with providers. https://github.com/bazel-contrib/rules_oci/pull/570/files#diff-e5546d9d1736e2980c4c365d05d59dcad35851551fa40ded3e24cc1c00049ab4R17

If we added providers for this, it would simply prevented us from doing things like this.

@gzm0
Copy link
Contributor

gzm0 commented May 27, 2024

Here's an example i made a year later and it still stupidly simple because we don't have to deal with providers.

That's an excellent example indeed.

I guess this basically boils down to the nominal / structural subtyping discussion and to some extent the static / dynamic subtyping discussion.

In any case, it feels like at this point, consistency with the bazel ecosystem is significantly more important than doing "the right thing". I do not feel I have a sufficiently large understanding of how the ecosystem handles this to have any opinion TBH. So from my personal POV, feel free to close this issue.

@thesayyn
Copy link
Collaborator

You are correct, but this isn't because we are trying specifically do the right thing thus diverging from the community. You can find plenty of rulesets that don't have a custom provider.

oci_image is in sense is like pkg_tar, you feed it a bunch files and it packages them into a folder (as opposed to a tar file) that is understood by most container tooling.

I guess this basically boils down to the nominal / structural subtyping discussion and to some extent the static / dynamic subtyping discussion.

I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can close? We'll close this issue if we don't get a new comment in 30 days. enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants