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.
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
RFC: Support Dockerfiles #173
RFC: Support Dockerfiles #173
Changes from 14 commits
4f73fe4
ef2b230
e21b2af
a1810ca
82cbe67
a9daed9
3534dc2
05dc2d3
f3e84c6
1e813b9
ba9ebd7
58ed531
ebf6b2a
22f0ef4
084822e
b3b38ea
6eed4b2
fde7b84
f643f8b
3a37d54
f513c24
884eaef
cd9471f
a2c06a5
2935c8a
01786f8
10a02f2
2c7bb58
2c6ca72
aa74ace
10b6c9f
1cc2789
a2282f1
9ba0bc2
9533642
bfc5781
68773ac
fefd338
74767a0
3b73177
4abb253
f8e7bfa
3a26570
73de183
9325a13
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It's unfortunate that this proposal introduces a new concept (hooks) that a builder-image maintainer needs to learn; especially when this and #172 begin to simplify and remove some concepts. I also
workworry that the hook concept is not very intuitive.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.
This is subjective, can you qualify this statement with some details or an example?
I would argue that you find the concept of a "hook" (or pre/post script, before/after, wrapper, etc..) in tons of places in the computer world. The general concept is something many sys-admins and dev's have probably already seen.
I agree it adds some complexity because to use a hook, you need to understand the expected output format & what information you have available to generate your output (as well as basic scripting). At the same time, it's optional. You don't need any hooks to generate a builder, so a builder-builder doesn't really need to know this unless they are trying to do more customizations, which you could say is a more advanced task.
My $0.02 only, but it seems like the ratio of complexity added versus additional functionality for users is quite reasonable.
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.
Here is what I believe makes it not intuitive:
These are not very difficult things to learn, but it's yet another thing to learn (which is a shame given that we're looking to remove things like the stack and mixin to make buildpacks easier to grok).
By comparison to your examples, a pre/post script usually stems from the same construct as the thing it's wrapping around (another executable). Similarly, the pre/post proposal for project.toml adds buildpacks before/after buildpacks. These things are all homogenous. But here, we have a concept that introduces a new mechanism into the existing "order". So now we have heterogeneous things running in some order. I think that's what makes it difficult to reason about.
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.
OK, so there seem to be some specifics in the RFC that you think need to be clarified (I agree with some of these btw). IMHO, that seems like something that can be addressed and not a fundamental flaw in the way this is working. Is that your thought as well?
I will have to respectfully disagree on the "hook" as a concept though. There's is a fairly basic lifecycle that buildpacks flow through, you're talking about being able to inject some actions at a couple of points in that lifecycle. This happens all the time in software, they even created AOP to formalize the concept. It happens in software systems as well, the venerable HTTP Servlet Filter in the Java world is an example. As long as we document and have a picture of the lifecycle & indicate where the hooks execute (which is why I agree we need to clarify that point), then we should be fine.
I also strongly believe the complexity we are adding is more than worth it with the functionality this is adding & the number of problems this can solve.
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 don't consider this a substitute for a tight set of constructs where people don't need to read and learn about them for each thing they need to do.
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.
Sure. No disagreement there. Learning nothing is simpler than having to learn something, even if it's simple.
What I'm left trying to understand is this. If your bullet list of concerns were addressed then would the complexity that this RFC adds be OK? or is it your opinion that this RFC is fundamentally too complex and not something we should add?
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.
No, I'm generally in favor of this RFC. I'm only trying to push us to find an abstraction over the hook mechanism that makes it's easier for users to understand (especially buildpack authors and builder-image owners).
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'm also in favour of this RFC. Nevertheless, It is still very hard (as reported too by Joe) to understand different very important points such as :
Remark: That should be great to have a simple text workflow explaining how hooks will be integrated within the existing buildpacks phases (e.g: DETECTION --> ANALYSIS --> BUILD --> EXPORT ==> DETECTION --> ANALYSIS --> HOOK --> BUILD --> EXPORT)
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.
These are covered in the RFC. Can you be more specific about what details you feel are missing?
Do you mean stored as in cached? Caching strategy would be up to the platform to decide.
The Dockerfile would run as root on the base image and produce a new image. No other changes to file permissions would occur.
As specified, buildpacks could create Dockerfiles in their ARGV[1] directory. They could copy them from the app directory, as described in the example, but that's not built-in to the API.
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 you mean hooks?
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.
Does this weaken the security stance of buildpacks? (My opinion: yes) You can literally do anything in a Dockerfile.
I feel like stack packs, while maybe not the answer, did put more controls around what you could do. The stack packs were buildpacks, so presumably (IDK exactly as it wasn't implemented), an Operations team could control which stack packs are available to their users, thereby limiting what their users could install or modify to some degree.
There's also a stronger guarantee that a stack pack would produce a legitimate BOM, since Operations teams can control what stack packs are available (and audit them). With this proposal, someone could make a Dockerfile that installs something malicious and set
genpkgs
to be a copy of thetrue
binary or some other no-op binary. The proposal essentially trusts the creator of the Dockerfile to be honest and report what they've installed, which I don't think is merited givn that this proposal would allow any app dev to customize what's install with a Dockerfile.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 don't have any answers here, but this seems like a fairly important point to linger on.
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 concern is pretty well addressed by including the hooks on the builder. If the app-specified Dockerfile hook (above example) is not present, then the developer cannot provide a Dockerfile. And I suppose that trusting hook authors to provide a valid BOM is the same as trusting buildpack authors to provide a valid BOM.
In the case of the app-specified Dockerfile, the hook author could apply some label to the image so that consumers would know to approach the BOM with some suspicion. I wonder if there's value in having that spec'd ('io.buildpacks.extended'?). It's just a thought.
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 like the idea of a label to easily identify images that have had any hooks executed.
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.
Also
/cnb/image/genpkgs
(because it is executed after Dockerfiles have been applied) is helping to trust the BOM.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.
@natalieparellano I don't really understand how this helps. By the time it is invoked, the hook author could have replaced
/cnb/image/genpgks
with a shell script that writes anything to the BOM.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.
@samj1912 and I have recently been discussing having the platform inject
genpgks
. That may help here...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.
How would that work? I don't think a mount would work, since the hook could discover and overwrite it. Since the hook could do anything, I'm not even sure running a program like
genpgks
is safe even if the binary was untouched. I would think thegenpgks
would almost have to be executed in a new container against the extended image to truly be accurate.That said - these hooks are likely authored and for sure approved/distributed by the stack author. Just because
Dockerfile
s are used, doesn't mean the stack distributor has to extend that control to buildpack authors or the app author. They can create a much more simple constraint (like defining anAptfile
to read package names).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 basically what we're proposing - please see https://github.com/buildpacks/rfcs/pull/173/files#r794608075 for the download I got from @samj1912 and @sclevine .