search

eclipse-che / che

Kubernetes based Cloud Development Environments for Enterprise Teams
http://eclipse.org/che
Eclipse Public License 2.0
6.99k stars 1.19k forks source link

Set set provenance:false in workflows with docker/build-push-action (remove 0.9.1 pin in use docker/setup-buildx-action) #21954

Closed SDawley closed 1 year ago

SDawley commented 1 year ago

Is your task related to a problem? Please describe

The 7.60.0 che-dashboard release failed. There had been issues since the Ubuntu image updated to v0.10.0, and setting the action to v0.9.1 fixed the issue.

Describe the solution you'd like

I'm currently updating the release.yaml files for now for the 7.60.0 release, but the other workflows using buildx should be updated for consistency.

Describe alternatives you've considered

Or, set provenance:false to be able to use 0.10?

Additional context

While fixing buildx version, we could also remove s390x and ppc64le in the same PRs (tech debt cleanup chore - https://github.com/eclipse/che/issues/21969).

I would leave adding support for arm64 for a subsequent set of PRs (new feature work - https://github.com/eclipse/che/issues/22007).

nickboldt commented 1 year ago

While fixing buildx version, we could also remove s390x and ppc64le in the same PRs (tech debt cleanup chore - #21969).

I would leave adding support for arm64 for a subsequent set of PRs (new feature work - #22007).

l0rd commented 1 year ago

What about trying to figure out the route cause and fix of the latest buildx failure?

mkuznyetsov commented 1 year ago

One of the fixes would be to set "provenance:false" in the build and push step, as suggested in https://github.com/docker/buildx/issues/1608 (new feature in buildx 10+ https://docs.docker.com/build/attestations/)

nickboldt commented 1 year ago

@mkuznyetsov if you're offering to take this issue and fix the various places we use buildx, please do.

mkuznyetsov commented 1 year ago

I just tried it and there is at least one seeming working solution, is to set 'provenace:false' for buildx image build https://github.com/docker/buildx/issues/1509#issuecomment-1378538197 Alternatively, need to change the manifest creation with approppriate 'buildx imagetools' command

mkuznyetsov commented 1 year ago

Seems to me, that the issue was originally appearing only in eclipse/che-dashboard, due to it's unique workflow, where multiplatform images were first built by buildx, and then combined with docker manifest create command. At some point, buildx action/GH related action started to use provenance which introduced conflict (which was rightlfully suggested to remove as alternative solution for this issue)

I created 2 PR for this issue, either one, or both merged together would resolve the problem. As I personally don't see immediate drawbacks with either approaches, unless there are arguments to have provenance on/not to use buildx imagetools

Now it seems to me that this problem was not occuring in other projects at all, such as che-operator and its pr-check/next workflows, where multiplatform images builds have been working reliably with latest buildx GH action. E.g. the latest run and the run a month ago, if you look at the logs and find the buildx build command, you can see in one case the provenance was on, and in the other off, and jobs still worked fine. Apparently, that's because the GH action has changed over the last several weeks, where provenance was disabled by default for v3 version of the action. At the same time, provenance can still be enabled in the v4 version of action

So as I see it, no matter how the action will work under the hood in the future, for our case with dashboard we need to either disable the provenance, or use buildx imagetools that can work with them (or use both approaches at the same time even), while in the other projects we should be fine with simply unpinning the v0.9.1 version

nickboldt commented 1 year ago

Renamed issue to reflect new plan.

Please make sure that ALL workflows that use buildx are properly patched so we don't get bitten by this again in future when we're looking at multiarch issues like:

For now I guess having dashboard use a different approach to multi-arch container assembly is OK, as we might want that in future if we find that other builds fail on arm64/aarch64 but pass on amd64/x86_64.

nickboldt commented 1 year ago

Note that this issue is also impacted by stuff like https://github.com/redhat-developer/devspaces/pull/898 -- where we will want to use SHAs instead of tags.

nickboldt commented 1 year ago

@mkuznyetsov can you confirm that with the above PRs merged, this issue is complete? Or are there more changes needed to ensure we have:

nickboldt commented 1 year ago

Looks like we definitely missed at least one spot in https://github.com/devfile/developer-images/pull/71

Proof:

image

mkuznyetsov commented 1 year ago

I made another pass of Che projects that we maintain to ensure that v0.9.1 version is unpinned (found at least 1 place it got overlooked).

As for any further work, I suppose setup action doesn't have this variable. And I'm actually not sure if it's possible (and reasonable) to control that every possible instance of 'buildx' usage has to have provenance disabled. As the issue occured in one particular exclusive case with 'che-dashboard' under particular circumstances.

nickboldt commented 1 year ago

one particular exclusive case with 'che-dashboard'

Yes, but also UDI https://github.com/eclipse/che/issues/21954#issuecomment-1481935012

I think we can resolve this.