search

operator-framework / enhancements

Apache License 2.0
9 stars 40 forks source link

describe a process for resolving image references in manifests #31

Closed stevekuznetsov closed 3 years ago

stevekuznetsov commented 4 years ago

Manifests that make up an Operator Bundle for installation via the Operator Lifecycle Manager refer to one or more container images by their pull specifications: these container images define the operator and operands that the manifests deploy and manage. As with OpenShift release payloads, operator bundles must refer to images by digest in order to produce reproducible installations. A shared process to build operator bundles that replaces image references with fully-resolved pull specifications that pin images by digest must be built; this process must allow for a number of separate build systems to direct how these replacements occur in order to support a full-featured build and test strategy.

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

/cc @gallettilance /assign @jwforres @shawn-hurley

stevekuznetsov commented 4 years ago

I am wondering if there is a place and time for someone to expect to create the bundle image from a docker/podman/buildah et al

I do not see a good reason for someone to want to build their bundle using these methods if the operator-sdk bundle create tooling exists to be the authoritative process.

njhale commented 4 years ago

(closed by mistake)

petr-muller commented 4 years ago

/cc

openshift-ci-robot commented 4 years ago

@petr-muller: GitHub didn't allow me to request PR reviews from the following users: petr-muller.

Note that only operator-framework members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to [this](https://github.com/operator-framework/enhancements/pull/31#issuecomment-645874513): >/cc Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
shawn-hurley commented 4 years ago

I do not see a good reason for someone to want to build their bundle using these methods if the operator-sdk bundle create tooling exists to be the authoritative process.

We can come back and add a command to the SDK or create something new that would just update the manifests. Maybe that is a premature optimization that we don't need to solve for yet.

@joelanford are there risks in vendoring in this openshift library that we will need to create?

I think that I agree with both petr and nick in that the ImageStream API being our resource/config might not be the best for the operator-framework. Could we create a different file, that could easily be translatable to an image stream to still work with the library? Could the library be refactored to use a nonkube API but still look like image streams such that the logic can be re-used and live in operator-framework?

Just to make sure that I am following what will be refactored into a library:

  1. containers/images wrappers that oc uses to create the release payload
  2. replacement logic done in the CI process today for image streams
stevekuznetsov commented 4 years ago

Yes, defining a different on-disk format for the mapping could be fine, but will increase the scope here. We should check with the OpenShift release team to ensure that we capture a large enough API surface for that file so that current functionality does not get lost and we can ensure a high-fidelity translation between an ImageStream and the format that we end up using.

stevekuznetsov commented 4 years ago

We can come back and add a command to the SDK or create something new that would just update the manifests.

@shawn-hurley as noted earlier I think we should explicitly not try to add this as a new command as we should be able to take a strong, authoritative stance that manifests appearing in a bundle must have their image pull specs resolved to a digest. I don't see the utility in allowing anything less strict. By creating a new manifest manipulation command and not making that the default workflow when creating a bundle, all we do is relax that requirement. To what end? Does that satisfy a use-case we have?

stevekuznetsov commented 4 years ago

@shawn-hurley @njhale updated to use a leaner format for the metadata files that are:

  1. co-located with the manifests that declares which images exist and their string placeholders
  2. used to do replacements at build-time
  3. used to hold build-time metadata for downstream consumption
stevekuznetsov commented 4 years ago

Updated to use a list of mapping items instead of a literal map to allow for possible future extensions.

shawn-hurley commented 4 years ago

Can we add where both the image-references file should be given to bundle create or where it should be stored in the bundle on disk?

I imagine references should be in /metadata/, and it will be replaced in the actual image layer with the images file? Is there a reason to keep both or all three? If we end up having all three files (image-references/image-replacements/images) would a known directory like metadata/images/{references/replacements/images} be worth it? WDYT?

shawn-hurley commented 4 years ago

@dirgim @scoheb @lcarva

stevekuznetsov commented 4 years ago

Can we add where both the image-references file should be given to bundle create or where it should be stored in the bundle on disk?

The proposal currently has it under metadata/ @shawn-hurley

Is there a reason to keep both or all three?

No. When the bundle is built, the only information necessary is the one output images file. @shawn-hurley

or split into two proposals, one in OpenShift and one here

@estroz I don't understand what the split is that you're talking about -- the OpenShift side of this has been implemented and is in use for creating release payloads today

Having mandatory extra files to pass in/manually modify is not ideal and would raise the barrier to entry to bundle builds via confusion. IMO this feature needs to be opt-in, or there needs to be smart tooling to generate some default image reference file.

I think the largest disagreement here is that bundle images are much less useful without SHA-pinning. A user who wishes to write a set of manifests and not declare the common names/mappings for the image references inside is simply not fully qualifying their manifests. The bundle creation process for that specific set of manifests may proceed without any image-references but will do no SHA-pinning.

to clarify: are you saying that you want all bundles to include image-references and image-replacements?

@estroz the former, categorically, yes. The latter is provided at build-time. It is critical to being able to create stable, reproducible bundles. There is a broad consensus on this in OpenShift and quite a lot of lessons learned that went into the creation of the release payload design, all of which furthermore apply to bundles.

estroz commented 4 years ago

The bundle creation process for that specific set of manifests may proceed without any image-references but will do no SHA-pinning.

to clarify: are you saying that you want all bundles to include image-references and image-replacements?

@estroz categorically, yes.

@stevekuznetsov are the above two statements not a contradiction?

There is a broad consensus on this in OpenShift and quite a lot of lessons learned that went into the creation of the release payload design, all of which furthermore apply to bundles.

I'm sure this is for valid reasons and I agree with the concept. What I'm saying is that you do not need references to create a runnable bundle and nor should you, since it's just a packaging format; bundles still have utility because they can be stored/pushed/pulled by widespread existing tooling. If references are optional, you should still be able to gate/validate bundles based on whether they pin SHAs or not.

stevekuznetsov commented 4 years ago

are the above two statements not a contradiction?

Maybe this is a matter of semantics. I would personally prefer them to all include references and do SHA-pinning at build time, and I think it will must be required for any consistent or high fidelity build system, but I do not think as part of this proposal we must enforce that all of them do so, if that will fracture a community effort (and as it would make existing manifest directories invalid). I do think, though, that there is very, very limited utility in building output bundles that do not pin.

you do not need references to create a runnable bundle

Sure, agreed in principle.

nor should you

This is where I disagree, because the utility of your bundle is limited if it is not SHA-pinned and reproducible. I do not want to be the engineer tasked with supporting operators that do not pin their operands and run whatever version happens to resolve from a floating tag. Nor do I want to be the engineer that tries to build a test framework that can have high fidelity for such artifacts. Pinning and explicit replacement are something you should do as an author who is trying to produce a quality product and have faith in output artifacts.

stevekuznetsov commented 4 years ago

@estroz I guess one statement I'd like to make is:

If we succeed in making this process simple and build it into the de facto common workflows for building bundles, we enable manifest authors to do the right thing with a trivially small entrance cost. The statements in the proposal re: a canonical build flow are to do with this goal. While I see the argument to maintain backwards compatibility for users who are not opting in today, I do not really see a strong incentive to explicitly want to continue supporting this type of workflow moving forward.

petr-muller commented 4 years ago

Nor do I want to be the engineer that tries to build a test framework that can have high fidelity for such artifacts. Pinning and explicit replacement are something you should do as an author who is trying to produce a quality product and have faith in output artifacts.

I generally agree with what you are saying for production artifacts, but I see some utility in not always SHA-pinning. While I was looking into the testing scenarios on operand repos ("can stable operator operate my candidate operand?" scenario), I discovered it would be useful to have existing bundle with manifests with floating tags and not SHAs: you can then build the operand images and publish them under that floating tag, and use the existing operator + bundle for integration testing.

stevekuznetsov commented 4 years ago

@petr-muller sure, but I think that, as mentioned before, if we want to test "does my operator ..." we should build a bundle, index (?) and use OLM. If you want to test your operand, do so independently. In the OpenShift case, you would use an existing release payload, mark an operator as unmanaged and push your own version, not hot-patch the image version of the operand.

petr-muller commented 4 years ago

@stevekuznetsov I don't follow what you mean by "use an existing release payload, mark an operator as unmanaged and push your version". It's irrelevant for this enhancement though, can we continue in https://issues.redhat.com/browse/DPTP-1268? I'm interested in this.

stevekuznetsov commented 4 years ago

Responded to the last round of review.

stevekuznetsov commented 4 years ago

@shawn-hurley updated to resolve all existing comments

stevekuznetsov commented 3 years ago

/close

openshift-ci[bot] commented 3 years ago

@stevekuznetsov: Closed this PR.

In response to [this](https://github.com/operator-framework/enhancements/pull/31#issuecomment-912695344): >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.