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.

Sign up for GitHub

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

Fix error for helm.Release previews with computed values #1760

Merged
merged 4 commits into from
Oct 8, 2021
Merged

Fix error for helm.Release previews with computed values #1760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aacab02
Fix error for helm.Release previews with computed values
lblackstone Oct 7, 2021
9bc68e4
Update test to use computed value
lblackstone Oct 7, 2021
76fa627
Fix check
lblackstone Oct 7, 2021
808430c
Invert error logic
lblackstone Oct 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
## HEAD (Unreleased)

- Fix error for helm.Release previews with computed values (https://github.com/pulumi/pulumi-kubernetes/pull/1760)

## 3.8.0 (October 6, 2021)

Breaking change note:
Expand Down
48 changes: 26 additions & 22 deletions provider/pkg/provider/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1148,12 +1148,6 @@ func (k *kubeProvider) Check(ctx context.Context, req *pulumirpc.CheckRequest) (
//

urn := resource.URN(req.GetUrn())
if isHelmRelease(urn) {
if !k.clusterUnreachable {
return k.helmReleaseProvider.Check(ctx, req, !k.suppressHelmReleaseBetaWarning)
}
return nil, fmt.Errorf("can't use Helm Release with unreachable cluster. Reason: %q", k.clusterUnreachableReason)
}

// Utilities for determining whether a resource's GVK exists.
gvkExists := func(gvk schema.GroupVersionKind) bool {
Expand Down Expand Up @@ -1218,6 +1212,13 @@ func (k *kubeProvider) Check(ctx context.Context, req *pulumirpc.CheckRequest) (
}
newInputs = annotatedInputs

if isHelmRelease(urn) && !hasComputedValue(newInputs) {
Copy link
Member

Choose a reason for hiding this comment

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

How disruptive is that? Say, we have 9 plain values and 1 computed value - wouldn't we still want to run Check for those 9 plain values? Should we teach helmReleaseProvider to work with computed values instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

The helm release check attempts to unmarshal the arguments, which doesn't work with computed values. The rest of the check after unmarshaling involves talking to the helm client, so it's not possible without resolved values. I can't think of a case where we could do useful checking (specific to Release) with unresolved values, so I think it's fine to only do the checks at the k8s provider layer.

To clarify, all of the normal Check logic still runs at the k8s provider layer, so the preview would look something like this:

+ pulumi:pulumi:Stack: (create)
    [urn=urn:pulumi:dev::pulumi-k8s-test::pulumi:pulumi:Stack::pulumi-k8s-test-dev]
    + random:index/randomPet:RandomPet: (create)
        [urn=urn:pulumi:dev::pulumi-k8s-test::random:index/randomPet:RandomPet::test]
        [provider=urn:pulumi:dev::pulumi-k8s-test::pulumi:providers:random::default_4_2_0::04da6b54-80e4-46f7-96ec-b56ff0331ba9]
        length    : 2
        separator : "-"
    + kubernetes:core/v1:Namespace: (create)
        [urn=urn:pulumi:dev::pulumi-k8s-test::kubernetes:core/v1:Namespace::test]
        [provider=urn:pulumi:dev::pulumi-k8s-test::pulumi:providers:kubernetes::default_3_9_0_alpha_1633622661_9bc68e49_dirty::04da6b54-80e4-46f7-96ec-b56ff0331ba9]
        apiVersion: "v1"
        kind      : "Namespace"
        metadata  : {
            labels: {
                app.kubernetes.io/managed-by: "pulumi"
            }
            name  : output<string>
        }
    + kubernetes:helm.sh/v3:Release: (create)
        [urn=urn:pulumi:dev::pulumi-k8s-test::kubernetes:helm.sh/v3:Release::nginx]
        [provider=urn:pulumi:dev::pulumi-k8s-test::pulumi:providers:kubernetes::default_3_9_0_alpha_1633622661_9bc68e49_dirty::04da6b54-80e4-46f7-96ec-b56ff0331ba9]
        chart         : "nginx"
        compat        : "true"
        metadata      : {
            annotations: {
                pulumi.com/autonamed: "true"
            }
            labels     : {
                app.kubernetes.io/managed-by: "pulumi"
            }
            name       : "nginx-wj35smcw"
        }
        namespace     : output<string>
        repositoryOpts: {
            repo: "https://charts.bitnami.com/bitnami"
        }

Copy link
Member

Choose a reason for hiding this comment

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

I see, thank you. Should we remove KeepUnknowns: true from here then? https://github.com/pulumi/pulumi-kubernetes/blob/master/provider/pkg/provider/helm_release.go#L261

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. I think we can remove all references to keepUnkowns in helm_release.

if !k.clusterUnreachable {
return k.helmReleaseProvider.Check(ctx, req, !k.suppressHelmReleaseBetaWarning)
}
return nil, fmt.Errorf("can't use Helm Release with unreachable cluster. Reason: %q", k.clusterUnreachableReason)
}

// Adopt name from old object if appropriate.
//
// If the user HAS NOT assigned a name in the new inputs, we autoname it and mark the object as
Expand Down Expand Up @@ -1391,12 +1392,6 @@ func (k *kubeProvider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*p
//

urn := resource.URN(req.GetUrn())
if isHelmRelease(urn) {
if !k.clusterUnreachable {
return k.helmReleaseProvider.Diff(ctx, req)
}
return nil, fmt.Errorf("can't use Helm Release with unreachable cluster. Reason: %q", k.clusterUnreachableReason)
}

label := fmt.Sprintf("%s.Diff(%s)", k.label(), urn)
logger.V(9).Infof("%s executing", label)
Expand Down Expand Up @@ -1431,6 +1426,13 @@ func (k *kubeProvider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*p
return nil, err
}

if isHelmRelease(urn) && !hasComputedValue(newInputs) {
if !k.clusterUnreachable {
return k.helmReleaseProvider.Diff(ctx, req)
Copy link
Member

Choose a reason for hiding this comment

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

Same question for Diff I guess. How does the helm-release diff differ from the default diff and are we ready to lose that for any computed value?

Copy link
Member Author

Choose a reason for hiding this comment

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

@viveklak can verify, but I don't think it will be possible to get a more detailed Release diff with computed values since it requires talking to Helm. We still get all the normal provider diffing, which shows client-side config changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah unfortunately we need to rely on helm's ability to handle the templated values which won't be possible with computed values. I believe this is the same experience with helm charts as well.

}
return nil, fmt.Errorf("can't use Helm Release with unreachable cluster. Reason: %q", k.clusterUnreachableReason)
}

namespacedKind, err := clients.IsNamespacedKind(gvk, k.clientSet)
if err != nil {
if clients.IsNoNamespaceInfoErr(err) {
Expand Down Expand Up @@ -1598,7 +1600,8 @@ func (k *kubeProvider) Create(
// comments in those methods for details.
//
urn := resource.URN(req.GetUrn())
if isHelmRelease(urn) {

if isHelmRelease(urn) && !req.GetPreview() {
if !k.clusterUnreachable {
return k.helmReleaseProvider.Create(ctx, req)
}
Expand Down Expand Up @@ -2063,15 +2066,6 @@ func (k *kubeProvider) Update(
}
newInputs := propMapToUnstructured(newResInputs)

if isHelmRelease(urn) {
if !k.clusterUnreachable {
return k.helmReleaseProvider.Update(ctx, req)
}
return nil, fmt.Errorf("can't update Helm Release with unreachable cluster. Reason: %q", k.clusterUnreachableReason)
}
// Ignore old state; we'll get it from Kubernetes later.
oldInputs, _ := parseCheckpointObject(oldState)

// If this is a preview and the input values contain unknowns, return them as-is. This is compatible with
// prior behavior implemented by the Pulumi engine. Similarly, if the server does not support server-side
// dry run, return the inputs as-is.
Expand All @@ -2082,6 +2076,16 @@ func (k *kubeProvider) Update(
return &pulumirpc.UpdateResponse{Properties: req.News}, nil
}

if isHelmRelease(urn) {
if k.clusterUnreachable {
return nil, fmt.Errorf("can't update Helm Release with unreachable cluster. Reason: %q", k.clusterUnreachableReason)
}

return k.helmReleaseProvider.Update(ctx, req)
}
// Ignore old state; we'll get it from Kubernetes later.
oldInputs, _ := parseCheckpointObject(oldState)

annotatedInputs, err := withLastAppliedConfig(newInputs)
if err != nil {
return nil, pkgerrors.Wrapf(
Expand Down
8 changes: 7 additions & 1 deletion tests/sdk/nodejs/examples/helm-release/step1/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
import * as pulumi from "@pulumi/pulumi";
import * as k8s from "@pulumi/kubernetes";
import * as random from "@pulumi/random";


const redisPassword = pulumi.secret("$053cr3t!");

const namespace = new k8s.core.v1.Namespace("release-ns");
const nsName = new random.RandomPet("test");
const namespace = new k8s.core.v1.Namespace("release-ns", {
metadata: {
name: nsName.id
}
});

const release = new k8s.helm.v3.Release("release", {
chart: "redis",
Expand Down
3 changes: 2 additions & 1 deletion tests/sdk/nodejs/examples/helm-release/step1/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"dependencies": {
"@pulumi/pulumi": "^3.0.0",
"@pulumi/kubernetes": "latest",
"@pulumi/kubernetesx": "^0.1.5"
"@pulumi/kubernetesx": "^0.1.5",
"@pulumi/random": "^4.2.0"
}
}
8 changes: 7 additions & 1 deletion tests/sdk/nodejs/examples/helm-release/step2/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
import * as pulumi from "@pulumi/pulumi";
import * as k8s from "@pulumi/kubernetes";
import * as random from "@pulumi/random";


const redisPassword = pulumi.secret("$053cr3t!");

const namespace = new k8s.core.v1.Namespace("release-ns");
const nsName = new random.RandomPet("test");
const namespace = new k8s.core.v1.Namespace("release-ns", {
metadata: {
name: nsName.id
}
});

const release = new k8s.helm.v3.Release("release", {
chart: "redis",
Expand Down