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.

Sign up for GitHub

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

Jump to bottom

Fix skipping not overridden labels uisng '--merge-labels' #1435

Merged
merged 1 commit into from
Oct 2, 2022
Merged

Fix skipping not overridden labels uisng '--merge-labels' #1435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion openstack/containerinfra_shared_v1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func expandContainerInfraV1LabelsString(v map[string]interface{}) (string, error
return formattedLabels, nil
}

func containerInfraV1GetLabelsMerged(labelsAdded map[string]string, labelsSkipped map[string]string, labelsOverridden map[string]string, labels map[string]string) map[string]string {
func containerInfraV1GetLabelsMerged(labelsAdded map[string]string, labelsSkipped map[string]string, labelsOverridden map[string]string, labels map[string]string, resourceDataLabels map[string]string) map[string]string {
m := make(map[string]string)
for key, val := range labelsAdded {
m[key] = val
Expand All @@ -71,6 +71,12 @@ func containerInfraV1GetLabelsMerged(labelsAdded map[string]string, labelsSkippe
// We have to get the actual value here, not the one overridden
m[key] = labels[key]
}
// If defined resource's labels don't override label (are the same)
for key, val := range resourceDataLabels {
if _, exist := m[key]; !exist {
m[key] = val
}
}
return m
}

Expand Down
6 changes: 5 additions & 1 deletion openstack/resource_openstack_containerinfra_cluster_v1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,11 @@ func resourceContainerInfraClusterV1Read(_ context.Context, d *schema.ResourceDa

labels := s.Labels
if d.Get("merge_labels").(bool) {
labels = containerInfraV1GetLabelsMerged(s.LabelsAdded, s.LabelsSkipped, s.LabelsOverridden, s.Labels)
resourceDataLabels, err := expandContainerInfraV1LabelsMap(d.Get("labels").(map[string]interface{}))
if err != nil {
return diag.FromErr(err)
}
labels = containerInfraV1GetLabelsMerged(s.LabelsAdded, s.LabelsSkipped, s.LabelsOverridden, s.Labels, resourceDataLabels)
Copy link
Contributor

@MrFreezeex MrFreezeex Sep 2, 2022 •
edited

Choose a reason for hiding this comment

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

Also in what case you would need to get the labels from the state? It seems to me that with this change if you would want to delete a label it would not consider that the cluster changed.

But since I am not sure what you are trying to fix exactly, perhaps I missed something.

Copy link
Contributor Author

@pawcykca pawcykca Sep 2, 2022 •
edited

Choose a reason for hiding this comment

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

This code fixes issue described in #1327.

Currently when you create cluster and nodegroup using 'merge_label' option with labels that looks like below:

$ cat cluster.tf 
resource "openstack_containerinfra_cluster_v1" "cluster_1" {
  ...
  merge_labels        = true
  labels = {
    # unchanged (the same as in the cluster template)
    availability_zone = "zone1"
    # overridden
    boot_volume_size = 15
    # added
    boot_volume_type = "gp-fast"
  }

$ cat nodegroup.tf 
resource "openstack_containerinfra_nodegroup_v1" "nodegroup" {
  ...
  merge_labels        = true
  labels = {
    # unchanged (the same as in the cluster template)
    availability_zone = "zone1"
    # overridden
    boot_volume_size = 20
    # added
    docker_volume_type = "gp-slow"
  }

and perform again apply operation (without any changes in .tf files) then TF will try to recreate both cluster and nodegroup again

Terraform will perform the following actions:

  # openstack_containerinfra_cluster_v1.cluster_1 must be replaced
-/+ resource "openstack_containerinfra_cluster_v1" "cluster_1" {
	  ...
      ~ labels              = { # forces replacement
          + "availability_zone" = "zone1"
            # (2 unchanged elements hidden)
        }
      ...
        # (7 unchanged attributes hidden)
    }

  # openstack_containerinfra_nodegroup_v1.nodegroup must be replaced
-/+ resource "openstack_containerinfra_nodegroup_v1" "nodegroup" {
	  ...
      ~ labels             = { # forces replacement
          + "availability_zone"  = "zone1"
            # (2 unchanged elements hidden)
        }
      ...
        # (6 unchanged attributes hidden)
    }

Plan: 2 to add, 0 to change, 2 to destroy.

This is because defined labels that are the same as in the cluster template are not stored in cluster/nodegroup object by Openstack API.

Copy link
Contributor

@MrFreezeex MrFreezeex Sep 2, 2022 •
edited

Choose a reason for hiding this comment

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

Weird I would expect to find those labels in labels skipped or even overridden somehow... But by reading the state of terrwform I don't think that you are able to remove an existing labels anymore... Could you test this scenario? If it's indeed not returned anywhere in the Openstack cluster API, it would probably require fetching the labels from the cluster template unfortunately...

Copy link
Contributor Author

@pawcykca pawcykca Sep 2, 2022 •
edited

Choose a reason for hiding this comment

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

Weird I would expect to find those labels in labels skipped or even overridden somehow...

This is mapped as below

  merge_labels        = true
  labels = {
    # unchanged (the same as in the cluster template) -> not stored in any field on Magnum API
    availability_zone = "zone1"
    # overridden -> stored in 'labels_overridden'
    boot_volume_size = 15
    # added -> stored in 'labels_added'
    boot_volume_type = "gp-fast"
  }
-------
  merge_labels        = false
  labels = {
    ...
    # not copied/inherited from cluster's labels or commented (as below)-> stored in 'labels_skipped'
    #boot_volume_type = "gp-fast"
  }

But by reading the state of terrwform I don't think that you are able to remove an existing labels anymore... Could you test this scenario?

Works fine for each type of below labels

  merge_labels        = true
  labels = {
    # unchanged (the same as in the cluster template)
    # availability_zone = "zone1"
    # overridden
    #boot_volume_size = 15
    # added
    #boot_volume_type = "gp-fast"
  }

Terraform will perform the following actions:

  # openstack_containerinfra_nodegroup_v1.nodegroup must be replaced
-/+ resource "openstack_containerinfra_nodegroup_v1" "nodegroup" {
     ...
      ~ labels             = {
          - "availability_zone"  = "zone1"
          - "boot_volume_size"   = "20"
          - "docker_volume_type" = "gp-fast"
        } -> (known after apply) # forces replacement
     ...
        # (7 unchanged attributes hidden)
    }

Plan: 1 to add, 0 to change, 1 to destroy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok I see then it should work indeed! Thanks :).

}
if err := d.Set("labels", labels); err != nil {
return diag.Errorf("Unable to set openstack_containerinfra_cluster_v1 labels: %s", err)
Expand Down
6 changes: 5 additions & 1 deletion openstack/resource_openstack_containerinfra_nodegroup_v1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,11 @@ func resourceContainerInfraNodeGroupV1Read(_ context.Context, d *schema.Resource

labels := nodeGroup.Labels
if d.Get("merge_labels").(bool) {
labels = containerInfraV1GetLabelsMerged(nodeGroup.LabelsAdded, nodeGroup.LabelsSkipped, nodeGroup.LabelsOverridden, nodeGroup.Labels)
resourceDataLabels, err := expandContainerInfraV1LabelsMap(d.Get("labels").(map[string]interface{}))
if err != nil {
return diag.FromErr(err)
}
labels = containerInfraV1GetLabelsMerged(nodeGroup.LabelsAdded, nodeGroup.LabelsSkipped, nodeGroup.LabelsOverridden, nodeGroup.Labels, resourceDataLabels)
}
if err := d.Set("labels", labels); err != nil {
return diag.Errorf("Unable to set openstack_containerinfra_nodegroup_v1 labels: %s", err)
Expand Down