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

build cross-platform images for envoy_iptables #166

Merged
merged 4 commits into from Apr 4, 2022

Conversation

charlesdaniels
Copy link
Collaborator

This should address #157

@anderseknert
Copy link
Member

Hey Charles :) And thanks for looking into this. Could you take a look at the build output, as there seems to be some related error there. Also, you'll need to add a sign-off to the commit for the DCO check, i.e.

git commit --amend --signoff
git push --force

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

LGTM one nitpick

@@ -9,9 +9,8 @@ build: image

.PHONY: image
image:
docker build -t $(REPOSITORY):latest -t $(REPOSITORY):$(VERSION) .
docker buildx build --platform=linux/amd64,linux/arm64 -t $(REPOSITORY):latest -t $(REPOSITORY):$(VERSION) .

.PHONY: push
push: build
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need to depend on build anymore... We'll build the image twice, won't we?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, but since docker buildx build should cache the intermediate results, running it twice is a no-op anyway. I can remove the dep there though, since it should if nothing else save a few seconds.

Signed-off-by: Charles Daniels <charles@styra.com>
Signed-off-by: Charles Daniels <charles@styra.com>
Signed-off-by: Charles Daniels <charles@styra.com>
@charlesdaniels
Copy link
Collaborator Author

I believe I have fixed the envoy_iptables build. It looks like that gatekeeper_mtail_violations_exporter is now failing though. I don't expect that any of the changes in the PR would have affect it. In particular, it looks like it's failing to install goyacc. Possibly an upstream dependency has changed since the last time that container was built?

@charlesdaniels
Copy link
Collaborator Author

Possibly, gatekeeper_mtail_violations_exporter needs to be updated to use go install rather than go get?

go install, with or without a version suffix (as described above), is now the recommended way to build and install packages in module mode. go get should be used with the -d flag to adjust the current module's dependencies without building packages, and use of go get to build and install packages is deprecated. In a future release, the -d flag will always be enabled. (source).

@charlesdaniels
Copy link
Collaborator Author

It looks like that simply updating the version of mtail fixes the build:

diff --git a/gatekeeper_mtail_violations_exporter/Dockerfile b/gatekeeper_mtail_violations_exporter/Dockerfile
index 7c8fe0d..4f8ded7 100644
--- a/gatekeeper_mtail_violations_exporter/Dockerfile
+++ b/gatekeeper_mtail_violations_exporter/Dockerfile
@@ -7,7 +7,7 @@ WORKDIR /go/src/github.com/google/mtail
 # build the mtail binary using the rc36 release
 RUN apk add --update --no-cache --virtual build-dependencies git make \
  && git clone https://github.com/google/mtail /go/src/github.com/google/mtail \
- && git checkout v3.0.0-rc36 \
+ && git checkout v3.0.0-rc48  \
  && make depclean && make install_deps \
  && PREFIX=/go make STATIC=y -B install

@@ -16,4 +16,4 @@ FROM scratch
 COPY --from=builder /go/bin/mtail /usr/bin/mtail
 ENTRYPOINT ["/usr/bin/mtail"]
 EXPOSE 3903
-WORKDIR /tmp
\ No newline at end of file
+WORKDIR /tmp

However, I don't know what differences there may be between rc36 and rc48, and indeed I'm not really sure what exactly gatekeeper_mtail_violations_exporter does, so I don't want to make this patch myself.

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Thanks again

@charlesdaniels
Copy link
Collaborator Author

Looks like I can't merge this since I don't have write access to the repository; feel free to merge if you are satisfied with these changes (or shoot me an invite).

Signed-off-by: Charles Daniels <charles@styra.com>
@anderseknert anderseknert merged commit 5b92f91 into open-policy-agent:main Apr 4, 2022
@anderseknert
Copy link
Member

Thanks Charles! Sounds good wrt the other fixes you suggested for the repo. I'll check about write access 👍

charlesdaniels added a commit to charlesdaniels/contrib that referenced this pull request Apr 4, 2022
Because the build for gatekeeper_mtail_violations_exporter is broken,
disable it for the time being.

Potential fix: open-policy-agent#166 (comment)

Signed-off-by: Charles Daniels <charles@styra.com>
anderseknert pushed a commit that referenced this pull request Apr 4, 2022
Because the build for gatekeeper_mtail_violations_exporter is broken,
disable it for the time being.

Potential fix: #166 (comment)

Signed-off-by: Charles Daniels <charles@styra.com>
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.

None yet

3 participants