From 3f97cb4b6d742cb4047fd03485d26d0d947119f8 Mon Sep 17 00:00:00 2001 From: Igal Tsoiref Date: Thu, 21 Sep 2023 14:12:29 +0300 Subject: [PATCH] Fix update proxy in cluster rosa --- .../cluster_rosa_classic_resource.go | 40 +++++++++---------- provider/proxy/proxy_validator.go | 20 +--------- subsystem/cluster_resource_rosa_test.go | 38 +++++++++--------- 3 files changed, 38 insertions(+), 60 deletions(-) diff --git a/provider/clusterrosaclassic/cluster_rosa_classic_resource.go b/provider/clusterrosaclassic/cluster_rosa_classic_resource.go index 90533f6c..bb9115ad 100644 --- a/provider/clusterrosaclassic/cluster_rosa_classic_resource.go +++ b/provider/clusterrosaclassic/cluster_rosa_classic_resource.go @@ -20,6 +20,7 @@ import ( "errors" "fmt" "net/http" + "reflect" "sort" "strings" "time" @@ -711,24 +712,26 @@ func buildProxy(state *ClusterRosaClassicState, builder *cmv1.ClusterBuilder) (* if state.Proxy != nil { httpsProxy := "" httpProxy := "" + httpNoProxy := "" additionalTrustBundle := "" if !common.IsStringAttributeEmpty(state.Proxy.HttpProxy) { httpProxy = state.Proxy.HttpProxy.ValueString() - proxy.HTTPProxy(httpProxy) } + proxy.HTTPProxy(httpProxy) if !common.IsStringAttributeEmpty(state.Proxy.HttpsProxy) { httpsProxy = state.Proxy.HttpsProxy.ValueString() - proxy.HTTPSProxy(httpsProxy) } + proxy.HTTPSProxy(httpsProxy) if !common.IsStringAttributeEmpty(state.Proxy.NoProxy) { - proxy.NoProxy(state.Proxy.NoProxy.ValueString()) + httpNoProxy = state.Proxy.NoProxy.ValueString() } + proxy.NoProxy(httpNoProxy) if !common.IsStringAttributeEmpty(state.Proxy.AdditionalTrustBundle) { additionalTrustBundle = state.Proxy.AdditionalTrustBundle.ValueString() - builder.AdditionalTrustBundle(additionalTrustBundle) } + builder.AdditionalTrustBundle(additionalTrustBundle) builder.Proxy(proxy) } @@ -1032,7 +1035,7 @@ func (r *ClusterRosaClassicResource) Update(ctx context.Context, request resourc return } - clusterBuilder, _, err = updateProxy(state, plan, clusterBuilder) + clusterBuilder, err = updateProxy(state, plan, clusterBuilder) if err != nil { response.Diagnostics.AddError( "Can't update cluster", @@ -1288,30 +1291,21 @@ func scheduleUpgrade(ctx context.Context, client *cmv1.ClustersClient, clusterID return nil } -func updateProxy(state, plan *ClusterRosaClassicState, clusterBuilder *cmv1.ClusterBuilder) (*cmv1.ClusterBuilder, bool, error) { - shouldUpdateProxy := false - if (state.Proxy == nil && plan.Proxy != nil) || (state.Proxy != nil && plan.Proxy == nil) { - shouldUpdateProxy = true - } else if state.Proxy != nil && plan.Proxy != nil { - _, patchNoProxy := common.ShouldPatchString(state.Proxy.NoProxy, plan.Proxy.NoProxy) - _, patchHttpProxy := common.ShouldPatchString(state.Proxy.HttpProxy, plan.Proxy.HttpProxy) - _, patchHttpsProxy := common.ShouldPatchString(state.Proxy.HttpsProxy, plan.Proxy.HttpsProxy) - _, patchAdditionalTrustBundle := common.ShouldPatchString(state.Proxy.AdditionalTrustBundle, plan.Proxy.AdditionalTrustBundle) - if patchNoProxy || patchHttpProxy || patchHttpsProxy || patchAdditionalTrustBundle { - shouldUpdateProxy = true - } - } - - if shouldUpdateProxy { +func updateProxy(state, plan *ClusterRosaClassicState, clusterBuilder *cmv1.ClusterBuilder) (*cmv1.ClusterBuilder, error) { + if !reflect.DeepEqual(state.Proxy, plan.Proxy) { var err error + if plan.Proxy == nil { + plan.Proxy = &proxy.Proxy{} + } clusterBuilder, err = buildProxy(plan, clusterBuilder) if err != nil { - return nil, false, err + return nil, err } } - return clusterBuilder, shouldUpdateProxy, nil + return clusterBuilder, nil } + func updateNodes(ctx context.Context, state, plan *ClusterRosaClassicState, clusterBuilder *cmv1.ClusterBuilder) (*cmv1.ClusterBuilder, bool, error) { // Send request to update the cluster: shouldUpdateNodes := false @@ -1623,6 +1617,8 @@ func populateRosaClassicClusterState(ctx context.Context, object *cmv1.Cluster, if ok { state.Proxy.NoProxy = types.StringValue(noProxy) } + } else { + state.Proxy = nil } trustBundle, ok := object.GetAdditionalTrustBundle() diff --git a/provider/proxy/proxy_validator.go b/provider/proxy/proxy_validator.go index 035dcf7b..9b5b4b26 100644 --- a/provider/proxy/proxy_validator.go +++ b/provider/proxy/proxy_validator.go @@ -3,12 +3,9 @@ package proxy import ( "context" "fmt" + "github.com/hashicorp/terraform-plugin-framework/schema/validator" "github.com/hashicorp/terraform-plugin-framework/types/basetypes" - "github.com/openshift/rosa/pkg/helper" "github.com/terraform-redhat/terraform-provider-rhcs/provider/common" - "strings" - - "github.com/hashicorp/terraform-plugin-framework/schema/validator" ) // atLeastValidator validates that an integer Attribute's value is at least a certain value. @@ -40,8 +37,6 @@ func (v proxyValidator) ValidateObject(ctx context.Context, request validator.Ob errSum := "Invalid proxy's attribute assignment" httpsProxy := "" httpProxy := "" - additionalTrustBundle := "" - var noProxySlice []string if !common.IsStringAttributeEmpty(proxy.HttpProxy) { httpProxy = proxy.HttpProxy.ValueString() @@ -49,22 +44,11 @@ func (v proxyValidator) ValidateObject(ctx context.Context, request validator.Ob if !common.IsStringAttributeEmpty(proxy.HttpsProxy) { httpsProxy = proxy.HttpsProxy.ValueString() } - if !common.IsStringAttributeEmpty(proxy.NoProxy) { - noProxySlice = helper.HandleEmptyStringOnSlice(strings.Split(proxy.NoProxy.ValueString(), ",")) - } - if !common.IsStringAttributeEmpty(proxy.AdditionalTrustBundle) { - additionalTrustBundle = proxy.AdditionalTrustBundle.ValueString() - } - - if httpProxy == "" && httpsProxy == "" && noProxySlice != nil && len(noProxySlice) > 0 { + if httpProxy == "" && httpsProxy == "" { response.Diagnostics.AddError(errSum, "Expected at least one of the following: http-proxy, https-proxy") return } - if httpProxy == "" && httpsProxy == "" && additionalTrustBundle == "" { - response.Diagnostics.AddError(errSum, "Expected at least one of the following: http-proxy, https-proxy, additional-trust-bundle") - return - } } func ProxyValidator() validator.Object { diff --git a/subsystem/cluster_resource_rosa_test.go b/subsystem/cluster_resource_rosa_test.go index 023c8230..19f08549 100644 --- a/subsystem/cluster_resource_rosa_test.go +++ b/subsystem/cluster_resource_rosa_test.go @@ -1561,7 +1561,7 @@ var _ = Describe("rhcs_cluster_rosa_classic - create", func() { Expect(resource).To(MatchJQ(`.attributes.proxy.no_proxy`, "test")) Expect(resource).To(MatchJQ(`.attributes.proxy.additional_trust_bundle`, "123")) }) - It("Creates cluster without http proxy and update trust bundle", func() { + It("Creates cluster without http proxy and update trust bundle - should fail", func() { // Prepare the server: server.AppendHandlers( CombineHandlers( @@ -1692,9 +1692,7 @@ var _ = Describe("rhcs_cluster_rosa_classic - create", func() { } } `) - Expect(terraform.Apply()).To(BeZero()) - resource := terraform.Resource("rhcs_cluster_rosa_classic", "my_cluster") - Expect(resource).To(MatchJQ(`.attributes.proxy.additional_trust_bundle`, "123")) + Expect(terraform.Apply()).ToNot(BeZero()) }) }) It("Creates cluster with default_mp_labels and update them", func() { @@ -1889,23 +1887,23 @@ var _ = Describe("rhcs_cluster_rosa_classic - create", func() { // Expected at least one of the following: http-proxy, https-proxy, additional-trust-bundle terraform.Source(` - resource "rhcs_cluster_rosa_classic" "my_cluster" { - name = "my-cluster" - cloud_region = "us-west-1" - aws_account_id = "123" - proxy = { - } - sts = { - operator_role_prefix = "test" - role_arn = "", - support_role_arn = "", - instance_iam_roles = { - master_role_arn = "", - worker_role_arn = "", + resource "rhcs_cluster_rosa_classic" "my_cluster" { + name = "my-cluster" + cloud_region = "us-west-1" + aws_account_id = "123" + proxy = { } - } - } - `) + sts = { + operator_role_prefix = "test" + role_arn = "", + support_role_arn = "", + instance_iam_roles = { + master_role_arn = "", + worker_role_arn = "", + } + } + } + `) Expect(terraform.Apply()).NotTo(BeZero()) }) It("Creates private cluster with aws subnet ids without private link", func() {