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

Warns if user is using untrusted builder #720

Merged
merged 2 commits into from
Jun 30, 2020

Conversation

dfreilich
Copy link
Member

  • Adds error message attributing lifecycle failure in untrusted case to untrusted builder

Signed-off-by: David Freilich dfreilich@vmware.com

Summary

This adds a warning if building with an untrusted builder, so the user understands that there may be some issues with it. This also adds an error if lifecycle execute fails, clarifying that it may be because the builder is untrusted.

Output

Before

image

After

Untrusted Builder:
image

With --trust-builder:
image

Documentation

  • Should this change be documented?
    • No

Related

Resolves #711

@dfreilich dfreilich requested a review from a team as a code owner June 30, 2020 21:27
@@ -192,7 +192,11 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error {
}
}

return c.lifecycle.Execute(ctx, lifecycleOpts)
if err := c.lifecycle.Execute(ctx, lifecycleOpts); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

I put the error here, rather than on lifecycle.Analyze or lifecycle.Export, because that would be easiest to test (by substituting a failing lifecycle).

if trustBuilder {
logger.Debugf("Builder %s is trusted", style.Symbol(flags.Builder))
} else {
logger.Warnf("Builder %s is untrusted", style.Symbol(flags.Builder))
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be changed to a Debugf?

Copy link
Member

Choose a reason for hiding this comment

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

👍

Yeah, I think so. I don't think this should be something alarming since we do all the right things to make it none concerning. It's really just something we want them to know (in debug).

Also, might be worth adding a few more sentences about the side effects. eg. "executing sensitive phases separate containers." and/or "using a known lifecycle image to execute sensitive phases."

Copy link
Member Author

Choose a reason for hiding this comment

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

Added, let me know what you think.

@@ -81,3 +81,19 @@ func getMirrors(config config.Config) map[string][]string {
}
return mirrors
}

func isTrustedBuilder(cfg config.Config, builder string) bool {
Copy link
Member Author

Choose a reason for hiding this comment

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

This pattern is followed a number of times in the commands, and should be refactored out in all cases. If that shouldn't be part of this refactor, I'm happy to undo this change and leave it for another PR.

@dfreilich dfreilich requested a review from djoyahoy June 30, 2020 21:30
@codecov
Copy link

codecov bot commented Jun 30, 2020

Codecov Report

Merging #720 into main will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #720      +/-   ##
==========================================
+ Coverage   73.60%   73.66%   +0.06%     
==========================================
  Files          71       71              
  Lines        5003     5014      +11     
==========================================
+ Hits         3682     3693      +11     
  Misses       1004     1004              
  Partials      317      317              
Flag Coverage Δ
#os_linux 76.14% <100.00%> (-<0.01%) ⬇️
#os_macos 72.42% <100.00%> (+0.05%) ⬆️
#os_windows 72.28% <100.00%> (+0.07%) ⬆️
#unit 73.66% <100.00%> (+0.06%) ⬆️

@dfreilich dfreilich force-pushed the fix/711-clarify-untrusted-warning branch from 2a4fd20 to 7c2cacc Compare June 30, 2020 21:43
* Adds error message attributing lifecycle failure in untrusted case to untrusted builder

Signed-off-by: David Freilich <dfreilich@vmware.com>
* Add more clarification around untrusted vs trusted builder

Signed-off-by: David Freilich <dfreilich@vmware.com>
@dfreilich dfreilich force-pushed the fix/711-clarify-untrusted-warning branch from aa7a1d6 to 9ef2932 Compare June 30, 2020 22:29
@dfreilich dfreilich merged commit a244398 into main Jun 30, 2020
@dfreilich dfreilich deleted the fix/711-clarify-untrusted-warning branch June 30, 2020 22:59
@jromero jromero added this to the 0.12.0 milestone Jul 1, 2020
@jromero jromero added the type/enhancement Issue that requests a new feature or improvement. label Jul 1, 2020
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 this pull request may close these issues.

Make it more obvious that a builder is trusted/untrusted
2 participants