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

Enable linux arm builds for OPA envoy plugin #577

Conversation

tjons
Copy link
Contributor

@tjons tjons commented Aug 4, 2024

This PR resolves open-policy-agent/opa#4965 by adding ARM builds to the CI process. make deploy-ci will now build binaries, start a docker builder for cross-platform builds and push manifests for static and dynamic multiarch builds.

You can see the results here in my docker hub repository: https://hub.docker.com/repository/docker/tylerschade268/opa/tags.

I'm no expert on build tooling so all suggestions are welcome. I consider this a first draft rather than a "please merge immediately" PR.

This Makefile is definitely a little messy and I question whether we need all of these targets as some of them don't seem necessary anymore. I also question if we need different tags for Istio and Envoy as the images are identical. Regardless, I'd like to leave both of those questions for another day.

@tjons tjons force-pushed the tjons/4965-opa-envoy-plugin-arm64 branch from 3fb6fba to 3b3a347 Compare August 4, 2024 15:01
@tjons tjons changed the title Enable arm builds for OPA envoy plugin Enable linux arm builds for OPA envoy plugin Aug 4, 2024
@kenissongmelo
Copy link

Great job, if there's anything else I can help with.

@tjons tjons force-pushed the tjons/4965-opa-envoy-plugin-arm64 branch from 3b3a347 to 658ed19 Compare August 4, 2024 15:18
@ashutosh-narkar
Copy link
Member

Thanks for working on this much requested feature. I'll review it this week.

@ashutosh-narkar ashutosh-narkar force-pushed the tjons/4965-opa-envoy-plugin-arm64 branch from 658ed19 to ab360ef Compare August 16, 2024 19:14
Copy link
Member

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tjons overall this looks good. One high-level comment is to keep this similar to OPAs build process and build only static images for arm64.

Dockerfile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated
Comment on lines 115 to 125
.PHONY: image-quick-%
image-quick-%:
$(DOCKER) build --platform=linux/$(GOARCH) -t $(IMAGE):$(VERSION) --build-arg BASE=chainguard/glibc-dynamic -f Dockerfile .

.PHONY: image-quick-static
image-quick-static:
$(MAKE) image-quick-static-$(GOARCH)

.PHONY: image-quick-static-%
image-quick-static-%:
$(DOCKER) build --platform=linux/$* --push -t $(IMAGE):$(VERSION)-static --build-arg BASE=chainguard/static:latest -f Dockerfile .
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should build only static images for arm64. It would be a good idea to structure this similar to OPA's Makefile.

Makefile Outdated
$(DOCKER) buildx build --platform=$(DOCKER_PLATFORMS) \
--push -t $(IMAGE):$(VERSION_ISTIO) \
--build-arg BASE=chainguard/glibc-dynamic:latest \
-f Dockerfile .
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This too would be good if we base this off OPA's Makefile and only static for arm64.

@ashutosh-narkar
Copy link
Member

@tjons if possible it would be great to get this change in for the next release of the plugin which will come out end of next week. Thanks!

@tjons
Copy link
Contributor Author

tjons commented Aug 24, 2024

I will get it done this weekend! Sorry for the wait... been busy. Feel free to make any changes you want too, maintainers are allowed to push to my fork. I should have some good time tomorrow, very excited to see this release!

@tjons tjons force-pushed the tjons/4965-opa-envoy-plugin-arm64 branch 2 times, most recently from b7daf22 to 5ee1223 Compare August 26, 2024 02:27
@tjons
Copy link
Contributor Author

tjons commented Aug 26, 2024

@ashutosh-narkar ready for another look! I think this looks a lot more like OPA's Makefile now, although with the caveat that we are building arm/amd images in the same build step here. In OPA itself, we build those in two different github actions. I added a small workaround for that (adding a VARIANT, e.g., static or dynamic, to the image name), so let me know if you want to follow OPA's github actions here too and I can try to get that done sometime this week.

@tjons tjons force-pushed the tjons/4965-opa-envoy-plugin-arm64 branch from 5ee1223 to 985cfea Compare August 26, 2024 02:53
@ashutosh-narkar
Copy link
Member

@tjons this seems fine. Have you tested the workflow and generated any sample artifacts?

@tjons
Copy link
Contributor Author

tjons commented Aug 29, 2024

@ashutosh-narkar I've tested the make commands on my linux box but not the full GHA workflow, as there should be no changes there. You can see the results here: https://hub.docker.com/repository/docker/tylerschade268/opa/tags

@tjons tjons force-pushed the tjons/4965-opa-envoy-plugin-arm64 branch from 985cfea to 69c13ef Compare August 29, 2024 00:36
@ashutosh-narkar
Copy link
Member

You can see the results here: https://hub.docker.com/repository/docker/tylerschade268/opa/tags

Is this from the latest changes? I would have expected to see linux/amd64 and linux/arm64 for static builds only.

@tjons tjons force-pushed the tjons/4965-opa-envoy-plugin-arm64 branch from 69c13ef to 89bf684 Compare August 29, 2024 01:14
@tjons
Copy link
Contributor Author

tjons commented Aug 29, 2024

You can see the results here: https://hub.docker.com/repository/docker/tylerschade268/opa/tags

Is this from the latest changes? I would have expected to see linux/amd64 and linux/arm64 for static builds only.

Good catch - they were missing the -static suffix for static images... not sure how I dropped that! Take a look at the latest :buildtest* tags

@tjons tjons force-pushed the tjons/4965-opa-envoy-plugin-arm64 branch 2 times, most recently from a7cbe1f to 4cf2358 Compare August 29, 2024 01:21
@ashutosh-narkar
Copy link
Member

You can see the results here: https://hub.docker.com/repository/docker/tylerschade268/opa/tags

Is this from the latest changes? I would have expected to see linux/amd64 and linux/arm64 for static builds only.

Good catch - they were missing the -static suffix for static images... not sure how I dropped that! Take a look at the latest :buildtest* tags

Looks good. We'll also need to update the Post Tag workflow to generate arm64 (static) binaries for darwin and linux. We'll need to update this Make target.

@ashutosh-narkar
Copy link
Member

I may have missed it but looking at your changes you may have already handled generating arm64 binaries.

@tjons tjons force-pushed the tjons/4965-opa-envoy-plugin-arm64 branch from 4cf2358 to 8d9b9fd Compare August 29, 2024 16:00
@tjons
Copy link
Contributor Author

tjons commented Aug 29, 2024

I may have missed it but looking at your changes you may have already handled generating arm64 binaries.

Thanks - added that now!

Copy link
Member

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the contribution @tjons 👏

@ashutosh-narkar ashutosh-narkar force-pushed the tjons/4965-opa-envoy-plugin-arm64 branch from 8d9b9fd to 9aebc87 Compare August 29, 2024 19:09
@ashutosh-narkar ashutosh-narkar merged commit f428496 into open-policy-agent:main Aug 29, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support opa-envoy linux/arm64 Docker Image
3 participants