Skip to content

fix: Adding missing extra_fqdn support for operator that was supported previously - #197

Merged
zacharyblasczyk merged 7 commits into
mainfrom
zacharyb/extra-fqdn-operator
Apr 17, 2024
Merged

fix: Adding missing extra_fqdn support for operator that was supported previously#197
zacharyblasczyk merged 7 commits into
mainfrom
zacharyb/extra-fqdn-operator

Conversation

@zacharyblasczyk

@zacharyblasczyk zacharyblasczyk commented Mar 29, 2024

Copy link
Copy Markdown
Contributor

This needs to go in after https://github.com/wandb/terraform-aws-wandb/pull/196/files

Given these vars as a constant:

domain_name   = "sandbox-aws.wandb.ml"
subdomain     = "smooth-operator"
extra_fqdn    = ["rough-operator.sandbox-aws.wandb.ml"]

I went from

module "wandb_infra" {
  source  = "wandb/wandb/aws"
  version = "4.7.0"

  #we have this set for  Woven Japan and wesfarmers
  enable_dummy_dns    = false 
  enable_operator_alb = false

to:

module "wandb_infra" {
  source = "github.com/wandb/terraform-aws-wandb.git?ref=zacharyb/extra-fqdn-operator"

  enable_dummy_dns    = false
  enable_operator_alb = false

with:

No changes. Your infrastructure matches the configuration.
Terraform has compared your real infrastructure against your configuration and found no differences, so no changes are needed.
Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

Step 2. Enable

Then I enabled:

  enable_dummy_dns    = true
  enable_operator_alb = true

module.wandb_infra.module.app_lb.aws_route53_record.alb gets replaced.

"smooth-operator.sandbox-aws.wandb.ml" -> "old-smooth-operator.sandbox-aws.wandb.ml"

module.wandb_infra.module.wandb.helm_release.wandb will be updated in-place with the following:

"external-dns.alpha.kubernetes.io/hostname": |
                        smooth-operator.sandbox-aws.wandb.ml\,rough-operator.sandbox-aws.wandb.ml

module.wandb_infra.module.app_eks.module.external_dns.helm_release.external_dns will get

      + set {
          + name  = "domainFilters[1]"
          + value = "rough-operator.sandbox-aws.wandb.ml"
        }

Outcome:

kubectl get ingress 
NAME    CLASS   HOSTS                                                                      ADDRESS                                                         PORTS   AGE
wandb   alb     smooth-operator.sandbox-aws.wandb.ml,rough-operator.sandbox-aws.wandb.ml   smooth-operator-alb-k8s-735018499.us-east-2.elb.amazonaws.com   80      2d21h
kubectl get po -n kube-system external-dns-569b49b6fb-9mz8v -oyaml | grep domain-filter  
    - --domain-filter=sandbox-aws.wandb.ml
    - --domain-filter=rough-operator.sandbox-aws.wandb.ml

@zacharyblasczyk
zacharyblasczyk requested a review from gls4 as a code owner March 29, 2024 19:57
Comment thread main.tf
length(var.extra_fqdn) > 0 && var.enable_dummy_dns ? {
"external-dns.alpha.kubernetes.io/hostname" = <<-EOF
${local.fqdn}\,${join("\\,", var.extra_fqdn)}\,${local.fqdn}
EOF

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reason I use the local.fqdn twice is because of a bug with external dns not reading this form of the annotation correctly:

      external-dns.alpha.kubernetes.io/hostname: |
        smooth-operator.sandbox-aws.wandb.ml,rough-operator.sandbox-aws.wandb.ml

In order to get around this, I added ${local.fqdn} to the end which works just perfectly. 😢

time="2024-03-29T21:23:33Z" level=debug msg="Endpoints generated from ingress: default/wandb: [smooth-operator.sandbox-aws.wandb.ml 0 IN CNAME  smooth-operator-alb-k8s-388333795.us-east-2.elb.amazonaws.com [] rough-operator.sandbox-aws.wandb.ml 0 IN CNAME  smooth-operator-alb-k8s-388333795.us-east-2.elb.amazonaws.com [] smooth-operator.sandbox-aws.wandb.ml\n 0 IN CNAME  smooth-operator-alb-k8s-388333795.us-east-2.elb.amazonaws.com []]"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That sure doesn't look perfect to me with a duplicated entry in the endpoints generated. Is there something goofy we're doing with writing out these annotations that it ends up in an incompatible format or is it just good ol' externaldns madness?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

External DNS madness. I am actually creating an issue now in https://github.com/kubernetes-sigs/external-dns/issues. Will comment when I create it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@adityachoudhari26 adityachoudhari26 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

approved but only merge after #196 is in

@zacharyblasczyk

Copy link
Copy Markdown
Contributor Author

approved but only merge after #196 is in

I believe @gls4 got this resolved a different way.

@zacharyblasczyk zacharyblasczyk changed the title feat: Adding extra_fqdn support for operator fix: Adding missing extra_fqdn support for operator that was supported previously Apr 16, 2024
@zacharyblasczyk
zacharyblasczyk requested a review from nfoucha April 16, 2024 20:28
@zacharyblasczyk
zacharyblasczyk merged commit 7adf420 into main Apr 17, 2024
@zacharyblasczyk
zacharyblasczyk deleted the zacharyb/extra-fqdn-operator branch April 17, 2024 15:16
jsbroks pushed a commit that referenced this pull request Apr 17, 2024
### [4.7.1](v4.7.0...v4.7.1) (2024-04-17)

### Bug Fixes

* Adding missing extra_fqdn support for operator that was supported previously ([#197](#197)) ([7adf420](7adf420))
@jsbroks

jsbroks commented Apr 17, 2024

Copy link
Copy Markdown
Contributor

This PR is included in version 4.7.1 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants