eddycharly / terraform-provider-kops

Brings kOps into terraform in a fully managed way
Apache License 2.0
85 stars 20 forks source link

Setting tag_subnets to false doesn't seem to have an effect #597

Closed dkulchinsky closed 2 years ago

dkulchinsky commented 2 years ago

Hey @eddycharly πŸ‘‹πŸΌ

Testing the latest provider version (1.23.4) in our sandbox, previously was 1.21.4

I changed disable_subnet_tags = true to tag_subnets = false as per the change in 1.23 and the diff looked correct, however after updating the cluster I noticed the tags were added to the subnets and when I checked the cluster spec it was missing tagSubnets: false attribute

❯ kops get cluster -oyaml|grep tagSubnets

any ideas?

eddycharly commented 2 years ago

Thanks for reporting, I will look at it asap. Remember that the provider does not manage subnets but expects the network to be created externally.

IIRC it should touch subnets at all.

dkulchinsky commented 2 years ago

Thanks @eddycharly, indeed the subnets are precreated by our Terraform code.

I believe kops is tagging the subnets you provide to it if tag_subnets is set to true (which is the default), we tag them ourselves and that why we set tag_subnets to false, but looks like the provider doesn't set this option in the cluster spec?

eddycharly commented 2 years ago

Yeah right. It’s a pointer in the kOps struct and there false or null will always yield to nil when expanding. As kOps defaults to true, it will always end up with true regardless of your tf config.

I will make a fix tomorrow. Would you be ok to test a beta release ?

dkulchinsky commented 2 years ago

Yeah right. It’s a pointer in the kOps struct and there false or null will always yield to nil when expanding. As kOps defaults to true, it will always end up with true regardless of your tf config.

I will make a fix tomorrow. Would you be ok to test a beta release ?

sure thing @eddycharly!

eddycharly commented 2 years ago

@dkulchinsky v1.23.5-alpha.1 is being released, let me know if you try it out.

dkulchinsky commented 2 years ago

Thanks @eddycharly! will give it a go πŸ‘πŸΌ

Do I understand correctly that this now needs to be defined as:

tag_subnets {
  value = false
}

I may need some time to get back with results as I've hit another pickle with a change that was recently introduced in kops 1.22.4, where etcd-manager will bind itself it an IPv6 address if one is present on the node and will resolve peers to their IPv6 IPs (if they have them), we indeed had IPv6 addresses assigned to our nodes (no particular reason, we just wanted to be ipv6 ready) and now the new master that comes up tries to connect to the other etcd members using their IPv6 addresses and fails (since they bind to the IPv4 IP) 😒

anyway - just figured I'd share this and will hopefully figure out a way forward and let you know how the above works.

dkulchinsky commented 2 years ago

Hey @eddycharly

Trying 1.23.5-alpha.1 now and hitting this issue:

β”‚ Error: missing expected [
β”‚
β”‚   with module.kubernetes_kops_aws_cluster_awsuse2["sbx-awsuse2"].kops_cluster.cluster,
β”‚   on ../modules/kubernetes-cluster/kops-aws/kops.tf line 15, in resource "kops_cluster" "cluster":
β”‚   15: resource "kops_cluster" "cluster" {

was also reported in https://github.com/eddycharly/terraform-provider-kops/pull/601#issuecomment-1138690576

eddycharly commented 2 years ago

Is this with an existing cluster ?

dkulchinsky commented 2 years ago

Is this with an existing cluster ?

yes, a cluster that was deployed with provider version 1.21.4

we're trying to upgrade to latest provider version and K8s 1.22.8

eddycharly commented 2 years ago

@dkulchinsky v1.23.5-alpha.2 is being released, with the fix from #603. I would apreciate if you can try it out.

dkulchinsky commented 2 years ago

Thanks @eddycharly! plan looks solid with v1.23.5-alpha.2!

eddycharly commented 2 years ago

πŸ‘ thanks !