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

Unable to use dynamic build property #620

Closed
wvanderdeijl opened this issue May 2, 2023 · 3 comments · Fixed by #799
Closed

Unable to use dynamic build property #620

wvanderdeijl opened this issue May 2, 2023 · 3 comments · Fixed by #799
Assignees
Labels
kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Milestone

Comments

@wvanderdeijl
Copy link

What happened?

When the build property to the Image resource is dynamic a pulumi preview fails since all command line arguments appear to be missing. This means our custom foo.Dockerfile is not being used and docker tries to use the default Dockerfile resulting in the following error:

Previewing update (skunkteam/dev)

View in Browser (Ctrl+O): https://app.pulumi.com/skunkteam/docker/dev/previews/053b8fb3-47c5-4c1c-956d-070192fa0953

     Type                        Name        Plan       Info
 +   pulumi:pulumi:Stack         docker-dev  create     
 +   ├─ random:index:RandomUuid  guid        create     
     └─ docker:index:Image       image                  1 error


Diagnostics:
  docker:index:Image (image):
    error: could not open dockerfile at relative path Dockerfile: stat Dockerfile: no such file or directory

Expected Behavior

pulumi preview should not fail.

Steps to reproduce

Full example at https://github.com/wvanderdeijl/docker-pulumi-issue

import * as docker from "@pulumi/docker";
import { interpolate, output } from "@pulumi/pulumi";
import * as random from '@pulumi/random';

const apiKey = new random.RandomUuid('guid', {}).result

new docker.Image(
    'image',
    {
        imageName: 'eu.gcr.io/my-project/foo:v1.0.0',
        build: {
            dockerfile: interpolate`./foo.Dockerfile`,
            platform: 'linux/arm64',
            args: {
                KEY: apiKey,
            },
        },
        skipPush: true,
    },
);

Output of pulumi about

CLI
Version 3.65.1
Go Version go1.20.3
Go Compiler gc

Plugins
NAME VERSION
docker 4.2.0
nodejs unknown
random 4.13.0

Host
OS darwin
Version 13.3.1
Arch arm64

This project is written in nodejs: executable='/Users/*****/.nvm/versions/node/v16.17.1/bin/node' version='v16.17.1'

Backend
Name pulumi.com
URL https://app.pulumi.com/*****
User *****
Organizations *****, *****

Dependencies:
NAME VERSION
@pulumi/docker 4.2.0
@pulumi/pulumi 3.65.1
@pulumi/random 4.13.0
@types/node 16.18.25

Pulumi locates its logs in /var/folders/16/6r7yw0gd7593qv1z1zsp6xbh0000gn/T/ by default
warning: Failed to get information about the current stack: No current stack

Additional context

When running a pulumi preview or pulumi up with --logtostderr --logflow -v=9 2> out.txt we can see in the logging that the build property is now a simple output<string> meaning the information for dockerfile (and platform) have been lost:

I0502 15:17:55.251320   89165 log.go:75] Unmarshaling property for RPC[ResourceMonitor.RegisterResource(docker:index/image:Image,image)]: build=output<string>{}
I0502 15:17:55.251336   89165 log.go:75] Unmarshaling property for RPC[ResourceMonitor.RegisterResource(docker:index/image:Image,image)]: imageName={eu.gcr.io/my-project/foo:v1.0.0}
I0502 15:17:55.251339   89165 log.go:75] Unmarshaling property for RPC[ResourceMonitor.RegisterResource(docker:index/image:Image,image)]: skipPush={true}

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@wvanderdeijl wvanderdeijl added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels May 2, 2023
@wvanderdeijl
Copy link
Author

We tried creating a fake Dockerfile that would be used during preview die to this bug. But build.context is also lost when build.args contains a dynamic output due to this bug. This means the current workdir (project root) is used as context which means the hash is calculated over the entire 21GB workspace. And it does that for 15+ docker images in the same pulumi stack resulting in over 1 hour for a pulumi preview.

@AaronFriel AaronFriel removed the needs-triage Needs attention from the triage team label May 2, 2023
@AaronFriel
Copy link
Contributor

Hey @wvanderdeijl thanks for reporting this. I can suggest a workaround to try here:

import * as docker from "@pulumi/docker";
import { interpolate, output, runtime } from "@pulumi/pulumi";
import * as random from '@pulumi/random';

const apiKey = new random.RandomUuid('guid', {}).result

if (!runtime.isDryRun()) {
    new docker.Image(
        'image',
        {
            imageName: 'eu.gcr.io/my-project/foo:v1.0.0',
            build: {
                dockerfile: interpolate`./foo.Dockerfile`,
                platform: 'linux/arm64',
                args: {
                    KEY: apiKey,
                },
            },
            skipPush: true,
        },
    );
}

This will skip evaluating the Docker.Image during previews, ensuring that the apiKey is known during the deployment ("up") phase.

If this works for you, let us know - we may file a follow up issue to track improving preview behavior.

@AaronFriel AaronFriel added the awaiting-feedback Blocked on input from the author label May 3, 2023
@wvanderdeijl
Copy link
Author

That's a great tip. I'm now using that in a slightly different way. Still creating the docker.Image resource during preview but without the dynamic arguments. Otherwise the preview would show that the Image resource would be deleted. I've then added {dependsOn:apiKey} to explicitly define the dependency during preview.

@guineveresaenger guineveresaenger removed the awaiting-feedback Blocked on input from the author label Jun 23, 2023
@mikhailshilkov mikhailshilkov added this to the 0.94 milestone Aug 30, 2023
@guineveresaenger guineveresaenger self-assigned this Sep 12, 2023
@mikhailshilkov mikhailshilkov modified the milestones: 0.94, 0.95 Sep 29, 2023
mikhailshilkov added a commit that referenced this issue Oct 13, 2023
mikhailshilkov added a commit that referenced this issue Oct 20, 2023
A somewhat bizarre fix for #676 

### Problem

Let me try to explain what is going on in #676. The relevant snippet is
(same as in the new test):

```
const image = new docker.Image("my-image", {
    build: {
        context: "app",
        cacheFrom: {
            images: [imageName]
        },
    }
```

Where `imageName` depends on another resource R. In the problematic
scenario, the resource R is being replaced, which turns `imageName` into
unknown. Unfortunately, this turns the entire `build` object into a big
unknown instead of the desired effect of it keeping its shape with only
an unknown value inside `cacheFrom`.

Here is a gRPC log:

```
{"build":"04da6b54-80e4-46f7-96ec-b56ff0331ba9","imageName":"04da6b54-80e4-46f7-96ec-b56ff0331ba9","registry":{"password":"04da6b54-80e4-46f7-96ec-b56ff0331ba9","server":"04da6b54-80e4-46f7-96ec-b56ff0331ba9","username":"04da6b54-80e4-46f7-96ec-b56ff0331ba9"},"skipPush":false}
```

It turns out that `build` is unknown because of the current
implementation of Node.js codegen that sets the parent object as unknown
if that object needs a default value (see
[here](https://github.com/pulumi/pulumi-docker/blob/24161acd0433b061e3b5acfa128b9ddb599b965b/sdk/nodejs/image.ts#L177)).

We then discard unknowns
[here](https://github.com/pulumi/pulumi-docker/blob/24161acd0433b061e3b5acfa128b9ddb599b965b/provider/provider.go#L131)
and proceed with filling default build properties in
[`marshalBuildAndApplyDefaults`](https://github.com/pulumi/pulumi-docker/blob/24161acd0433b061e3b5acfa128b9ddb599b965b/provider/provider.go#L139),
which causes the issue because `Dockerfile` is not available at its
default location.

### Fix

We don't really need to set the default `builderVersion` value in the
schema. We assume the same default value in
[`marshalBuilder`](https://github.com/pulumi/pulumi-docker/blob/24161acd0433b061e3b5acfa128b9ddb599b965b/provider/image.go#L563)
and cover this scenario with a few unit tests
([example](https://github.com/pulumi/pulumi-docker/blob/24161acd0433b061e3b5acfa128b9ddb599b965b/provider/image_test.go#L55)).
Therefore, in this PR I chose to remove the default value from schema
and SDKs and, as a side effect, avoid turning `build` into an unknown.

### Test

I tested it manually and adjusted the ACR test into a two-step test that
exercises the scenario. You can see the test fail before my change
[here](https://github.com/pulumi/pulumi-docker/actions/runs/6510330782/job/17684951125#step:27:166).
Also added another test to validate #620.

Fix #676
Fix #620
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Oct 20, 2023
guineveresaenger added a commit that referenced this issue Dec 5, 2023
This pull request's intent is to allow a user to build their image
during `pulumi preview` by using the new `buildOnPreview` resource
field.

This field is added to the Image schema as a boolean and defaults to
false.

The implementation specifics are as follows, with some things for the
reviewer to weigh in on.
1. As a prerequisite, `supportPreview` is enabled and all implemented
RPD methods should handle Unknowns. This should also help address #847
and #620.
2. `ContainsUnknowns()` checks are added to the marshaler and some of
the `Check()` logic. I wasn't sure if `ContainsUnknowns()` or
`IsComputed()`should be used here; the former contains a check for the
latter.
3. Unit tests verify the new marshaling behavior.
4. When a Dockerfile is Unknown, we do not verify its location during
Preview `Check()`, instead we apply other defaults and carry on. We will
calculate the build hash on the `update` call once Unknowns are
computed.
5. When in preview mode, and buildOnPreview is false, we return all
inputs as-is in Update() and Create().
6. When an attempt is made to build on preview, but there are Unknowns
in the inputs or news, we send an error instructing the user to set
`buildOnPreview` to false.
7. An integration test is added that verifies an image builds on preview
if the `buildOnPreview` flag is set to true.
8. An integration test is added that verifies an image _fails_ to build
on preview if there are Unknown inputs and the `buildOnPreview` flag is
set to true

Fixes #540.

- Handle unknowns in Build object
- Handle unknowns in Check; skip dockerfile location finding.
- Set SupportPreview to true
- Add ContainsUnknowns() checks for build: target, stages, platform,
Dockerfile, Context; and registry: username, password, server
- Add tests for Unknowns, and tweak Unknown checks as a result of a bit
of TDD
- Add logic to imageBuild that allows for buildOnPreview
- Use Command.stdout to test unknowns
- Add a few more Unknown checks in Check()
- Add an integration test for Build On Preview
- Build SDKs

---------

Co-authored-by: Bryce Lampe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants