Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly out of scope for this, but is it worth pulling out this
COPY& adding a mount point to the context dir so that we can install directly from there in the script?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an option I guess, but I'm not sure it buys us much. We're still going to need to parameterise enough to select the right
.debfile. I might be more inclined to pull the deb archive from the release server or S3 bucket inside the dockerfile.But either way, this being a legacy component, I'll probably just leave it as is and let it disappear in 12 months when teleport 14 drops out of support (the last version we'll publish with legacy OCI images).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this removes a layer from the final image, which reduces the image size. Not 100% sure.
I had considered this when I rewrote this file. I think I chose not to do this because:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I think I flaked a little bit working on this. I saw the "FROM ..." at the end and assumed a standard multi-stage build, but at the same time I knew it was only for setting the FIPS entrypoint. But this is leaving the
/tmp/teleport.debfile behind in the image. It would be good not to do this, and that's what the volume mount would give us, perhaps using https://github.com/moby/buildkit/blob/master/frontend/dockerfile/docs/reference.md#run---mountRUN --mount. I'll try that out.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tcsc @fheinecke I've reworked this to use a bind mount for the deb file so it is no longer copied in, "bloating" the image. I also separated the installation of stuff with
apt-getand the installation of the teleport.deb. This will allow caching of theapt-getlayer which I'll make use of in the workflow building OSS and Enterprise (and FIPS) in the same workflow sequentially. Since most is cached, the subsequent builds are quite fast. Thank you both for prompting me on this - this is much better now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I've changed it materially, I would appreciate it if you could give it another look over.