terraform-kops / terraform-provider-kops

Brings kOps into terraform in a fully managed way
https://registry.terraform.io/providers/terraform-kops/kops/latest
Apache License 2.0
10 stars 4 forks source link

How to handle attributes that needs to be included with an empty string? #68

Closed dkulchinsky closed 5 months ago

dkulchinsky commented 5 months ago

Hey folks ๐Ÿ‘‹๐Ÿผ

This is a follow-up on #46, I was able to swap the gossip protocols and it worked as expected.

Now, I'm trying to disable the secondary protocol, for this the generated kops config should look like this:

  gossipConfig:
    protocol: memberlist
    listen: 0.0.0.0:4000
    secondary:
      protocol: ""

  dnsControllerGossipConfig:
    protocol: memberlist
    listen: 0.0.0.0:3993
    seed: 127.0.0.1:4000
    secondary:
      protocol: ""

notice the secondary empty string value for the protocol key.

with the current provider code, if I set the config to this:

  gossip_config {
    protocol = "memberlist"
    listen   = "0.0.0.0:4000"
    secondary {
      protocol = ""
    }
  }

  dns_controller_gossip_config {
    protocol = "memberlist"
    listen   = "0.0.0.0:3993"
    seed     = "127.0.0.1:4000"
    secondary {
      protocol = ""
    }
  }

The terraform plan diff looks correct: image

However, the produced kops config is rendered with an empty secondary block:

  dnsControllerGossipConfig:
    listen: 0.0.0.0:3993
    protocol: memberlist
    secondary: {}
    seed: 127.0.0.1:4000

  gossipConfig:
    listen: 0.0.0.0:4000
    protocol: memberlist
    secondary: {}

while I was expecting:

  dnsControllerGossipConfig:
    listen: 0.0.0.0:3993
    protocol: memberlist
    secondary:
      protocol: ""
    seed: 127.0.0.1:4000

  gossipConfig:
    listen: 0.0.0.0:4000
    protocol: memberlist
    secondary:
      protocol: ""

Any suggestion on how to handle this case?

eddycharly commented 5 months ago

@dkulchinsky does it changes something ? what is the go type for protocol ?

eddycharly commented 5 months ago

Ok, i think this is where terraform really is a pain. Basically there's no way to know if a specific value was provided in the config or not.

eddycharly commented 5 months ago

IIRC that's why i created the nullable helper. Can you try with:

generate(kops.GossipConfigSecondary{},
  nullable("Protocol"),
),
generate(kops.DNSControllerGossipConfigSecondary{},
  nullable("Protocol"),
),
dkulchinsky commented 5 months ago

IIRC that's why i created the nullable helper. Can you try with:

generate(kops.GossipConfigSecondary{},
  nullable("Protocol"),
),
generate(kops.DNSControllerGossipConfigSecondary{},
  nullable("Protocol"),
),

ohh!! that looks promising ๐Ÿ‘๐Ÿผ

I'll give it a go ๐Ÿคž๐Ÿผ

dkulchinsky commented 5 months ago

@eddycharly so I'm testing this out with the change in #70

now the produced kops config is:

  dnsControllerGossipConfig:
    listen: 0.0.0.0:3993
    protocol: memberlist
    secondary:
      protocol: "false"
    seed: 127.0.0.1:4000

  gossipConfig:
    listen: 0.0.0.0:4000
    protocol: memberlist
    secondary:
      protocol: "false"

so it renders as protocol: "false" instead of protocol: "" ๐Ÿ˜ž

The terraform code I used:

  gossip_config {
    protocol = "memberlist"
    listen   = "0.0.0.0:4000"
    secondary {
      protocol {
        value = false
      }
    }
  }

  dns_controller_gossip_config {
    protocol = "memberlist"
    listen   = "0.0.0.0:3993"
    seed     = "127.0.0.1:4000"
    secondary {
      protocol {
        value = false
      }
    }
  }
dkulchinsky commented 5 months ago

@dkulchinsky does it changes something ? what is the go type for protocol ?

it's a string

https://github.com/kubernetes/kops/blob/fc910854e5c5a4a78e14e173dc523ed4c2f70f80/pkg/apis/kops/cluster.go#L991

https://github.com/kubernetes/kops/blob/fc910854e5c5a4a78e14e173dc523ed4c2f70f80/pkg/apis/kops/cluster.go#L1005

eddycharly commented 5 months ago

@dkulchinsky nope, it's a string pointer ;-)

eddycharly commented 5 months ago

The tf config should be something like this:

  gossip_config {
    protocol = "memberlist"
    listen   = "0.0.0.0:4000"
    secondary {
      protocol {
        value = ""
      }
    }
  }
eddycharly commented 5 months ago

To highlight the difference with nullable you can now do:

  gossip_config {
    protocol = "memberlist"
    listen   = "0.0.0.0:4000"
    secondary {
      protocol {
        value = ""
      }
    }
  }

Should produce:

  gossipConfig:
    listen: 0.0.0.0:4000
    protocol: memberlist
    secondary:
      protocol: ""

  gossip_config {
    protocol = "memberlist"
    listen   = "0.0.0.0:4000"
    secondary {}
  }

Should produce:

  gossipConfig:
    listen: 0.0.0.0:4000
    protocol: memberlist
    secondary: {}

That is, we can differentiate a value that is not set at all from a default value (which is not possible natively in a plugin).

dkulchinsky commented 5 months ago

@eddycharly thank you! this makes total sense!

and of course, it's a string pointer ๐Ÿคฆ๐Ÿผโ€โ™‚๏ธ

will report back once I had a chance to test it out as suggested.

dkulchinsky commented 5 months ago

@eddycharly thanks again for your help! can confirm with value = "" the correct kops config is now produced!

  gossip_config {
    protocol = "memberlist"
    listen   = "0.0.0.0:4000"
    secondary {
      protocol {
        value = ""
      }
    }
  }

  dns_controller_gossip_config {
    protocol = "memberlist"
    listen   = "0.0.0.0:3993"
    seed     = "127.0.0.1:4000"
    secondary {
      protocol {
        value = ""
      }
    }
  }

produces:

  dnsControllerGossipConfig:
    listen: 0.0.0.0:3993
    protocol: memberlist
    secondary:
      protocol: ""
    seed: 127.0.0.1:4000

  gossipConfig:
    listen: 0.0.0.0:4000
    protocol: memberlist
    secondary:
      protocol: ""
eddycharly commented 5 months ago

Awesome, thanks for checking it out!