Skip to content

Commit

Permalink
Add validations for Composer 2/3 only fields (GoogleCloudPlatform#9917)
Browse files Browse the repository at this point in the history 
* block upgrade to composer 3

* make isComposer3 more generic, correct imageVersionChangeValidationFunc

* added validation for Composer 2/3 specific fields

* add tests for validation

* add checks in flattenComposerEnvironmentConfig

* Update attributes of fields not used in Composer 3

* make customizeDiff functions beta only

* remove Computed from gke_cluster

* remove Optional instead of Computed

* add envCfg.PrivateEnvironmentConfig is nil check

* modify isComposer3 to take string

* minor correction to avoid merge conflicts
  • Loading branch information
@spapi17 @tdbhacks
spapi17 authored and tdbhacks committed Feb 23, 2024
1 parent 1cdd437 commit 2550ff0
Show file tree
Hide file tree
Showing 2 changed files with 193 additions and 8 deletions.
70 changes: 62 additions & 8 deletions mmv1/third_party/terraform/services/composer/resource_composer_environment.go.erb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"regexp"
"strings"
"time"
"context"

"github.com/hashicorp/go-version"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff"
Expand Down Expand Up @@ -168,6 +169,10 @@ func ResourceComposerEnvironment() *schema.Resource {
tpgresource.DefaultProviderProject,
tpgresource.DefaultProviderRegion,
tpgresource.SetLabelsDiff,
<% unless version == "ga" -%>
customdiff.ValidateChange("config.0.software_config.0.image_version", imageVersionChangeValidationFunc),
versionValidationCustomizeDiffFunc,
<% end -%>
),

Schema: map[string]*schema.Schema{
Expand Down Expand Up @@ -1592,10 +1597,15 @@ func flattenComposerEnvironmentConfig(envCfg *composer.EnvironmentConfig) interf
transformed["airflow_uri"] = envCfg.AirflowUri
transformed["node_config"] = flattenComposerEnvironmentConfigNodeConfig(envCfg.NodeConfig)
transformed["software_config"] = flattenComposerEnvironmentConfigSoftwareConfig(envCfg.SoftwareConfig)
transformed["private_environment_config"] = flattenComposerEnvironmentConfigPrivateEnvironmentConfig(envCfg.PrivateEnvironmentConfig)
imageVersion := envCfg.SoftwareConfig.ImageVersion
if !isComposer3(imageVersion){
transformed["private_environment_config"] = flattenComposerEnvironmentConfigPrivateEnvironmentConfig(envCfg.PrivateEnvironmentConfig)
}
<% unless version == "ga" -%>
transformed["enable_private_environment"] = envCfg.PrivateEnvironmentConfig.EnablePrivateEnvironment
transformed["enable_private_builds_only"] = envCfg.PrivateEnvironmentConfig.EnablePrivateBuildsOnly
if isComposer3(imageVersion) && envCfg.PrivateEnvironmentConfig != nil {
transformed["enable_private_environment"] = envCfg.PrivateEnvironmentConfig.EnablePrivateEnvironment
transformed["enable_private_builds_only"] = envCfg.PrivateEnvironmentConfig.EnablePrivateBuildsOnly
}
<% end -%>
transformed["web_server_network_access_control"] = flattenComposerEnvironmentConfigWebServerNetworkAccessControl(envCfg.WebServerNetworkAccessControl)
transformed["database_config"] = flattenComposerEnvironmentConfigDatabaseConfig(envCfg.DatabaseConfig)
Expand Down Expand Up @@ -1803,7 +1813,9 @@ func flattenComposerEnvironmentConfigWorkloadsConfig(workloadsConfig *composer.W
transformed["web_server"] = []interface{}{transformedWebServer}
transformed["worker"] = []interface{}{transformedWorker}
<% unless version == "ga" -%>
transformed["dag_processor"] = []interface{}{transformedDagProcessor}
if transformedDagProcessor != nil {
transformed["dag_processor"] = []interface{}{transformedDagProcessor}
}
<% end -%>


Expand Down Expand Up @@ -1982,7 +1994,8 @@ func expandComposerEnvironmentConfig(v interface{}, d *schema.ResourceData, conf
composer.PrivateEnvironmentConfig.EnablePrivateEnvironment in API.
Check image version to avoid overriding EnablePrivateEnvironment in case of other versions.
*/
if isComposer3(d, config) {
imageVersion := d.Get("config.0.software_config.0.image_version").(string)
if isComposer3(imageVersion) {
transformed.PrivateEnvironmentConfig = &composer.PrivateEnvironmentConfig{}
if enablePrivateEnvironmentRaw, ok := original["enable_private_environment"]; ok {
transformed.PrivateEnvironmentConfig.EnablePrivateEnvironment = enablePrivateEnvironmentRaw.(bool)
Expand Down Expand Up @@ -2934,7 +2947,48 @@ func versionsEqual(old, new string) (bool, error) {
return o.Equal(n), nil
}

func isComposer3(d *schema.ResourceData, config *transport_tpg.Config) bool {
image_version := d.Get("config.0.software_config.0.image_version").(string)
return strings.Contains(image_version, "composer-3")
func isComposer3(imageVersion string) bool {
return strings.Contains(imageVersion, "composer-3")
}

func imageVersionChangeValidationFunc(ctx context.Context, old, new, meta any) error {
if old.(string) != "" && !isComposer3(old.(string)) && isComposer3(new.(string)) {
return fmt.Errorf("upgrade to composer 3 is not yet supported")
}
return nil
}

func validateComposer3FieldUsage(d *schema.ResourceDiff, key string, requireComposer3 bool) error {
_, ok := d.GetOk(key)
imageVersion := d.Get("config.0.software_config.0.image_version").(string)
if ok && ( isComposer3(imageVersion) != requireComposer3 ) {
if requireComposer3 {
return fmt.Errorf("error in configuration, %s should only be used in Composer 3", key)
} else {
return fmt.Errorf("error in configuration, %s should not be used in Composer 3", key)
}
}
return nil
}

func versionValidationCustomizeDiffFunc(ctx context.Context, d *schema.ResourceDiff, meta interface{}) error {
composer3FieldUsagePolicy := map[string]bool{
"config.0.node_config.0.max_pods_per_node": false, // not allowed in composer 3
"config.0.node_config.0.enable_ip_masq_agent": false,
"config.0.node_config.0.config.0.node_config.0.ip_allocation_policy": false,
"config.0.private_environment_config": false,
"config.0.master_authorized_networks_config": false,
"config.0.node_config.0.composer_network_attachment": true, // allowed only in composer 3
"config.0.node_config.0.composer_internal_ipv4_cidr_block": true,
"config.0.software_config.0.web_server_plugins_mode": true,
"config.0.enable_private_environment": true,
"config.0.enable_private_builds_only": true,
"config.0.workloads_config.0.dag_processor": true,
}
for key, allowed := range composer3FieldUsagePolicy {
if err := validateComposer3FieldUsage(d, key, allowed); err != nil {
return err
}
}
return nil
}
131 changes: 131 additions & 0 deletions mmv1/third_party/terraform/services/composer/resource_composer_environment_test.go.erb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1186,6 +1186,77 @@ func TestAccComposerEnvironmentComposer3_update(t *testing.T) {
})
}

func TestAccComposerEnvironmentComposer3_upgrade_expectError(t *testing.T) {
t.Parallel()

envName := fmt.Sprintf("%s-%d", testComposerEnvironmentPrefix, acctest.RandInt(t))
network := fmt.Sprintf("%s-%d", testComposerNetworkPrefix, acctest.RandInt(t))
subnetwork := network + "-1"
errorRegExp, _ := regexp.Compile(".*upgrade to composer 3 is not yet supported.*")

acctest.VcrTest(t, resource.TestCase{
PreCheck: func() { acctest.AccTestPreCheck(t) },
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t),
CheckDestroy: testAccComposerEnvironmentDestroyProducer(t),
Steps: []resource.TestStep{
{
Config: testAccComposerEnvironmentComposer2_empty(envName, network, subnetwork),
},
{
Config: testAccComposerEnvironmentComposer3_empty(envName, network, subnetwork),
ExpectError: errorRegExp,
},
// This is a terrible clean-up step in order to get destroy to succeed,
// due to dangling firewall rules left by the Composer Environment blocking network deletion.
// TODO: Remove this check if firewall rules bug gets fixed by Composer.
{
PlanOnly: true,
ExpectNonEmptyPlan: false,
Config: testAccComposerEnvironmentComposer2_empty(envName, network, subnetwork),
Check: testAccCheckClearComposerEnvironmentFirewalls(t, network),
},
},
})
}

func TestAccComposerEnvironmentComposer2_usesUnsupportedField_expectError(t *testing.T) {
t.Parallel()

envName := fmt.Sprintf("%s-%d", testComposerEnvironmentPrefix, acctest.RandInt(t))
errorRegExp, _ := regexp.Compile(".*error in configuration, .* should only be used in Composer 3.*")

acctest.VcrTest(t, resource.TestCase{
PreCheck: func() { acctest.AccTestPreCheck(t) },
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t),
CheckDestroy: testAccComposerEnvironmentDestroyProducer(t),
Steps: []resource.TestStep{
{
Config: testAccComposerEnvironmentComposer2_usesUnsupportedField(envName),
ExpectError: errorRegExp,
},
},
})
}

func TestAccComposerEnvironmentComposer3_usesUnsupportedField_expectError(t *testing.T) {
t.Parallel()

envName := fmt.Sprintf("%s-%d", testComposerEnvironmentPrefix, acctest.RandInt(t))
errorRegExp, _ := regexp.Compile(".*error in configuration, .* should not be used in Composer 3.*")

acctest.VcrTest(t, resource.TestCase{
PreCheck: func() { acctest.AccTestPreCheck(t) },
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t),
CheckDestroy: testAccComposerEnvironmentDestroyProducer(t),
Steps: []resource.TestStep{
{
Config: testAccComposerEnvironmentComposer3_usesUnsupportedField(envName),
ExpectError: errorRegExp,
},
},
})
}

// Checks Composer 3 specific updatable fields.
func TestAccComposerEnvironmentComposer3_updateToEmpty(t *testing.T) {
t.Parallel()
Expand Down Expand Up @@ -2835,6 +2906,34 @@ resource "google_project_iam_member" "composer-worker" {
}

<% unless version == "ga" -%>
func testAccComposerEnvironmentComposer2_empty(name, network, subnetwork string) string {
return fmt.Sprintf(`
resource "google_composer_environment" "test" {
name = "%s"
region = "us-central1"
config {
software_config {
image_version = "composer-2-airflow-2"
}
}
}

// use a separate network to avoid conflicts with other tests running in parallel
// that use the default network/subnet
resource "google_compute_network" "test" {
name = "%s"
auto_create_subnetworks = false
}

resource "google_compute_subnetwork" "test" {
name = "%s"
ip_cidr_range = "10.2.0.0/16"
region = "us-central1"
network = google_compute_network.test.self_link
}
`, name, network, subnetwork)
}

func testAccComposerEnvironmentComposer3_empty(name, network, subnetwork string) string {
return fmt.Sprintf(`
resource "google_composer_environment" "test" {
Expand Down Expand Up @@ -2863,6 +2962,38 @@ resource "google_compute_subnetwork" "test" {
`, name, network, subnetwork)
}

func testAccComposerEnvironmentComposer2_usesUnsupportedField(name string) string {
return fmt.Sprintf(`
resource "google_composer_environment" "test" {
name = "%s"
region = "us-central1"
config {
software_config {
image_version = "composer-2-airflow-2"
web_server_plugins_mode = "ENABLED"
}
}
}
`, name)
}

func testAccComposerEnvironmentComposer3_usesUnsupportedField(name string) string {
return fmt.Sprintf(`
resource "google_composer_environment" "test" {
name = "%s"
region = "us-central1"
config {
node_config {
enable_ip_masq_agent = true
}
software_config {
image_version = "composer-3-airflow-2"
}
}
}
`, name)
}

func testAccComposerEnvironmentComposer3_basic(name, network, subnetwork string) string {
return fmt.Sprintf(`
resource "google_composer_environment" "test" {
Expand Down

0 comments on commit 2550ff0

Please sign in to comment.