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

Optionally render YAML for k8s resources #936

Merged
merged 14 commits into from
Feb 7, 2020
Merged

Conversation

lblackstone
Copy link
Member

@lblackstone lblackstone commented Jan 2, 2020

Proposed changes

This is a proof of concept for rendering YAML manifests rather than managing the resources
directly using Pulumi. The idea here is to allow users to manage k8s apps with other tooling,
while still integrating nicely with Pulumi-managed components. This would also allow users to leverage the kubernetesx (kx) library to define their applications without also requiring them to use Pulumi to deploy them.

In this prototype, I created an option on the Provider resource that allows a user to toggle the YAML rendering behavior by specifying an output directory for the manifests. When pulumi up runs, the provider renders the manifests to the specified directory rather than creating them on a k8s cluster.

Here's what this looks like currently:
Screen Shot 2020-01-02 at 2 01 23 PM
Screen Shot 2020-01-02 at 2 01 44 PM

There are some open questions I'd like input on:

  1. Should we create/update/delete the rendered manifests in lieu of the k8s resources, or simply render them to the specified directory and let users manage lifecycle beyond that?
  2. Do we need to do anything special related to manifest ordering, or just let downstream users handle that? CRDs generally have to be applied prior to other resources, but it wouldn't be too hard for clients to apply customresourcedefinition-* prior to the rest.

Update 1:
Based on the early feedback, I've updated this PR to manage the full lifecycle of the rendered manifests (create/update/delete) in lieu of the k8s resources.

Related issues (optional)

Fixes #29

@brandonkal
Copy link

brandonkal commented Jan 3, 2020

This is great to see. Some thoughts/questions:
A status of "created" may be a bit confusing. The YAML was created, but perhaps giving it a different status such as "rendered" would make things clearer.

Should we create/update/delete the rendered manifests in lieu of the k8s resources, or simply render them to the specified directory and let users manage lifecycle beyond that?

Pulumi should assume management of the rendered YAML directory. So deleted resources should be removed. However, Pulumi should assume that other tools may modify the YAML downstream, so state should essentially be ignored (i.e. always replace a YAML file if the rendered contents do not match the file state.

Do we need to do anything special related to manifest ordering, or just let downstream users handle that? CRDs generally have to be applied prior to other resources, but it wouldn't be too hard for clients to apply customresourcedefinition-* prior to the rest.

Using annotations, Pulumi can include information about the DAG in the rendered manifests. Other tools can then use this information for ordering. Take for instance https://get-kapp.io/. I recently discovered this tool and it offers a pulumi-esque workflow with preview but is essentially a drop-in-replacement for kubectl apply -f. That tool has some apply ordering annotations. Based on the information Pulumi knows about dependencies, Pulumi could directly render these annotations. A deploymentTool parameter for the provider could decide which such annotations to render as other tools have similar but different annotations. Or pulumi could invent its own schema for such annotations, and then some downstream binary could be used to convert to the deployment-tool-specific annotation schema. The point is that instead of just rendering plain YAML, pulumi can also communicate information it knows about the dependency graph.

Another concern is that if the pulumi up generates secrets, those would be in the rendered YAML in the clear. Supporting something like sops or automatic Kubernetes Secret to Bitnami SealedSecrets conversion would be more important here.

@lblackstone
Copy link
Member Author

A status of "created" may be a bit confusing. The YAML was created, but perhaps giving it a different status such as "rendered" would make things clearer.

This would require changes to the core engine, so this probably won't happen. I'll output a message in the info column for each resource that includes the render target to make that clearer.

Pulumi should assume management of the rendered YAML directory.

I think this makes sense. Just to be clear, this would overwrite any out-of-band changes to the rendered manifests when pulumi up runs.

Using annotations, Pulumi can include information about the DAG in the rendered manifests.
...

I'm not so sure about this one. I'm hesitant to add non-standard fields to user manifests, and generally the ordering doesn't matter much beyond CRDs being applied first. Pulumi uses this information to minimize time spent retrying, but Kubernetes' eventual consistency model means that it's reasonable to apply all of the rest of the manifests at the same time.

Perhaps it would make sense to create numbered subdirectories to make this easier. For example:

0-crd/
- customresourcedefinition-foo.yaml
- customresourcedefinition-bar.yaml
1-manifests/
- deployment-foo.yaml
- service-foo.yaml

This could be applied with
kubectl apply -f 0-crd/
followed by
kubectl apply -f 1-manifests/

I haven't spent much time with other deployment tools like kustomize, Argo, Skaffold, etc., so I'm curious to hear feedback on this proposal.

Another concern is that if the pulumi up generates secrets, those would be in the rendered YAML in the clear. Supporting something like sops or automatic Kubernetes Secret to Bitnami SealedSecrets conversion would be more important here.

This is a great point. It would be easy to accidentally leak secrets like this, especially if the rendered manifests get checked into source control.

@brandonkal
Copy link

Under ideal conditions, the user could have the option to only template out the YAML. I see this proof-of-concept PR doesn't change anything in regards to the other parts of the lifecycle. Given that pulumi-kubernetes Diff is extremely slow (#937), it would be nice to skip that stage entirely if rendering only YAML manifests.

I can see the hesitation regarding embedding annotations as that increases complexity. Still, it would be nice to have pulumi write out information about dependencies as it goes, Perhaps this could be done to a seperate file, and it is not kubernetes-specific.

@lblackstone
Copy link
Member Author

Updated to render manifests to two subdirectories: one for CRDs and one for other manifests. Also added a status message showing the YAML path corresponding to each resource.

@lblackstone
Copy link
Member Author

For now, we decided to simply print a warning for rendered resources that contain a Pulumi secret value, and leave it up to the user to handle appropriately.

Here's what that looks like:
Screen Shot 2020-01-23 at 4 11 24 PM

@lblackstone lblackstone changed the title WIP - Proof of concept for rendering YAML on update Optionally render YAML for k8s resources Jan 23, 2020
@lblackstone
Copy link
Member Author

@pgavlin This is RFR now.

@lblackstone lblackstone marked this pull request as ready for review January 28, 2020 00:31
@pgavlin
Copy link
Member

pgavlin commented Feb 3, 2020

This is a proof of concept for rendering YAML manifests rather than managing the resources
directly using Pulumi. The idea here is to allow users to manage k8s apps with other tooling,
while still integrating nicely with Pulumi-managed components

How do resource outputs work here?

@lblackstone
Copy link
Member Author

How do resource outputs work here?

In most cases, Outputs are already resolved by the time the YAML is rendered, since this occurs during the update step. Some Outputs will not be available as usual, such as endpoints on a k8s Service, which won't ever be allocated since the resource isn't actually created.

Most usages should work as expected, including more complex scenarios using libraries like kx.

@pgavlin
Copy link
Member

pgavlin commented Feb 3, 2020

Some Outputs will not be available as usual

This is what I suspected. Won't that violate our stated types?

@lblackstone
Copy link
Member Author

Won't that violate our stated types?

Possibly? Here's an example where the Outputs are unavailable since they rely on a real k8s resource being created. It doesn't fail with an error; the Outputs just resolve to undefined.

Do you have suggestions for something to change here? I'd generally consider this kind of usage to be a user error.

const provider = new k8s.Provider("render-yaml", {
    renderYamlToDirectory: "/Users/levi/workspace/pulumi-k8s-test/rendered",
});

const appLabels = {app: "nginx"};
const deployment = new k8s.apps.v1.Deployment("nginx", {
    spec: {
        selector: {matchLabels: appLabels},
        template: {
            metadata: {labels: appLabels},
            spec: {containers: [{name: "nginx", image: "nginx", ports: [{containerPort: 80}]}]}
        }
    }
}, {provider});
const service = new k8s.core.v1.Service("nginx", {
    spec: {
        ports: [{port: 80, protocol: "TCP"}],
        selector: appLabels,
    }
}, {provider});
export const clusterIP = service.spec.clusterIP;
export const endpoint = service.status.loadBalancer.ingress[0].ip || service.status.loadBalancer.ingress[0].hostname;

Screen Shot 2020-02-03 at 4 26 17 PM

@pgavlin
Copy link
Member

pgavlin commented Feb 3, 2020

Do you have suggestions for something to change here? I'd generally consider this kind of usage to be a user error.

I don't, unfortunately, besides rigorously documenting this. However, this could break programs in surprising ways if they pas outputs that are typed as always present to other resources. Not sure that there's anything we can do about that.

@lblackstone
Copy link
Member Author

I'll update the docstring to make the expectation clear, but I think it's probably fine to ship. I think that these types of errors should be relatively easy to spot.

Copy link
Member

@lukehoban lukehoban left a comment

Choose a reason for hiding this comment

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

Would be great to have test coverage of this option.

Won't that violate our stated types?

Just so I understand better - what exactly will we do with the current implementation? Will we resolve all the non-input outputs to undefined?

Will things like this work correctly? https://github.com/pulumi/examples/blob/master/kubernetes-ts-s3-rollout/index.ts#L49

I think that these types of errors should be relatively easy to spot.

Unfortunately, I'm not sure that will be true. We've seen examples of this same sort of reasoning in many other places - in testing frameworks, in PaC, and even in standard preview - and in all cases I'd say we've been surprised the extent to which users just reasonably expect any API that is exposed to always produce a meaningful value of the defined type.

That said - like @pgavlin I have no better suggestions.

pkg/gen/dotnet-templates/Provider.cs Outdated Show resolved Hide resolved
pkg/provider/provider.go Outdated Show resolved Hide resolved
pkg/provider/provider.go Outdated Show resolved Hide resolved
pkg/provider/provider.go Outdated Show resolved Hide resolved
@lblackstone
Copy link
Member Author

Just so I understand better - what exactly will we do with the current implementation? Will we resolve all the non-input outputs to undefined?

Yes, the values will be undefined. For example:

service.spec.apply(spec => console.log('applied clusterIP = ' + spec.clusterIP));

results in the following:

applied clusterIP = undefined

even though clusterIP is typed as string.

Will things like this work correctly? https://github.com/pulumi/examples/blob/master/kubernetes-ts-s3-rollout/index.ts#L49

Yes, that works as expected. The only Outputs that aren't resolved are ones that a Kubernetes cluster would set, such as status fields, or the .spec.clusterIP from the example above.

@lukehoban
Copy link
Member

I think we should go ahead with this. We need to get real - world feedback on this to guide any next steps here - and on the core technical question of how to represent outputs - I think there are no concretely better suggestions than the current implementation despite general unease.

We might want to add a note to RenderYamlToDirectory doc comments that this is a preview feature of the provider, just to provide some room to adjust to user feedback.

- Add support for Update and Delete
- Add support for DiffConfig
CRDs are rendered to a separate subdirectory to simplify application ordering
@lblackstone
Copy link
Member Author

Updated the doc comments with a note that this feature is in beta / developer preview

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a way to export Kubernetes YAML
4 participants