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

Validate mixins during create-builder and build #179

Closed
ekcasey opened this issue May 9, 2019 · 14 comments · Fixed by #374
Closed

Validate mixins during create-builder and build #179

ekcasey opened this issue May 9, 2019 · 14 comments · Fixed by #374
Labels
type/enhancement Issue that requests a new feature or improvement.
Milestone

Comments

@ekcasey
Copy link
Member

ekcasey commented May 9, 2019

create-builder

Adds buildpack mixins to builder layers label

Given I have a buildpack.toml with the following section:

[[stacks]]
  id = "some.stack.id"
  mixins = ["mixinA", "mixinB"]

When I run pack create-builder against the buildpack and stack some.stack.id
Then The builder contains the buildpack mixins in its io.buildpacks.buildpack.layers label:

{
  "some.buildpack.id": {
    "some.buildpack.version": {
      "stacks": [{
        "id": "some.stack.id",
        "mixins": ["mixinA", "mixinB"]
      }],
      ...
    }
  },
  ...
}

NOTE: Order buildpacks will not have mixins (as they have no stacks), so they can be skipped over


Validates buildpack mixins against build image mixins

Mixins satisfied

Given I have a build image with a io.buildpacks.stack.mixins label:

[ "mixinA", "build:mixinB", "mixinC" ]

And I have a buildpack that requires mixins: mixinA, build:mixinB, run:mixinD
When I run pack create-builder against the build image and buildpack
Then the command succeeds

NOTE: run:-prefixed mixins do not need to be validated during create-builder (e.g. run:mixinD above).


Mixins unsatisfied

Given I have a build image some/build with a io.buildpacks.stack.mixins label:

[ "mixinA", "build:mixinB", "mixinC" ]

And I have a buildpack that requires mixins: mixinX, mixinY, build:mixinB, run:mixinD
When I run pack create-builder against the build image and buildpack
Then the command fails with error similar to:

Error: invalid build image 'some/build': buildpack 'some.buildpack.id@some.buildpack.version' requires missing mixin(s): mixinX, mixinY

NOTE: run:-prefixed mixins do not need to be validated during create-builder (e.g. run:mixinD above).

build

Validates stack image mixins against each other

Mixins match

Given I have a builder image with a io.buildpacks.stack.mixins label:

["mixinA", "build:mixinB"]

And the builder's corresponding run image has a io.buildpacks.stack.mixins label:

["mixinA", "run:mixinC"]

When I run pack build against the builder
Then the command succeeds


Missing one or more common mixins

Given I have a builder image with a io.buildpacks.stack.mixins label:

[ ]

And the builder's corresponding run image has a io.buildpacks.stack.mixins label:

["mixinA", "mixinB"]

When I run pack build against the builder
Then the command fails with error similar to:

Error: invalid builder image: 'some/builder' requires missing mixin(s): mixinA, mixinB

NOTE: Similar behavior expected for common mixin missing from run image


Invalid mixin

Given I have a builder image with a io.buildpacks.stack.mixins label:

[ "run:mixinA", "run:mixinB" ]

When I run pack build against the builder
Then the command fails with error similar to:

Error: invalid builder image: 'some/builder' contains run-only mixin(s): run:mixinA, run:mixinB

NOTE: Similar behavior expected for build-only mixin present in run image


Validates buildpack mixins against stack image mixins

Mixins satisfied

Given I have a builder image with a io.buildpacks.stack.mixins label:

[ "mixinA", "build:mixinB", "mixinC" ]

And the builder's corresponding run image has a io.buildpacks.stack.mixins label:

[ "mixinA", "mixinC", "run:mixinD" ]

And I have a buildpack that requires mixins: mixinA, build:mixinB, run:mixinD
When I run pack build against the builder image and buildpack
Then the command succeeds

NOTE:

  • both build:-prefixed and run:-prefixed must be satisfied
  • must behave similarly with --buildpack and/or --run-image flags

Mixins unsatisfied

Given I have a builder image some/builder with a io.buildpacks.stack.mixins label:

[ "mixinA", "build:mixinB" ]

And the builder's corresponding run image some/run has a io.buildpacks.stack.mixins label:

[ "mixinA" ]

And I have a buildpack that requires mixins: mixinA, build:mixinB, mixinC, run:mixinD
When I run pack build against the builder image and buildpack
Then the command fails with error similar to:

Error: invalid stack image(s): buildpack 'some.buildpack.id@some.buildpack.version' requires missing mixin(s): mixinC, run:mixinD

NOTE:

  • both build:-prefixed and run:-prefixed must be satisfied
  • must behave similarly with --buildpack and/or --run-image flags
@ekcasey ekcasey added the type/enhancement Issue that requests a new feature or improvement. label May 9, 2019
@sclevine
Copy link
Member

Note, needs to be updated for stage-specific mixins: run:some-name and build:some-name
These allow you to specify a mixin that doesn't need to be specified on both the run and build stack images.

@ameyer-pivotal
Copy link
Contributor

Current WIP on branch wip-validate-mixins

@ameyer-pivotal ameyer-pivotal added this to the 0.5.0 milestone Sep 27, 2019
@ekcasey ekcasey removed this from the 0.5.0 milestone Oct 14, 2019
@ameyer-pivotal ameyer-pivotal added this to the 0.6.0 milestone Oct 14, 2019
@ameyer-pivotal ameyer-pivotal changed the title Validate mixins Validate mixins during create-builder and build Oct 30, 2019
@simonjjones
Copy link
Member

Generated error statement is slightly inconsistent with story acceptance criteria. We have:

ERROR: validating buildpacks: buildpack 'bp.one@1.2.3' requires missing mixin(s): mixinX

we want:

Error: invalid build image 'pack-test/build': buildpack 'bp.one@1.2.3' requires missing mixin(s): mixinX

@simonjjones
Copy link
Member

Should we be validating run image mixins during create-builder? Currently we can successfully create a builder with a run image that is missing required common & run mixins.

@ameyer-pivotal
Copy link
Contributor

The error messaging is OK. The original was written without regards to extra context.

For validating run image mixins, this will be covered by a separate story (https://github.com/orgs/buildpacks/projects/1#card-28467986)

@simonjjones
Copy link
Member

During pack build when validating stack mixins where both a common & run mixin are missing from the run image, we first fail with only the common mixin marked as missing from the stack, then once we add the common mixin and reattempt the build we see a validation error missing the run mixin.

$ pack build acceptance-app --builder acceptance-builder
ERROR: validating stack mixins: pack-test/run missing required mixin(s): mixinX

then, once I add mixinX to the run image:

pack build acceptance-app --builder acceptance-builder
ERROR: validating stack mixins: buildpack bp.one@1.2.3 requires missing mixin(s): run:mixinZ

It occurs that all of this validation could have happened during create-builder, and when using e.g. a local buildpack with a builder the validation phase should catch all validation failures and report back to the user.

Perhaps this is more in line with further improvements that could be spun out into a different issue?

@ameyer-pivotal I see your comment - I guess the trouble with waiting for that story is that we appear to be doing some validation of the run image, during some actions, however, it's a less than ideal experience right now.

@ameyer-pivotal
Copy link
Contributor

ameyer-pivotal commented Dec 17, 2019

@simonjjones I agree that it's less than ideal. The complication is that even if we validate the build/run image mixins at create-builder, it's only valid as long as the stack images don't change between create-builder and build (where we do validate the stack images), which could be an indeterminate amount of time. The best that validating everything at create-builder can do is that it gives a warning, but cannot be a guarantee.

On the other topic of aggregate vs fail fast errors, I agree that this is an improvement that would give a better user experience. It would take some rework, so would opt to put it as part of a separate story. Unless we want to either reject this and hold the release or remove the validation to continue with the release.

@simonjjones
Copy link
Member

When validating the use case that pack build fails when the build image includes run mixins we do see it correctly fail:

$ pack build acceptance-app --builder acceptance-builder
ERROR: validating stack mixins: index.docker.io/library/acceptance-builder:latest contains run-only mixin(s): run:notamixin, run:stillnotworking

An interesting note is the index.docker.io address for the builder - this is a locally built builder, where is this address coming from? Is it correct?

@ameyer-pivotal
Copy link
Contributor

ameyer-pivotal commented Dec 17, 2019

When validating the use case that pack build fails when the build image includes run mixins we do see it correctly fail:

$ pack build acceptance-app --builder acceptance-builder
ERROR: validating stack mixins: index.docker.io/library/acceptance-builder:latest contains run-only mixin(s): run:notamixin, run:stillnotworking

An interesting note is the index.docker.io address for the builder - this is a locally built builder, where is this address coming from? Is it correct?

Interesting. Pack usually interprets it as this when there is no registry specified. However, I do not usually see it printed out. It wouldn't be a difficult fix, but seems minor at the moment. Perhaps we file an issue to track it and continue?

@simonjjones
Copy link
Member

The complication is that even if we validate the build/run image mixins at create-builder, it's only valid as long as the stack images don't change between create-builder and build (where we do validate the stack images)

I agree with regards the validity, but given that we do some validation at create-builder I think we should do all of the relevant validation here, and then do it again during build. Unless there's a valid use case for building an invalid builder that could then be fixed by updating the build or run images at a later point?

@simonjjones
Copy link
Member

It's not clear that the following acceptance criteria is a valid one. If any buildpacks require our builder image to include common mixins, then create-builder correctly fails. If the buildpacks do not require any common mixins then build does not fail at validation. This sounds like correct behavior, it would appear difficult to trigger this any other way.

Missing one or more common mixins
Given I have a builder image with a io.buildpacks.stack.mixins label:

[ ]
And the builder's corresponding run image has a io.buildpacks.stack.mixins label:

["mixinA", "mixinB"]
When I run pack build against the builder
Then the command fails with error similar to:

Error: invalid builder image: 'some/builder' requires missing mixin(s): mixinA, mixinB

NOTE: Similar behavior expected for common mixin missing from run image

@simonjjones
Copy link
Member

Aside from aforementioned suggestions for future improvements we have validated the scenarios detailed in the story.

@ameyer-pivotal
Copy link
Contributor

ameyer-pivotal commented Dec 18, 2019

@simonjjones This may be a slightly outdated AC. A decision was made during the story that the run image could contain a superset of the build image mixins (with regards to the common ones).

So really, the scenario outlined by the NOTE: is the only one that should be enforced, regardless of whether buildpacks require any of the mixins.

Note was:

NOTE: Similar behavior expected for common mixin missing from run image

To summarize:

  • Build image mixins MAY NOT contain all mixins from the run image
  • Run image mixins MUST contain all mixins from the build image
  • An error should surface even if no buildpacks require the mixins

This is the related code, for the curious:

_, missing, _ := stringset.Compare(runImageMixins, buildImageMixins)

if len(missing) > 0 {
	sort.Strings(missing)
	return fmt.Errorf("%s missing required mixin(s): %s", style.Symbol(runImageName), strings.Join(missing, ", "))
}

@sclevine feel free to fact check me on the superset decision

@simonjjones
Copy link
Member

@ameyer-pivotal I believe that this scenario was validated. I've moved this to Ready to Release given that there are no false failures where everything is configured correctly, yet validation fails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants