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

Platform Spec Feedback. #165

Open
BarDweller opened this issue Nov 24, 2020 · 0 comments
Open

Platform Spec Feedback. #165

BarDweller opened this issue Nov 24, 2020 · 0 comments
Labels
api/platform enhancement New feature or request

Comments

@BarDweller
Copy link
Contributor

Platform Spec Comments..

Here's my feedback on the Platform Spec, after using it to build a Platform implementation.

Overview

Ideally, the platform spec supplies the information required by platform authors to create compliant implementations of buildpack platforms.

Scope

Usage of MUST, SHOULD, etc should be limited to requirements of such a buildpack platform implementation, unless otherwise explicitly qualified to explain the alternate context in which they are being used. Usage of alternate contexts should be minimised.

The current spec appears to combine elements of a Stack spec, and a Platform spec into one document. This makes it difficult today to know if a given platform implementation is compliant to the spec, or not, as some usage of MUST appears to encode things beyond the capability of a Platform implementation.

Structure

It would be useful after the Terminology section, to start with a section defining what a Platform implementation does. This would ideally be a more detailed version of the abstract at the top of the document. This section would likely be where it would be documented that a platform executes lifecycle phases by executing their corresponding binaries within the context of a container built from the build image. This execution context should be introduced to explain the responsibilities of the platform implementation when creating the same (eg, the user id to run the container based on the build image as, etc).

Feedback on the rest of the specification, arranged by section...

Stacks

The first section "Stacks" is not really focused on creating a platform implementation at all, but is more an overview of Stacks themselves, which at this stage in the document, have only been introduced in the terminology section. This section repeatedly defines responsibilities for Stack authors, which would be out of scope for a Platform implementation. Eg, taking the first bulleted list, of things a typical stack might specify, a platform implementation will likely only need to know the last item, assuming that The default user and the build and launch environments is trying to say the user the build environment should be run as ? (unclear)

Sections starting Stack authors MUST should be rewritten to say that compliant stacks "SHALL have", eg, "Stacks SHALL have globally unique case-insensitive ID's", "Stacks ID's SHALL only contain the characters ., /, and -". These aspects of a Stack are not enforceable by a platform, as the Stack implementations are beyond the control of the Platform. It would be fair to suggest that a Platform MUST validate that the ID's of stacks it uses are compliant to those rules, but that's not what's written today.

The Build Image / Run Image section attempts to explain the Platform responsibilities, but more clarity is required, as it is unclear what is being asked for some of the existing entries, eg.

The plaform SHOULD ensure that:
The image config's Label field has the label io.buildpacks.stack.maintainer set to the name of the stack maintainer.
The image config's Label field has the label io.buildpacks.stack.homepage set to the homepage of the stack.
The image config's Label field has the label io.buildpacks.stack.distro.name set to the name of the stack's OS distro.
The image config's Label field has the label io.buildpacks.stack.distro.version set to the version of the stack's OS distro.
The image config's Label field has the label io.buildpacks.stack.released set to the release date of the stack.
The image config's Label field has the label io.buildpacks.stack.description set to the description of the stack.
The image config's Label field has the label io.buildpacks.stack.metadata set to additional metadata related to the stack.

(Note: also see question in Data format section below re label names too!)

Realistically this section just comes down to 'The platform SHOULD ensure that that build image has the following labels, with content'. The plaform can't verify that the content of those labels is as requested, as it logically has no way to verify that supplied maintainer value is the name of a stack maintainer, or that the release date is actually correct. If the platform is expected to verify content for the labels, then the specification needs to document the mechanisms required.

The same comment can be applied to the Run Image label SHOULD's. I think the Build/Run image MUST's are also similarly affected, as they look to be asking for confirmation of build/run image metadata rather than defining execution constraints for them.

If the sections relating to Run Image / Build Image are attempting to define the validation of image metadata to be performed by the Platform implementation today, the sections need restating to be clear about what validation is to be performed, given the knowledge that a Platform implementation has, (and critically, does not have).

The Mixins section for Stack also feels like it belongs in an entirely different spec. A quick read through suggests that most of it is relevant to Stack authoring, rather than Platform implementations. The one part that does reference the Platform A platform MAY support any number of mixins for a given stack... fails to define what would be meant by a platform 'supporting' a mixin.

The Compatibility Guarantees section again is mostly focused toward Stack authors, although again, there is a statement that During build, platforms MUST use the same set of mixins for the run image as were used in the build image .. It is undefined and unclear what is meant by a platform 'using' a set of mixins.

Lifecycle Interface

This part is where the most useful part of the platform spec started.. here is the first mention of the lifecycle's execution environment which really needs a section detailing saying what it is, and what expectations there are for it (eg, uid, gid for the container to execute as, required env vars to be set, and a some overview suggesting that various directories will be mounted to the container to satisfy the directory arguments of the various lifecycle binaries.). It should also be clear about the use of env vars to communicate to the lifecycle phase, and the use of the mounted env filesystem <platform>/env for user vars.. much of this today is down in an 'additional guidance' section, when it should be up front here, describing the lifecycle execution environment, before talking about the lifecycle binaries themselves.

The section does detail the global CNB_PLATFORM_API env var, (although it doesn't refer to it as an env var). But it also details constraints on the lifecycle binaries, which are beyond the platforms control.

Operations.

Build.

I think this section could do with a little more text, and possibly reordering to put the creator choice first, with more guidance as to why you would want to invoke the phases separately. As written, the MUST requires each phase to be run in order, so it's unclear why you would ever want to invoke them individually vs using the creator approach, which from a platform perspective is simpler, and guarantees that the MUST is met.

Rebase.

Unclear here what this means to a platform impl, I think it's trying to say that plaform implementations MAY offer a "rebase" activity in addition to the "build" activity, and if the platform chooses to do this, this is what will happen, and this is how a platform can invoke it. But it's unclear if thats a correct interpretation, or if the rebasing is expected to occur automatically somehow at some time ?

Launch.

Feels oddly out of place, and doesn't appear to define or constrain the behavior of a platform implementation ?

Usage.

This was the most useful section in the document, although again it still referred to responsibilities of the lifecycle phases themselves. These references need to be rewritten in the context of a platform implementation, eg.

All lifecycle phases:

  • MUST read CNB_PLATFORM_API from the execution environment and evaluate compatibility before attempting to parse other inputs (see Platform API Compatibility)
  • MUST give command line inputs precedence over other inputs

Is describing the behavior of the lifecycle binaries, which are beyond the control of the platform implementation. If it were to say

All lifecycle phases:

  • SHALL read CNB_PLATFORM_API from the execution environment to evaluate compatibility (see Platform API Compatibility)
  • SHALL give their command line inputs precedence over other inputs

Would make it a little more platform centric, but the definition of 'execution environment' is lacking here. Repeatedly the Usage section refers to a build environment eg, The platform MUST execute detector in the *build environment* but the terminology defines build environment as the containerized environment in which the lifecycle executes buildpacks. but here we're describing the lifecycle execution environment, which is one onion layer outward.. (admittedly it's the same container, but the platform defines the environment for exeucting lifecycle phases, and the lifecycle does whatever it does to execute buildpacks, the build pack execution environment is effectively managed by the lifecycle binary, not by the platform)

The tables of arguments / descriptions, outputs, exit codes are generally good, although I would really prefer to see the creator binary have it's own tables, rather than making me go scroll up and down to find the right sub phase for the argument I want to know about.. The short notes after each table vary in context, but do use SHALL for lifecycle behavior, and MUST etc for constraints related to the arguments that must be met by the platform. (although restorer is using MUST in the context of lifecycle behavior, and very few of the MUSTs of the exporter would appear to be platform responsibilities).

Run Image Resolution

This section is entirely related to plaform spec, it should stay, it would be greatly beneficial to add a few simple inline examples. From a spec perspective, it refers to 'stack metadata' which is not defined in the terminology. Also, see the Query in Data Formats section re which label name for this process.

Registry Authentication

This section is also totally important to a platform impl, and would aslo benefit from an inline example.

Buildpacks

Again, an inline example would solidify the text here, possibly including the arguments passed to an example lifecycle phase (but not the output from) to show the passing of the buildpack dir to the phase etc.

Security Considerations

This section may want to speak also about use of creator vs individual phases, (and should possibly link to the Security Considerations section in the Buildpack Interface Spec, rather than the entire doc itself). It sounds like using creator may expose buildpacks to registry creds?

Additional Guidance

Much of this section should be rolled into the Lifecycle Environment definition section that would come before the documentation for the lifecycle binaries.

Data Formats

Should probably include the authentication json. Much of the content here is really only of use to stack authors, rather than the platform, although the platform does need to be able to parse the buildpack metadata to retrieve the stack/runImage/image.

Label Names

I'm wondering if the label names are correct here.. and back in the 'Stack' section.

I'm currently using io.buildpacks.builder.metadata for my Run Image Resolution, which is what is present on the gcr.io/paketo-buildpacks/builder:base-platform-api-0.3 image, and is the same property consumed by the Spring plugin.

Note that the io.buildpacks.build.metadata example does not contain runImage data. io.buildpacks.lifecycle.metadata does, but it doesn't use mirrors, so isn't a good example for runImage resolution. I'm not even sure what io.buildpacks.lifecycle.metadata is for, it doesn't seem to be referenced by the spec anywhere else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api/platform enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants