GoogleCloudPlatform / k8s-config-connector

GCP Config Connector, a Kubernetes add-on for managing GCP resources
https://cloud.google.com/config-connector/docs/overview
Apache License 2.0
898 stars 231 forks source link

Allow DNSRecordSet rrdata to merge #635

Open zevisert opened 2 years ago

zevisert commented 2 years ago

DNSRecordSet manifests compete for the same hostname, overwriting eachother with every control loop. I would like to split complex records (especially DNS TXT) into multiple manifests.

I would like these two expressions of state to be the same:

Case 1

# Multiple manifests split across some N files.
apiVersion: dns.cnrm.cloud.google.com/v1beta1
kind: DNSRecordSet
metadata:
  annotations:
    cnrm.cloud.google.com/project-id: my-project
  name: example-com-txt-one
  namespace: default
spec:
  managedZoneRef:
    name: example-com-zone
  name: example.com.
  rrdatas:
  - '"TXT Record 1"'
  ttl: 5
  type: TXT
---
apiVersion: dns.cnrm.cloud.google.com/v1beta1
kind: DNSRecordSet
metadata:
  annotations:
    cnrm.cloud.google.com/project-id: my-project
  name: example-com-txt-two
  namespace: default
spec:
  managedZoneRef:
    name: example-com-zone
  name: example.com.
  rrdatas:
  - '"TXT Record 2"'
  ttl: 5
  type: TXT

The record switches back and forth:

$ dig example.com TXT +nocmd +nocomments +nostats +noshort               
;example.com.                     IN      TXT
example.com.              0       IN      TXT     "TXT Record 1"
$ sleep 5
$ dig example.com TXT +nocmd +nocomments +nostats +noshort               
;example.com.                     IN      TXT
example.com.              0       IN      TXT     "TXT Record 2"

Case 2

But I currently have to do this:

#  Combine all records into one manifest
apiVersion: dns.cnrm.cloud.google.com/v1beta1
kind: DNSRecordSet
metadata:
  annotations:
    cnrm.cloud.google.com/project-id: my-project
  name: example-com-txt-combined
  namespace: default
spec:
  managedZoneRef:
    name: example-com-zone
  name: example.com.
  rrdatas:
  - '"TXT Record 1"'
  - '"TXT Record 2"'
  ttl: 5
  type: TXT
$ dig example.com TXT +nocmd +nocomments +nostats +noshort               
;example.com.                     IN      TXT
example.com.              0       IN      TXT     "TXT Record 1"
example.com.              0       IN      TXT     "TXT Record 2"

What actually happens

With separate manifests that target the same name, when one is reconciled it removes all records from the other. I would like for CNRM to only affect the records it created, and merge multiple TXT rrdatas for the same hostname into one record.

caieo commented 2 years ago

Hi @zevisert, thank you filing this enhancement request. We apologize for this lacking feature and any inconvenience that comes with it. While this is a very valid feature request, I think it would require a complicated design process to determine when the merging array values is applicable/allowed. For now, I'm not sure if this is something our team has the bandwidth to address, but I've added it into our queue of feature requests/design topics to investigate eventually.

zevisert commented 2 years ago

That's not surprising really. In that light, one thing that could help avoid the issues we encountered (our domain's SPF records were intermittently removed) would at least be for the Kubernetes admission controller to deny a new DNSRecordSet if the cluster already has one for a given name and type!

caieo commented 2 years ago

@zevisert, thank you for the good suggestion! We actually do have a way to handle resource conflicts (documentation here), but unfortunately, this feature does not work for resources that don't use metadata.name as the name of the GCP resource (i.e. resources that use resourceID or in your example's case spec.name). Instead of your suggestion, would something like our cnrm.cloud.google.com/management-conflict-prevention-policy annotation work instead?

zevisert commented 2 years ago

Hmm, thanks for the documentation link. I wasn't aware if that annotation. I'll take a look, try it out, and report back here.