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

fix!: unable to create fsx for OpenZfs Multi-AZ file system #4095

Merged
merged 8 commits into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 1 addition & 6 deletions provider/cmd/pulumi-resource-aws/bridge-metadata.json
Original file line number Diff line number Diff line change
Expand Up @@ -12167,7 +12167,7 @@
"maxItemsOne": false
},
"subnet_ids": {
"maxItemsOne": true
"maxItemsOne": false
}
}
},
Expand Down Expand Up @@ -230633,11 +230633,6 @@
"certificate_authority": true
}
},
"aws_fsx_openzfs_file_system": {
"maxItemsOneOverrides": {
"subnet_ids": true
}
},
"aws_lexv2models_slot": {
"maxItemsOneOverrides": {
"value_elicitation_setting.$.prompt_specification.$.message_group.$.message.$.custom_payload": false,
Expand Down

Large diffs are not rendered by default.

21 changes: 15 additions & 6 deletions provider/cmd/pulumi-resource-aws/schema.json

Large diffs are not rendered by default.

200 changes: 200 additions & 0 deletions provider/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
package provider

import (
"encoding/json"
"fmt"
"os"
"path/filepath"
"testing"
Expand All @@ -11,7 +13,12 @@ import (
"github.com/pulumi/providertest/optproviderupgrade"
"github.com/pulumi/providertest/pulumitest"
"github.com/pulumi/providertest/pulumitest/assertpreview"
"github.com/pulumi/providertest/pulumitest/optnewstack"
"github.com/pulumi/providertest/pulumitest/opttest"
"github.com/pulumi/pulumi/sdk/v3/go/auto/optpreview"
"github.com/pulumi/pulumi/sdk/v3/go/auto/optup"
"github.com/pulumi/pulumi/sdk/v3/go/common/apitype"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -82,6 +89,107 @@ func testProviderUpgrade(t *testing.T, dir string, opts *testProviderUpgradeOpti
assertpreview.HasNoReplacements(t, result)
}

type testProviderCodeChangesOptions struct {
Copy link
Member

Choose a reason for hiding this comment

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

I'm looking at this and I'm thinking that ideally we'd figure out how to upstream this into https://github.com/pulumi/providertest

This is almost like:

func PreviewProviderUpgrade(t pulumitest.PT, pulumiTest *pulumitest.PulumiTest, providerName string, baselineVersion string, opts ...optproviderupgrade.PreviewProviderUpgradeOpt) auto.PreviewResult {

But not quite.

One difference is that it lets you specify two sets of programs and options which I've also wanted and I think would be a very useful thing to have.

Another difference is that your test also runs pulumi up? Or am I misreading this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think we could upstream this since I think it is a generic enough use case. And yes those are the two main differences.

region string
firstProgram []byte
secondProgram []byte
firstProgramOptions []opttest.Option
secondProgramOptions []opttest.Option
}

// testProviderCodeChanges tests two different runs of a pulumi program. This allow you to run
// pulumi up with an initial program, change the code of the program and then run another pulumi command
func testProviderCodeChanges(t *testing.T, opts *testProviderCodeChangesOptions) *pulumitest.PulumiTest {
corymhall marked this conversation as resolved.
Show resolved Hide resolved
skipIfShort(t)
t.Parallel()
t.Helper()

workdir := t.TempDir()
cacheDir := filepath.Join("testdata", "recorded", "TestProviderUpgrade", t.Name())
err := os.MkdirAll(cacheDir, 0755)
assert.NoError(t, err)
stackExportFile := filepath.Join(cacheDir, "stack.json")

err = os.WriteFile(filepath.Join(workdir, "Pulumi.yaml"), opts.firstProgram, 0o600)
require.NoError(t, err)

options := []opttest.Option{
opttest.SkipInstall(),
opttest.NewStackOptions(optnewstack.DisableAutoDestroy()),
}
for _, o := range opts.firstProgramOptions {
options = append(options, o)
}

pt := pulumitest.NewPulumiTest(t, workdir, options...)
region := "us-east-2"
if opts != nil && opts.region != "" {
region = opts.region
}
pt.SetConfig("aws:region", region)

var export *apitype.UntypedDeployment
export, err = tryReadStackExport(stackExportFile)
if err != nil {
pt.Up()
grptLog := pt.GrpcLog()
grpcLogPath := filepath.Join(cacheDir, "grpc.json")
pt.T().Logf("writing grpc log to %s", grpcLogPath)
grptLog.WriteTo(grpcLogPath)

e := pt.ExportStack()
export = &e
err = writeStackExport(stackExportFile, export, true)
assert.NoError(t, err)
}

secondOptions := []opttest.Option{
opttest.SkipInstall(),
opttest.NewStackOptions(optnewstack.EnableAutoDestroy()),
}
for _, o := range opts.secondProgramOptions {
secondOptions = append(secondOptions, o)
}
err = os.WriteFile(filepath.Join(workdir, "Pulumi.yaml"), opts.secondProgram, 0o600)
require.NoError(t, err)
secondTest := pulumitest.NewPulumiTest(t, workdir, secondOptions...)
secondTest.SetConfig("aws:region", region)
secondTest.ImportStack(*export)

return secondTest
}

// pulumiUpWithSnapshot will only run the up portion of the test if the plan has changed since the
// last time the test was run.
//
// This should be used when the plan is a good representation of what you are testing. Sometimes
// there are issues where the plan is consistent, but the apply fails. In those cases a snapshot test is not
// a good fit.
func pulumiUpWithSnapshot(t *testing.T, pulumiTest *pulumitest.PulumiTest) {
Copy link
Member

Choose a reason for hiding this comment

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

This looks curious, I could use some help understanding what this is testing additionally, and perhaps if this is generally useful if we can upstream it to pulumitest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm experimenting with something here. I think it may be useful to upstream, but I'm not sure yet since I think it would be easy to footgun yourself. I'm trying to figure out ways we can short circuit the test.

I think it is useful for the type of test where everything that you want to test exists in the plan snapshot. In those cases we can short circuit the test and only actually run the test when the plan has changed. In this case I'm testing that subnets as a list is able to be deployed and the snapshot contains this information.

In other cases the snapshot won't contain enough information, for example if the test depends on the value of unknowns the plan won't have those values so will never show a difference.

Copy link
Member

Choose a reason for hiding this comment

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

So you're looking for an optimization that doesn't run pulumi up when the plan hasn't changed, but does run it if there's changes in plans? This can be interesting. I worry that plan support is not yet solid enough to trust for this, but it can be interesting.

workdir := os.TempDir()
cwd, err := os.Getwd()
assert.NoError(t, err)
cacheDir := filepath.Join(cwd, "testdata", "recorded", "TestProviderUpgrade", t.Name())
planFile := filepath.Join(cacheDir, "plan.json")
if ok, _ := exists(planFile); ok {
err := os.MkdirAll(cacheDir, 0755)
assert.NoError(t, err)
tmpPlanFile := filepath.Join(workdir, "plan.json")

pulumiTest.Preview(optpreview.Plan(tmpPlanFile))

if equal := planEqual(t, planFile, tmpPlanFile); equal {
return
}

pulumiTest.T().Log("Plan is not equal, re-running up")
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!

}
pulumiTest.Preview(optpreview.Plan(planFile))
upResult := pulumiTest.Up(optup.Plan(planFile))
fmt.Printf("stdout: %s \n", upResult.StdOut)
fmt.Printf("stderr: %s \n", upResult.StdErr)
}

func pulumiTest(t *testing.T, dir string, opts ...opttest.Option) *pulumitest.PulumiTest {
if testing.Short() {
t.Skipf("Skipping in testing.Short() mode, assuming this is a CI run without AWS creds")
Expand Down Expand Up @@ -110,3 +218,95 @@ func maxDuration(dur time.Duration, t *testing.T, test func(t *testing.T)) {
case <-done:
}
}

// writeStackExport writes the stack export to the given path creating any directories needed.
func writeStackExport(path string, snapshot *apitype.UntypedDeployment, overwrite bool) error {
if snapshot == nil {
return fmt.Errorf("stack export must not be nil")
}
dir := filepath.Dir(path)
err := os.MkdirAll(dir, 0755)
if err != nil {
return err
}
stackBytes, err := json.MarshalIndent(snapshot, "", " ")
if err != nil {
return err
}
pathExists, err := exists(path)
if err != nil {
return err
}
if pathExists && !overwrite {
return fmt.Errorf("stack export already exists at %s", path)
}
return os.WriteFile(path, stackBytes, 0644)
}

// tryReadStackExport reads a stack export from the given file path.
// If the file does not exist, returns nil, nil.
func tryReadStackExport(path string) (*apitype.UntypedDeployment, error) {
stackBytes, err := os.ReadFile(path)
if err != nil {
return nil, fmt.Errorf("failed to read stack export at %s: %v", path, err)
}
var stackExport apitype.UntypedDeployment
err = json.Unmarshal(stackBytes, &stackExport)
if err != nil {
return nil, fmt.Errorf("failed to unmarshal stack export at %s: %v", path, err)
}
return &stackExport, nil
}

func exists(filePath string) (bool, error) {
_, err := os.Stat(filePath)
switch {
case err == nil:
return true, nil
case !os.IsNotExist(err):
return false, err
}
return false, nil
}

// cleanPlan removes fields that change every time, i.e. 'time' or 'seed'
func cleanPlan(t *testing.T, plan map[string]interface{}) map[string]interface{} {
t.Helper()
if val, exists := plan["resourcePlans"]; exists {
resourcePlans := val.(map[string]interface{})
for _, v := range resourcePlans {
resourcePlan := v.(map[string]interface{})
delete(resourcePlan, "seed")
}
}

if val, exists := plan["manifest"]; exists {
manifest := val.(map[string]interface{})
delete(manifest, "time")
}

return plan
}

func readPlan(t *testing.T, planPath string) map[string]interface{} {
t.Helper()
var firstData map[string]interface{}

firstFile, err := os.ReadFile(planPath)
assert.NoError(t, err)

err = json.Unmarshal(firstFile, &firstData)
assert.NoError(t, err)
firstData = cleanPlan(t, firstData)

return firstData
}

// planEqual asserts that two plans are equal
func planEqual(t *testing.T, firstPlan, secondPlan string) bool {
t.Helper()
firstPlanData := readPlan(t, firstPlan)
secondPlanData := readPlan(t, secondPlan)

return assert.Equal(t, firstPlanData, secondPlanData)
}
77 changes: 77 additions & 0 deletions provider/provider_yaml_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,83 @@ func TestNonIdempotentSnsTopic(t *testing.T) {
require.ErrorContains(t, err, "already exists")
}

func TestOpenZfsFileSystemUpgrade(t *testing.T) {
if testing.Short() {
t.Skipf("Skipping in testing.Short() mode, assuming this is a CI run without credentials")
}
const pulumiYaml = `
name: openzfs
runtime: yaml
resources:
MyFileSystem:
properties:
deploymentType: SINGLE_AZ_1
storageCapacity: 64
%s
throughputCapacity: 64
type: aws:fsx:OpenZfsFileSystem
MySubnet:
properties:
cidrBlock: "10.0.1.0/24"
vpcId: ${MyVPC.id}
type: aws:ec2:Subnet
MyVPC:
properties:
cidrBlock: "10.0.0.0/16"
type: aws:ec2:Vpc
`

var (
providerName string = "aws"
baselineVersion string = "6.41.0"
)
cwd, err := os.Getwd()
assert.NoError(t, err)
workdir := t.TempDir()

firstProgram := []byte(fmt.Sprintf(pulumiYaml, "subnetIds: ${MySubnet.id}"))
corymhall marked this conversation as resolved.
Show resolved Hide resolved
secondProgram := []byte(fmt.Sprintf(pulumiYaml, "subnetIds:\n - ${MySubnet.id}"))
// test that we can upgrade from the previous version which accepted a string for `subnetIds`
// to the new version which accepts a list
t.Run("upgrade", func(t *testing.T) {
pulumiTest := testProviderCodeChanges(t, &testProviderCodeChangesOptions{
firstProgram: firstProgram,
firstProgramOptions: []opttest.Option{
opttest.DownloadProviderVersion(providerName, baselineVersion),
},
secondProgram: secondProgram,
secondProgramOptions: []opttest.Option{
opttest.LocalProviderPath("aws", filepath.Join(cwd, "..", "bin")),
},
})

res := pulumiTest.Preview()
fmt.Printf("stdout: %s \n", res.StdOut)
fmt.Printf("stderr: %s \n", res.StdErr)
assertpreview.HasNoChanges(t, res)

upResult := pulumiTest.Up()
fmt.Printf("stdout: %s \n", upResult.StdOut)
fmt.Printf("stderr: %s \n", upResult.StdErr)
})

// test that we can deploy a new filesystem with a list of subnetIds
// we use a test with a snapshot since this test is only useful the first time, once
// we know it works it should continue to work.
t.Run("new-version", func(t *testing.T) {
err = os.WriteFile(filepath.Join(workdir, "Pulumi.yaml"), secondProgram, 0o600)
assert.NoError(t, err)
pulumiTest := pulumitest.NewPulumiTest(t, workdir,
opttest.SkipInstall(),
opttest.LocalProviderPath("aws", filepath.Join(cwd, "..", "bin")),
)

pulumiTest.SetConfig("aws:region", "us-east-2")

pulumiUpWithSnapshot(t, pulumiTest)
})
}

// Make sure that legacy Bucket supports deleting tags out of band and detecting drift.
func TestRegress3674(t *testing.T) {
ptest := pulumiTest(t, filepath.Join("test-programs", "regress-3674"), opttest.SkipInstall())
Expand Down
24 changes: 20 additions & 4 deletions provider/resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (

pftfbridge "github.com/pulumi/pulumi-terraform-bridge/pf/tfbridge"
"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge"
"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge/info"
tks "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge/tokens"
shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim"
shimv2 "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim/sdk-v2"
Expand Down Expand Up @@ -2279,10 +2280,25 @@ compatibility shim in favor of the new "name" field.`)
"aws_fsx_ontap_file_system": {Tok: awsResource(fsxMod, "OntapFileSystem")},
"aws_fsx_ontap_storage_virtual_machine": {Tok: awsResource(fsxMod, "OntapStorageVirtualMachine")},
"aws_fsx_ontap_volume": {Tok: awsResource(fsxMod, "OntapVolume")},
"aws_fsx_openzfs_file_system": {Tok: awsResource(fsxMod, "OpenZfsFileSystem")},
"aws_fsx_openzfs_snapshot": {Tok: awsResource(fsxMod, "OpenZfsSnapshot")},
"aws_fsx_openzfs_volume": {Tok: awsResource(fsxMod, "OpenZfsVolume")},
"aws_fsx_windows_file_system": {Tok: awsResource(fsxMod, "WindowsFileSystem")},
"aws_fsx_openzfs_file_system": {
Tok: awsResource(fsxMod, "OpenZfsFileSystem"),
TransformFromState: func(ctx context.Context, state resource.PropertyMap) (resource.PropertyMap, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Curious if you found this necessary? If I recall correctly the bridge might be already applying this transformation generically to permit lenient state reads to compensate for MaxItems=1 mismatches. If it doesn't at the moment we could consider moving it there anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it was necessary. Without it it showed a resource replacement.

if val, ok := state["subnetIds"]; ok {
if val.IsString() {
state["subnetIds"] = resource.NewArrayProperty([]resource.PropertyValue{val})
}
}
return state, nil
},
Fields: map[string]*info.Schema{
"subnet_ids": {
MaxItemsOne: tfbridge.False(),
},
},
},
"aws_fsx_openzfs_snapshot": {Tok: awsResource(fsxMod, "OpenZfsSnapshot")},
"aws_fsx_openzfs_volume": {Tok: awsResource(fsxMod, "OpenZfsVolume")},
"aws_fsx_windows_file_system": {Tok: awsResource(fsxMod, "WindowsFileSystem")},

// GameLift

Expand Down
Loading
Loading