feat: new CRD to manage multiple PowerDNS #204

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

Open

mydoomfr wants to merge 1 commit into powerdns-operator:develop from mydoomfr:feat-cluster

Contributor

mydoomfr commented Sep 1, 2025 •

edited

Loading

Manage multiple PowerDNS instances

Issue: #197

Introduce a PDNSProvider CRD to manage multiple PowerDNS instances.
Enhance TLS and credential management (including fetching data from Secrets/ConfigMaps).
Add .spec.providerRef (mandatory) property to Zone and ClusterZone.
Introduce breaking change with the bump of all CRD to v1alpha3

CRD

apiVersion: dns.cav.enablers.ob/v1alpha3
kind: PDNSProvider
metadata:
  name: powerdns1
spec:
  # Reconciliation interval - how often to check PowerDNS connection ; default: 5m
  interval: 3m
  url: https://powerdns.example.com:8081
  vhost: localhost
  timeout: 45s
  proxy: https://corporate-proxy.example.com:8080
  tls:
    insecure: false
    caBundleRef:
      name: powerdns-ca-bundle
      namespace: powerdns-operator-system
      kind: ConfigMap
      key: ca.crt
  credentials:
    secretRef:
      name: powerdns1
      namespace: powerdns1
      key: pdns-api-token

Remarks & Questions

The SDK exposes PowerDNS statistics; Maybe we could store them into cluster.status? https://github.com/joeig/go-powerdns/blob/main/statistics.go
~~Do we want to keep Cluster CRD name?~~
The Cluster.spec.interval is the reconciliation interval of the Cluster (controller) to check the connectivity to PowerDNS. This could be misunderstood as "how often Zones/RRsets are reconciled for this specific PowerDNS instance". We should make this explicit in the documentation. Also I'd like to rely on this interval as a kind of health check to prevent subresources reconciliation when the PowerDNS API is unavailable, using cluster.IsConnectionHealthy().
Should we keep the legacy configuration to avoid a breaking change? It adds a lot of code + documentation overhead and forces clusterRef to remain optional, even though Zone/ClusterZone requires it.
Check test coverage.
Write the documentation.

I rebased develop so we can merge this PR on it, then iterate with futur PR until we have something stable.
I'll squash my commits before merge.

mydoomfr added the enhancement label

mydoomfr changed the title ~~Feat Cluster~~ feat: new CRD to manage multiple PowerDNS

mydoomfr force-pushed the feat-cluster branch from 9468d02 to 612e8f5 Compare

September 9, 2025 08:59

mydoomfr force-pushed the feat-cluster branch 2 times, most recently from 84fe1d4 to 66329c8 Compare

September 19, 2025 09:47

Collaborator

antrema commented Sep 25, 2025

@mydoomfr What about PDNSServer or PDNSInstance as CRD name

mydoomfr force-pushed the feat-cluster branch from 66329c8 to 0c51f6b Compare

September 28, 2025 15:53

mydoomfr changed the base branch from develop to main

September 28, 2025 16:07

mydoomfr changed the base branch from main to develop

September 28, 2025 16:07

mydoomfr force-pushed the feat-cluster branch from 0c51f6b to 65d64e7 Compare

September 28, 2025 16:17


          feat!: add PDNSProvider CRD to manage multiple PowerDNS instances

9ed6413

Signed-off-by: Benjamin Pinchon <[email protected]>

mydoomfr force-pushed the feat-cluster branch from 65d64e7 to 9ed6413 Compare

September 28, 2025 17:56

Contributor Author

mydoomfr commented Sep 28, 2025 •

edited

Loading

@antrema Ready to be reviewed and merged into develop.
I have squashed all commits to keep the history clean.
There’s still a lot of work to do on tests, documentation, and on the controller reconcile loops.

Tests are passing and the code is running well, but we should double-check with more YAML samples.

PS: Renaming the CRD from Cluster to PDNSProvider was a real pain in the ass and I didn't expect that.

antrema reviewed

View reviewed changes

Collaborator

antrema left a comment

Well done, some remarks regarding terminology, after validating them, we should validate the PR and merge into develop, we will further add tests, metrics regarding new resource

api/v1alpha3/pdnsprovider_types.go

    
              // +kubebuilder:object:root=true

              // +kubebuilder:subresource:status

              // +kubebuilder:resource:scope=Cluster

Collaborator

antrema Oct 18, 2025

As discussed, we should probably consider namespace-scoped resource to better deal with fine-tuned authorization.
Example:
John DOE can only create a RRSet for PDNProvider #1 where Jane DOE can create on both PDNProvider #1 and PDNProvider #2

api/v1alpha3/pdnsprovider_types.go

    
              	// Namespace is the namespace of the Secret

              	// If not specified, defaults to the PDNSProvider resource namespace

              	// +optional

              	Namespace *string `json:"namespace,omitempty"`

Collaborator

antrema Oct 18, 2025

Does it still make sense to consider a namespace if PDNSProvider becomes a namespace-scoped resource, I don't have a strong opinion

api/v1alpha3/pdnsprovider_types.go

    
              // PDNSProviderTLSConfig defines TLS configuration for PowerDNS API connection

              type PDNSProviderTLSConfig struct {

              	// Insecure enables insecure connections to PowerDNS API (skip TLS verification)

Collaborator

antrema Oct 18, 2025

Suggested change

      
            	// Insecure enables insecure connections to PowerDNS API (skip TLS verification)
          
            	// Enables insecure connections to PowerDNS API (skip TLS verification), default to false

api/v1alpha3/pdnsprovider_types.go

    
              	// +optional

              	Insecure *bool `json:"insecure,omitempty"`

              	// CABundleRef is a reference to a ConfigMap or Secret containing a CA bundle

Collaborator

antrema Oct 18, 2025

Suggested change

      
            	// CABundleRef is a reference to a ConfigMap or Secret containing a CA bundle
          
            	// Reference to a ConfigMap or Secret containing a CA bundle

api/v1alpha3/pdnsprovider_types.go

    
              	// +kubebuilder:validation:Required

              	Name string `json:"name"`

              	// Namespace is the namespace of the ConfigMap or Secret

Collaborator

antrema Oct 18, 2025

Suggested change

      
            	// Namespace is the namespace of the ConfigMap or Secret
          
            	// Namespace containing the ConfigMap or Secret

config/samples/dns_v1alpha3_zone.yaml

    
            @@ -0,0 +1,64 @@
          
              ---

              # Specific Catalog

              apiVersion: dns.cav.enablers.ob/v1alpha2

Collaborator

antrema Oct 18, 2025

Suggested change

      
            apiVersion: dns.cav.enablers.ob/v1alpha2
          
            apiVersion: dns.cav.enablers.ob/v1alpha3

config/samples/dns_v1alpha3_zone.yaml

    
              ---

              # Specific SOA_EDIT_API

              apiVersion: dns.cav.enablers.ob/v1alpha2

Collaborator

antrema Oct 18, 2025

Suggested change

      
            apiVersion: dns.cav.enablers.ob/v1alpha2
          
            apiVersion: dns.cav.enablers.ob/v1alpha3

config/samples/dns_v1alpha3_zone.yaml

    
              ---

              # Fake Zone

              apiVersion: dns.cav.enablers.ob/v1alpha2

Collaborator

antrema Oct 18, 2025

Suggested change

      
            apiVersion: dns.cav.enablers.ob/v1alpha2
          
            apiVersion: dns.cav.enablers.ob/v1alpha3

config/samples/dns_v1alpha3_zone.yaml

    
              ---

              # Fake Zone

              apiVersion: dns.cav.enablers.ob/v1alpha2

Collaborator

antrema Oct 18, 2025

Suggested change

      
            apiVersion: dns.cav.enablers.ob/v1alpha2
          
            apiVersion: dns.cav.enablers.ob/v1alpha3

config/samples/kustomization.yaml

    
              ## Append samples of your project ##

              resources:

              - dns_v1alpha2_zone.yaml

              - dns_v1alpha2_rrset.yaml

Collaborator

antrema Oct 18, 2025

Any interest to keep "v1alpha2" sample resources ?
Missing v1alpha3 sample resources

antrema reviewed

View reviewed changes

Collaborator

antrema left a comment

Well done, some remarks regarding terminology, after validating them, we should validate the PR and merge into develop, we will further add tests, metrics regarding new resource

Collaborator

antrema commented Oct 18, 2025

The SDK exposes PowerDNS statistics; Maybe we could store them into cluster.status? https://github.com/joeig/go-powerdns/blob/main/statistics.go

Don't know what are these statistics but aren't there metrics ?

antrema reviewed

View reviewed changes

Collaborator

antrema left a comment

Today is another day, it comes with more comments ... Have a nice week-end ;-)

internal/controller/pdnsprovider_controller_test.go

    
              			// First reconcile might fail due to finalizer addition, retry if needed

              			const retryDelay = 100 * time.Millisecond

              			result, err := controllerReconciler.Reconcile(ctx, reconcile.Request{

Collaborator

antrema Oct 19, 2025

Why calling Reconcile function whereas PDNSProviderReconciler has been started on suite_test.go:

err = (&PDNSProviderReconciler{
	Client: k8sManager.GetClient(),
	Scheme: k8sManager.GetScheme(),
}).SetupWithManager(k8sManager)
Expect(err).ToNot(HaveOccurred())

internal/controller/pdnsprovider_controller_test.go

    
              		// PDNSProvider is cluster-scoped, so no namespace

              	}

              	Context("When reconciling a resource", func() {

Collaborator

antrema Oct 19, 2025

In other tests, we used to have Context after BeforeEach and AfterEach, I think we should have the same structure for all tests

internal/controller/pdnsprovider_controller.go

    
              // +kubebuilder:rbac:groups=dns.cav.enablers.ob,resources=pdnsproviders,verbs=get;list;watch;create;update;patch;delete

              // +kubebuilder:rbac:groups=dns.cav.enablers.ob,resources=pdnsproviders/status,verbs=get;update;patch

              // +kubebuilder:rbac:groups=dns.cav.enablers.ob,resources=pdnsproviders/finalizers,verbs=update

              // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch

Collaborator

antrema Oct 19, 2025

I think it is missing access to ConfigMap for CABundle stored as ConfigMap

internal/controller/pdns_client.go

    
              		Namespace: namespace,

              	}

              	if kind == "Secret" {

Collaborator

antrema Oct 19, 2025

"if/else" or "switch" structure ? I have no strong opinion

antrema reviewed

View reviewed changes

internal/controller/pdnsprovider_controller_test.go

    
              				return err == nil && updatedProvider.Status.Conditions != nil && len(updatedProvider.Status.Conditions) > 0

              			}, timeout, interval).Should(BeTrue())

              			_ = result // Use the result to avoid unused variable warnings

Collaborator

antrema Oct 19, 2025

I prefer the variable not to have been created instead

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

Labels