From 1ff5764815e514a7804735416ef4677db99a603d Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 16 Aug 2024 13:06:02 +0100 Subject: [PATCH 1/2] fix(internal/provider): set all supported envbuilder options --- internal/provider/cached_image_resource.go | 182 ++++++++++++------ .../provider/cached_image_resource_test.go | 15 +- 2 files changed, 133 insertions(+), 64 deletions(-) diff --git a/internal/provider/cached_image_resource.go b/internal/provider/cached_image_resource.go index 4ea28b8..c1d2fd3 100644 --- a/internal/provider/cached_image_resource.go +++ b/internal/provider/cached_image_resource.go @@ -25,6 +25,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/diag" + "github.com/hashicorp/terraform-plugin-framework/path" "github.com/hashicorp/terraform-plugin-framework/resource" "github.com/hashicorp/terraform-plugin-framework/resource/schema" "github.com/hashicorp/terraform-plugin-framework/resource/schema/boolplanmodifier" @@ -160,7 +161,6 @@ func (r *CachedImageResource) Schema(ctx context.Context, req resource.SchemaReq MarkdownDescription: "(Envbuilder option) Terminates upon a build failure. This is handy when preferring the FALLBACK_IMAGE in cases where no devcontainer.json or image is provided. However, it ensures that the container stops if the build process encounters an error.", Optional: true, }, - // TODO(mafredri): Map vs List? Support both? "extra_env": schema.MapAttribute{ MarkdownDescription: "Extra environment variables to set for the container. This may include envbuilder options.", ElementType: types.StringType, @@ -293,6 +293,124 @@ func (r *CachedImageResource) Configure(ctx context.Context, req resource.Config r.client = client } +// setComputedEnv sets data.Env and data.EnvMap based on the values of the +// other fields in the model. +func (data *CachedImageResourceModel) setComputedEnv(ctx context.Context) diag.Diagnostics { + env := make(map[string]string) + + env["ENVBUILDER_CACHE_REPO"] = tfValueToString(data.CacheRepo) + env["ENVBUILDER_GIT_URL"] = tfValueToString(data.GitURL) + + if !data.BaseImageCacheDir.IsNull() { + env["ENVBUILDER_BASE_IMAGE_CACHE_DIR"] = tfValueToString(data.BaseImageCacheDir) + } + + if !data.BuildContextPath.IsNull() { + env["ENVBUILDER_BUILD_CONTEXT_PATH"] = tfValueToString(data.BuildContextPath) + } + + if !data.CacheTTLDays.IsNull() { + env["ENVBUILDER_CACHE_TTL_DAYS"] = tfValueToString(data.CacheTTLDays) + } + + if !data.DevcontainerDir.IsNull() { + env["ENVBUILDER_DEVCONTAINER_DIR"] = tfValueToString(data.DevcontainerDir) + } + + if !data.DevcontainerJSONPath.IsNull() { + env["ENVBUILDER_DEVCONTAINER_JSON_PATH"] = tfValueToString(data.DevcontainerJSONPath) + } + + if !data.DockerfilePath.IsNull() { + env["ENVBUILDER_DOCKERFILE_PATH"] = tfValueToString(data.DockerfilePath) + } + + if !data.DockerConfigBase64.IsNull() { + env["ENVBUILDER_DOCKER_CONFIG_BASE64"] = tfValueToString(data.DockerConfigBase64) + } + + if !data.ExitOnBuildFailure.IsNull() { + env["ENVBUILDER_EXIT_ON_BUILD_FAILURE"] = tfValueToString(data.ExitOnBuildFailure) + } + + if !data.FallbackImage.IsNull() { + env["ENVBUILDER_FALLBACK_IMAGE"] = tfValueToString(data.FallbackImage) + } + + if !data.GitCloneDepth.IsNull() { + env["ENVBUILDER_GIT_CLONE_DEPTH"] = tfValueToString(data.GitCloneDepth) + } + + if !data.GitCloneSingleBranch.IsNull() { + env["ENVBUILDER_GIT_CLONE_SINGLE_BRANCH"] = tfValueToString(data.GitCloneSingleBranch) + } + + if !data.GitHTTPProxyURL.IsNull() { + env["ENVBUILDER_GIT_HTTP_PROXY_URL"] = tfValueToString(data.GitHTTPProxyURL) + } + + if !data.GitSSHPrivateKeyPath.IsNull() { + env["ENVBUILDER_GIT_SSH_PRIVATE_KEY_PATH"] = tfValueToString(data.GitSSHPrivateKeyPath) + } + + if !data.GitUsername.IsNull() { + env["ENVBUILDER_GIT_USERNAME"] = tfValueToString(data.GitUsername) + } + + if !data.GitPassword.IsNull() { + env["ENVBUILDER_GIT_PASSWORD"] = tfValueToString(data.GitPassword) + } + + if !data.IgnorePaths.IsNull() { + env["ENVBUILDER_IGNORE_PATHS"] = strings.Join(tfListToStringSlice(data.IgnorePaths), ",") + } + + if !data.Insecure.IsNull() { + env["ENVBUILDER_INSECURE"] = tfValueToString(data.Insecure) + } + + // Default to remote build mode. + if data.RemoteRepoBuildMode.IsNull() { + env["ENVBUILDER_REMOTE_REPO_BUILD_MODE"] = "true" + } else { + env["ENVBUILDER_REMOTE_REPO_BUILD_MODE"] = tfValueToString(data.RemoteRepoBuildMode) + } + + if !data.SSLCertBase64.IsNull() { + env["ENVBUILDER_SSL_CERT_BASE64"] = tfValueToString(data.SSLCertBase64) + } + + if !data.Verbose.IsNull() { + env["ENVBUILDER_VERBOSE"] = tfValueToString(data.Verbose) + } + + if !data.WorkspaceFolder.IsNull() { + env["ENVBUILDER_WORKSPACE_FOLDER"] = tfValueToString(data.WorkspaceFolder) + } + + // Do ExtraEnv last so that it can override any other values. + var diag, ds diag.Diagnostics + if !data.ExtraEnv.IsNull() { + for key, elem := range data.ExtraEnv.Elements() { + if _, ok := env[key]; ok { + // This is a warning because it's possible that the user wants to override + // a value set by the provider. + diag.AddAttributeWarning(path.Root("extra_env"), + "Overriding provider environment variable", + fmt.Sprintf("The key %q in extra_env overrides an environment variable set by the provider.", key), + ) + } + env[key] = tfValueToString(elem) + } + } + + data.EnvMap, ds = basetypes.NewMapValueFrom(ctx, types.StringType, env) + diag = append(diag, ds...) + data.Env, ds = basetypes.NewListValueFrom(ctx, types.StringType, sortedKeyValues(env)) + diag = append(diag, ds...) + return diag +} + func (r *CachedImageResource) Read(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) { var data CachedImageResourceModel @@ -350,35 +468,7 @@ func (r *CachedImageResource) Read(ctx context.Context, req resource.ReadRequest data.Exists = types.BoolValue(true) // Set the expected environment variables. - env := make(map[string]string) - for key, elem := range data.ExtraEnv.Elements() { - env[key] = tfValueToString(elem) - } - - env["ENVBUILDER_CACHE_REPO"] = tfValueToString(data.CacheRepo) - env["ENVBUILDER_GIT_URL"] = tfValueToString(data.GitURL) - - if !data.CacheTTLDays.IsNull() { - env["ENVBUILDER_CACHE_TTL_DAYS"] = tfValueToString(data.CacheTTLDays) - } - if !data.GitUsername.IsNull() { - env["ENVBUILDER_GIT_USERNAME"] = tfValueToString(data.GitUsername) - } - if !data.GitPassword.IsNull() { - env["ENVBUILDER_GIT_PASSWORD"] = tfValueToString(data.GitPassword) - } - // Default to remote build mode. - if data.RemoteRepoBuildMode.IsNull() { - env["ENVBUILDER_REMOTE_REPO_BUILD_MODE"] = "true" - } else { - env["ENVBUILDER_REMOTE_REPO_BUILD_MODE"] = tfValueToString(data.RemoteRepoBuildMode) - } - - var diag diag.Diagnostics - data.EnvMap, diag = basetypes.NewMapValueFrom(ctx, types.StringType, env) - resp.Diagnostics.Append(diag...) - data.Env, diag = basetypes.NewListValueFrom(ctx, types.StringType, sortedKeyValues(env)) - resp.Diagnostics.Append(diag...) + resp.Diagnostics.Append(data.setComputedEnv(ctx)...) resp.Diagnostics.Append(resp.State.Set(ctx, &data)...) } @@ -415,36 +505,9 @@ func (r *CachedImageResource) Create(ctx context.Context, req resource.CreateReq data.Image = types.StringValue(fmt.Sprintf("%s@%s", data.CacheRepo.ValueString(), digest)) data.ID = types.StringValue(digest.String()) } - // Compute the env attribute from the config map. - env := make(map[string]string) - for key, elem := range data.ExtraEnv.Elements() { - env[key] = tfValueToString(elem) - } - env["ENVBUILDER_CACHE_REPO"] = tfValueToString(data.CacheRepo) - env["ENVBUILDER_GIT_URL"] = tfValueToString(data.GitURL) - - if !data.CacheTTLDays.IsNull() { - env["ENVBUILDER_CACHE_TTL_DAYS"] = tfValueToString(data.CacheTTLDays) - } - if !data.GitUsername.IsNull() { - env["ENVBUILDER_GIT_USERNAME"] = tfValueToString(data.GitUsername) - } - if !data.GitPassword.IsNull() { - env["ENVBUILDER_GIT_PASSWORD"] = tfValueToString(data.GitPassword) - } - // Default to remote build mode. - if data.RemoteRepoBuildMode.IsNull() { - env["ENVBUILDER_REMOTE_REPO_BUILD_MODE"] = "true" - } else { - env["ENVBUILDER_REMOTE_REPO_BUILD_MODE"] = tfValueToString(data.RemoteRepoBuildMode) - } - - var diag diag.Diagnostics - data.EnvMap, diag = basetypes.NewMapValueFrom(ctx, types.StringType, env) - resp.Diagnostics.Append(diag...) - data.Env, diag = basetypes.NewListValueFrom(ctx, types.StringType, sortedKeyValues(env)) - resp.Diagnostics.Append(diag...) + // Set the expected environment variables. + resp.Diagnostics.Append(data.setComputedEnv(ctx)...) // Save data into Terraform state resp.Diagnostics.Append(resp.State.Set(ctx, &data)...) @@ -556,6 +619,7 @@ func (r *CachedImageResource) runCacheProbe(ctx context.Context, data CachedImag Insecure: data.Insecure.ValueBool(), // might have internal CAs? IgnorePaths: tfListToStringSlice(data.IgnorePaths), // may need to be specified? // The below options are not relevant and are set to their zero value explicitly. + // They must be set by extra_env. CoderAgentSubsystem: nil, CoderAgentToken: "", CoderAgentURL: "", diff --git a/internal/provider/cached_image_resource_test.go b/internal/provider/cached_image_resource_test.go index 929c7b0..98cd2dd 100644 --- a/internal/provider/cached_image_resource_test.go +++ b/internal/provider/cached_image_resource_test.go @@ -149,16 +149,21 @@ func assertEnv(t *testing.T, deps testDependencies) resource.TestCheckFunc { return resource.ComposeAggregateTestCheckFunc( // Check that the environment variables are set correctly. resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env.0", fmt.Sprintf("ENVBUILDER_CACHE_REPO=%s", deps.CacheRepo)), - resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env.1", fmt.Sprintf("ENVBUILDER_GIT_URL=%s", deps.Repo.URL)), - resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env.2", "ENVBUILDER_REMOTE_REPO_BUILD_MODE=true"), + resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env.1", fmt.Sprintf("ENVBUILDER_GIT_SSH_PRIVATE_KEY_PATH=%s", deps.Repo.Key)), + resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env.2", fmt.Sprintf("ENVBUILDER_GIT_URL=%s", deps.Repo.URL)), + resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env.3", "ENVBUILDER_REMOTE_REPO_BUILD_MODE=true"), + resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env.4", "ENVBUILDER_VERBOSE=true"), // Check that the extra environment variables are set correctly. - resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env.3", "FOO=bar\nbaz"), + resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env.5", "FOO=bar\nbaz"), // We should not have any other environment variables set. - resource.TestCheckNoResourceAttr("envbuilder_cached_image.test", "env.4"), + resource.TestCheckNoResourceAttr("envbuilder_cached_image.test", "env.6"), + // Check that the same values are set in env_map. - resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env_map.FOO", "bar\nbaz"), resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env_map.ENVBUILDER_CACHE_REPO", deps.CacheRepo), + resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env_map.ENVBUILDER_GIT_SSH_PRIVATE_KEY_PATH", deps.Repo.Key), resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env_map.ENVBUILDER_GIT_URL", deps.Repo.URL), resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env_map.ENVBUILDER_REMOTE_REPO_BUILD_MODE", "true"), + resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env_map.ENVBUILDER_VERBOSE", "true"), + resource.TestCheckResourceAttr("envbuilder_cached_image.test", "env_map.FOO", "bar\nbaz"), ) } From 6bec2908529cca048d2a11638081fec0087178f1 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 16 Aug 2024 14:50:35 +0100 Subject: [PATCH 2/2] do not allow overriding git_url and cache_repo --- internal/provider/cached_image_resource.go | 23 ++++++++++++++----- .../provider/cached_image_resource_test.go | 2 ++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/internal/provider/cached_image_resource.go b/internal/provider/cached_image_resource.go index c1d2fd3..dbd5500 100644 --- a/internal/provider/cached_image_resource.go +++ b/internal/provider/cached_image_resource.go @@ -389,18 +389,29 @@ func (data *CachedImageResourceModel) setComputedEnv(ctx context.Context) diag.D } // Do ExtraEnv last so that it can override any other values. + // With one exception: ENVBUILDER_CACHE_REPO and ENVBUILDER_GIT_URL are required and should not be overridden. + // Other values set by the provider may be overridden, but will generate a warning. var diag, ds diag.Diagnostics if !data.ExtraEnv.IsNull() { for key, elem := range data.ExtraEnv.Elements() { - if _, ok := env[key]; ok { - // This is a warning because it's possible that the user wants to override - // a value set by the provider. + switch key { + // These are required and should not be overridden. + case "ENVBUILDER_CACHE_REPO", "ENVBUILDER_GIT_URL": diag.AddAttributeWarning(path.Root("extra_env"), - "Overriding provider environment variable", - fmt.Sprintf("The key %q in extra_env overrides an environment variable set by the provider.", key), + "Cannot override required environment variable", + fmt.Sprintf("The key %q in extra_env cannot be overridden.", key), ) + default: + if _, ok := env[key]; ok { + // This is a warning because it's possible that the user wants to override + // a value set by the provider. + diag.AddAttributeWarning(path.Root("extra_env"), + "Overriding provider environment variable", + fmt.Sprintf("The key %q in extra_env overrides an environment variable set by the provider.", key), + ) + } + env[key] = tfValueToString(elem) } - env[key] = tfValueToString(elem) } } diff --git a/internal/provider/cached_image_resource_test.go b/internal/provider/cached_image_resource_test.go index 98cd2dd..c4f3f9a 100644 --- a/internal/provider/cached_image_resource_test.go +++ b/internal/provider/cached_image_resource_test.go @@ -48,6 +48,8 @@ RUN date > /date.txt`, //nolint: paralleltest deps := setup(ctx, t, tc.files) deps.ExtraEnv["FOO"] = testEnvValue + deps.ExtraEnv["ENVBUILDER_GIT_URL"] = "https://not.the.real.git/url" + deps.ExtraEnv["ENVBUILDER_CACHE_REPO"] = "not-the-real-cache-repo" resource.Test(t, resource.TestCase{ ProtoV6ProviderFactories: testAccProtoV6ProviderFactories,