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

chore: replace kennethreitz/httpbin with arm64 enabled arnaudlacour/httpbin #2548

Closed

Conversation

pmalek
Copy link
Member

@pmalek pmalek commented Jun 7, 2022

What this PR does / why we need it:

NOTE: This supersedes #2547 in the sense that this replace all the occurrences of kennethreitz/httpbin into arnaudlacour/httpbin not only those in tests.

This PR address the fact that as of now we're using kennethreitz/httpbin which provides only linux/amd64.

Since the github repo that hosts code for that image seems to be unmaintained, and since there is an issue that raises the lack of arm64 support let's use a repo that hosts images for a fork of the original one.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • tested in a kind cluster on an arm64 Mac M1

Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Switching to this fork seems like a net negative for supply chain security.

I want to suggest we consider doing one of the following:

  1. trying to engage with postmanlabs and see if they would accept new PRs to port the ARM64 builds into httpbin (it's been years, so they might not)
  2. do our own kong fork of httpbin so that we're controlling the upstream

Let me know your thoughts?

@pmalek pmalek force-pushed the replace-kennethreitz-httpbin-with-arnaudlacour-httpbin branch from ca07bad to d398270 Compare June 8, 2022 08:41
@pmalek pmalek temporarily deployed to Configure ci June 8, 2022 08:41 Inactive
@pmalek pmalek temporarily deployed to Configure ci June 8, 2022 08:41 Inactive
@pmalek pmalek temporarily deployed to Configure ci June 8, 2022 09:04 Inactive
@jrsmroz
Copy link
Contributor

jrsmroz commented Jun 13, 2022

postmanlabs have not updated httpbin repo since 4 years. There is plenty of issues without any response. The image that we used so far kennethreitz/httpbin was last published 4 years ago. It's base layer is ubuntu:18.04, so it's hardly secure if it is not updated regularly.

@pmalek
Copy link
Member Author

pmalek commented Jun 13, 2022

postmanlabs have not updated httpbin repo since 4 years. There is plenty of issues without any response. The image that we used so far kennethreitz/httpbin was last published 4 years ago. It's base layer is ubuntu:18.04, so it's hardly secure if it is not updated regularly.

That's true but the idea behind this PR was to make the tests run on arm64. The fork that I proposed didn't even have a single commit more than what the original repo has, so there's hardly any change w.r.t functionalities and/or base image etc.

The consensus around this issue was to either change the image used in tests or fork it and make appropriate changes so that it builds for arm64 and publishes the images onto e.g. the docker hub and preferably update the base image as well (as you've mentioned).

Perhaps there was a mishap on my part that I didn't communicate this clearly here.

@shaneutt
Copy link
Contributor

To my understanding we're simply at a point where we want to fork, update, and thereafter maintain this and the go-echo tool and I think that's the right way to go for supply chain security purposes. I would consider this blocked on us completing that, or perhaps we could even consider this one closed and make that change part of a separate effort to do the full fork.

@pmalek pmalek added do not merge let the author merge this, don't merge for them. blocked and removed blocked labels Jun 13, 2022
@pmalek
Copy link
Member Author

pmalek commented Jun 13, 2022

To my understanding we're simply at a point where we want to fork, update, and thereafter maintain this and the go-echo tool and I think that's the right way to go for supply chain security purposes. I would consider this blocked on us completing that, or perhaps we could even consider this one closed and make that change part of a separate effort to do the full fork.

Actually, let's leave this as is so that it we don't drop the ball on this. We can then add a "superseded by #...." comment in the new PR that will introduce the usage of said fork. WDYT?

@shaneutt
Copy link
Contributor

To my understanding we're simply at a point where we want to fork, update, and thereafter maintain this and the go-echo tool and I think that's the right way to go for supply chain security purposes. I would consider this blocked on us completing that, or perhaps we could even consider this one closed and make that change part of a separate effort to do the full fork.

Actually, let's leave this as is so that it we don't drop the ball on this. We can then add a "superseded by #...." comment in the new PR that will introduce the usage of said fork. WDYT?

I'm fine with leaving it open as a reminder, though for review purposes I think we should switch it to draft 👍

@pmalek pmalek marked this pull request as draft June 13, 2022 15:52
@svenwal
Copy link
Member

svenwal commented Jun 21, 2022

You could use also https://hub.docker.com/repository/docker/svenwal/httpbin/general which is under my control (and I am a member of Kong). It runs on ARM and also does not need root privileges (exposed 8080 instead of 80).
Dockerfile is here: https://github.com/svenwal/httpbin/blob/master/Dockerfile

@pmalek
Copy link
Member Author

pmalek commented Jun 21, 2022

Thanks @svenwal but I think @mlavacca is already pretty close to the final result: https://github.com/Kong/httpbin with multi arch images already being pushed to https://hub.docker.com/r/kong/httpbin/tags

@svenwal
Copy link
Member

svenwal commented Jun 21, 2022

Don't miss the chance to also deploy as non-root (like secure K8s and OpenShift env need it)

@pmalek
Copy link
Member Author

pmalek commented Jun 21, 2022

Yup, good idea.

cc: @mlavacca

@mlavacca
Copy link
Member

I agree this can be a good idea. I'm running the test with the svenwal/httpbin:0.9.2 image. If everything works fine, I'll use it.

@pmalek
Copy link
Member Author

pmalek commented Jun 24, 2022

Superseded by #2596

@pmalek pmalek closed this Jun 24, 2022
@pmalek pmalek deleted the replace-kennethreitz-httpbin-with-arnaudlacour-httpbin branch June 24, 2022 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge let the author merge this, don't merge for them.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants