Skip to content

Commit 4281558

Browse files
committed
fix: address v0.1.0 release review findings
- fix(client): clone http.DefaultTransport to avoid connection pool leak when TLS skip-verify is enabled (closes #112) - fix(platform): require credentials for external TimeSeries/Search instead of silently auto-generating random ones (closes #113) - fix(platform): tighten nil-server guard to only apply to internal components; external components without a Server are valid (closes #114) - fix(backups): log validation error before early return on mutual exclusion of tables/keyspaces (closes #115) - test: update platform controller tests to assert MissingExternalCredentials condition for external components with no auth configured
1 parent fabf2d3 commit 4281558

4 files changed

Lines changed: 125 additions & 28 deletions

File tree

internal/axonops/client.go

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -191,10 +191,23 @@ func NewClient(host, protocol, orgID, apiKey, tokenType string, tlsSkipVerify bo
191191

192192
baseURL := fmt.Sprintf("%s://%s", protocol, host)
193193

194-
var transport http.RoundTripper = &http.Transport{
195-
TLSClientConfig: &tls.Config{
196-
InsecureSkipVerify: tlsSkipVerify,
197-
},
194+
// Clone http.DefaultTransport to inherit connection pool settings (MaxIdleConns,
195+
// IdleConnTimeout, TLSHandshakeTimeout, DialContext, etc.) while overriding only
196+
// the TLS config. A bare &http.Transport{} has no idle-connection timeout, so
197+
// abandoned transports (one is created per reconcile) accumulate open sockets.
198+
var transport http.RoundTripper
199+
if dt, ok := http.DefaultTransport.(*http.Transport); ok {
200+
cloned := dt.Clone()
201+
cloned.TLSClientConfig = &tls.Config{
202+
InsecureSkipVerify: tlsSkipVerify, //nolint:gosec // controlled by user-facing field TLSSkipVerify
203+
}
204+
transport = cloned
205+
} else {
206+
transport = &http.Transport{
207+
TLSClientConfig: &tls.Config{
208+
InsecureSkipVerify: tlsSkipVerify, //nolint:gosec
209+
},
210+
}
198211
}
199212
if o.verbose {
200213
transport = &verboseTransport{base: transport}

internal/controller/axonopsplatform_controller.go

Lines changed: 75 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -331,6 +331,37 @@ func (r *AxonOpsPlatformReconciler) Reconcile(ctx context.Context, req ctrl.Requ
331331
}
332332
}
333333

334+
// Validate that internal database components are not enabled without the server component.
335+
// External components (with spec.*.external.hosts) are already-running services; the operator
336+
// only writes connection references for them, so they do not require a Server to be present.
337+
// Internal (operator-managed) TimeSeries and Search exist solely to be consumed by the Server;
338+
// enabling them without a Server produces a confusing partial state.
339+
internalTSEnabled := r.isComponentEnabled(server.Spec.TimeSeries) && !isTimeSeriesExternal(server)
340+
internalSearchEnabled := r.isComponentEnabled(server.Spec.Search) && !isSearchExternal(server)
341+
if !r.isComponentEnabled(server.Spec.Server) && (internalTSEnabled || internalSearchEnabled) {
342+
var components []string
343+
if internalTSEnabled {
344+
components = append(components, "timeSeries")
345+
}
346+
if internalSearchEnabled {
347+
components = append(components, "search")
348+
}
349+
msg := fmt.Sprintf("spec.server must be configured when %s is enabled", strings.Join(components, " and "))
350+
log.Info("Invalid configuration: database components require server", "components", components)
351+
meta.SetStatusCondition(&server.Status.Conditions, metav1.Condition{
352+
Type: "Ready",
353+
Status: metav1.ConditionFalse,
354+
ObservedGeneration: server.Generation,
355+
Reason: "InvalidConfiguration",
356+
Message: msg,
357+
})
358+
server.Status.ObservedGeneration = server.Generation
359+
if err := r.Status().Update(ctx, server); err != nil {
360+
log.Error(err, "Failed to update status for invalid configuration")
361+
}
362+
return ctrl.Result{}, nil
363+
}
364+
334365
// Verify cert-manager CRDs are available (only needed for internal database/workload resources)
335366
if needsInternalResources(server) {
336367
if !r.isCertManagerAvailable() {
@@ -397,10 +428,30 @@ func (r *AxonOpsPlatformReconciler) Reconcile(ctx context.Context, req ctrl.Requ
397428
return ctrl.Result{}, err
398429
}
399430

431+
// External databases require explicit credentials — auto-generating random
432+
// credentials would cause permanent authentication failures since the operator
433+
// cannot know what credentials the existing cluster was provisioned with.
434+
tsAuth := server.Spec.TimeSeries.Authentication
435+
if tsAuth.SecretRef == "" && tsAuth.Username == "" {
436+
msg := "external TimeSeries requires credentials: set spec.timeSeries.authentication.secretRef or spec.timeSeries.authentication.username/password"
437+
log.Info("Missing external TimeSeries credentials", "message", msg)
438+
meta.SetStatusCondition(&server.Status.Conditions, metav1.Condition{
439+
Type: "Ready",
440+
Status: metav1.ConditionFalse,
441+
ObservedGeneration: server.Generation,
442+
Reason: "MissingExternalCredentials",
443+
Message: msg,
444+
})
445+
server.Status.ObservedGeneration = server.Generation
446+
if statusErr := r.Status().Update(ctx, server); statusErr != nil {
447+
log.Error(statusErr, "Failed to update status")
448+
}
449+
return ctrl.Result{RequeueAfter: 30 * time.Second}, nil
450+
}
451+
400452
// Ensure authentication secret for external TimeSeries.
401453
// ensureAuthenticationSecret handles all cases: SecretRef (validates it),
402-
// inline username/password (creates a managed Secret), and no credentials
403-
// (auto-generates into a managed Secret).
454+
// inline username/password (creates a managed Secret).
404455
var err error
405456
timeSeriesSecretName, _, err = r.ensureAuthenticationSecret(ctx, server, componentTimeseries, server.Spec.TimeSeries.Authentication, server.Spec.TimeSeries.StorageConfig)
406457
if err != nil {
@@ -447,10 +498,30 @@ func (r *AxonOpsPlatformReconciler) Reconcile(ctx context.Context, req ctrl.Requ
447498
return ctrl.Result{}, err
448499
}
449500

501+
// External databases require explicit credentials — auto-generating random
502+
// credentials would cause permanent authentication failures since the operator
503+
// cannot know what credentials the existing cluster was provisioned with.
504+
searchAuth := server.Spec.Search.Authentication
505+
if searchAuth.SecretRef == "" && searchAuth.Username == "" {
506+
msg := "external Search requires credentials: set spec.search.authentication.secretRef or spec.search.authentication.username/password"
507+
log.Info("Missing external Search credentials", "message", msg)
508+
meta.SetStatusCondition(&server.Status.Conditions, metav1.Condition{
509+
Type: "Ready",
510+
Status: metav1.ConditionFalse,
511+
ObservedGeneration: server.Generation,
512+
Reason: "MissingExternalCredentials",
513+
Message: msg,
514+
})
515+
server.Status.ObservedGeneration = server.Generation
516+
if statusErr := r.Status().Update(ctx, server); statusErr != nil {
517+
log.Error(statusErr, "Failed to update status")
518+
}
519+
return ctrl.Result{RequeueAfter: 30 * time.Second}, nil
520+
}
521+
450522
// Ensure authentication secret for external Search.
451523
// ensureAuthenticationSecret handles all cases: SecretRef (validates it),
452-
// inline username/password (creates a managed Secret), and no credentials
453-
// (auto-generates into a managed Secret).
524+
// inline username/password (creates a managed Secret).
454525
var err error
455526
searchSecretName, _, err = r.ensureAuthenticationSecret(ctx, server, componentSearch, server.Spec.Search.Authentication, server.Spec.Search.StorageConfig)
456527
if err != nil {

internal/controller/axonopsplatform_controller_test.go

Lines changed: 28 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,7 @@ var _ = Describe("AxonOpsPlatform Controller", func() {
214214
Expect(k8sClient.Delete(ctx, searchSecret)).To(Succeed())
215215
})
216216

217-
It("should auto-generate credentials when external Search has no auth configured", func() {
217+
It("should set MissingExternalCredentials condition when external Search has no auth configured", func() {
218218
const resourceName = "external-search-no-auth"
219219
ctx := context.Background()
220220
typeNamespacedName := types.NamespacedName{
@@ -236,7 +236,7 @@ var _ = Describe("AxonOpsPlatform Controller", func() {
236236
Hosts: []string{"https://opensearch.example.com:9200"},
237237
},
238238
Authentication: corev1alpha1.AxonAuthentication{
239-
// No SecretRef or Username - credentials will be auto-generated
239+
// No SecretRef or Username
240240
},
241241
},
242242
},
@@ -257,20 +257,24 @@ var _ = Describe("AxonOpsPlatform Controller", func() {
257257
})
258258
Expect(err).NotTo(HaveOccurred())
259259

260-
By("Verifying that a managed auth secret was created with auto-generated credentials")
260+
By("Verifying that Ready=False with MissingExternalCredentials is set")
261+
updatedResource := &corev1alpha1.AxonOpsPlatform{}
262+
Expect(k8sClient.Get(ctx, typeNamespacedName, updatedResource)).To(Succeed())
263+
cond := meta.FindStatusCondition(updatedResource.Status.Conditions, "Ready")
264+
Expect(cond).NotTo(BeNil())
265+
Expect(cond.Status).To(Equal(metav1.ConditionFalse))
266+
Expect(cond.Reason).To(Equal("MissingExternalCredentials"))
267+
268+
By("Verifying that no managed auth secret was created")
261269
managedSecret := &corev1.Secret{}
262-
Expect(k8sClient.Get(ctx, types.NamespacedName{
270+
err = k8sClient.Get(ctx, types.NamespacedName{
263271
Name: resourceName + "-search-auth",
264272
Namespace: "default",
265-
}, managedSecret)).To(Succeed())
266-
Expect(managedSecret.Data).To(HaveKey("AXONOPS_SEARCH_USER"))
267-
Expect(managedSecret.Data).To(HaveKey("AXONOPS_SEARCH_PASSWORD"))
268-
Expect(string(managedSecret.Data["AXONOPS_SEARCH_USER"])).NotTo(BeEmpty())
269-
Expect(string(managedSecret.Data["AXONOPS_SEARCH_PASSWORD"])).NotTo(BeEmpty())
273+
}, managedSecret)
274+
Expect(errors.IsNotFound(err)).To(BeTrue())
270275

271276
By("Cleanup")
272277
Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
273-
Expect(k8sClient.Delete(ctx, managedSecret)).To(Succeed())
274278
})
275279

276280
It("should exclude search-tls volume when Search is external", func() {
@@ -441,7 +445,7 @@ var _ = Describe("AxonOpsPlatform Controller", func() {
441445
Expect(k8sClient.Delete(ctx, timeseriesSecret)).To(Succeed())
442446
})
443447

444-
It("should auto-generate credentials when external TimeSeries has no auth configured", func() {
448+
It("should set MissingExternalCredentials condition when external TimeSeries has no auth configured", func() {
445449
const resourceName = "external-timeseries-no-auth"
446450
ctx := context.Background()
447451
typeNamespacedName := types.NamespacedName{
@@ -463,7 +467,7 @@ var _ = Describe("AxonOpsPlatform Controller", func() {
463467
Hosts: []string{"cassandra-node1.example.com:9042"},
464468
},
465469
Authentication: corev1alpha1.AxonAuthentication{
466-
// No SecretRef or Username - credentials will be auto-generated
470+
// No SecretRef or Username
467471
},
468472
},
469473
},
@@ -484,20 +488,24 @@ var _ = Describe("AxonOpsPlatform Controller", func() {
484488
})
485489
Expect(err).NotTo(HaveOccurred())
486490

487-
By("Verifying that a managed auth secret was created with auto-generated credentials")
491+
By("Verifying that Ready=False with MissingExternalCredentials is set")
492+
updatedResource := &corev1alpha1.AxonOpsPlatform{}
493+
Expect(k8sClient.Get(ctx, typeNamespacedName, updatedResource)).To(Succeed())
494+
cond := meta.FindStatusCondition(updatedResource.Status.Conditions, "Ready")
495+
Expect(cond).NotTo(BeNil())
496+
Expect(cond.Status).To(Equal(metav1.ConditionFalse))
497+
Expect(cond.Reason).To(Equal("MissingExternalCredentials"))
498+
499+
By("Verifying that no managed auth secret was created")
488500
managedSecret := &corev1.Secret{}
489-
Expect(k8sClient.Get(ctx, types.NamespacedName{
501+
err = k8sClient.Get(ctx, types.NamespacedName{
490502
Name: resourceName + "-timeseries-auth",
491503
Namespace: "default",
492-
}, managedSecret)).To(Succeed())
493-
Expect(managedSecret.Data).To(HaveKey("AXONOPS_DB_USER"))
494-
Expect(managedSecret.Data).To(HaveKey("AXONOPS_DB_PASSWORD"))
495-
Expect(string(managedSecret.Data["AXONOPS_DB_USER"])).NotTo(BeEmpty())
496-
Expect(string(managedSecret.Data["AXONOPS_DB_PASSWORD"])).NotTo(BeEmpty())
504+
}, managedSecret)
505+
Expect(errors.IsNotFound(err)).To(BeTrue())
497506

498507
By("Cleanup")
499508
Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
500-
Expect(k8sClient.Delete(ctx, managedSecret)).To(Succeed())
501509
})
502510

503511
It("should exclude timeseries-tls volume when TimeSeries is external", func() {

internal/controller/backups/axonopsbackup_controller.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,8 @@ func (r *AxonOpsBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reques
108108

109109
// Validate mutual exclusivity of tables and keyspaces
110110
if len(backup.Spec.Tables) > 0 && len(backup.Spec.Keyspaces) > 0 {
111+
validationErr := fmt.Errorf("spec.tables and spec.keyspaces are mutually exclusive — specify one or neither")
112+
log.Error(validationErr, "Validation failed", "backup", req.NamespacedName)
111113
meta.SetStatusCondition(&backup.Status.Conditions, metav1.Condition{
112114
Type: "Failed",
113115
Status: metav1.ConditionTrue,
@@ -119,6 +121,9 @@ func (r *AxonOpsBackupReconciler) Reconcile(ctx context.Context, req ctrl.Reques
119121
if err := r.Status().Update(ctx, backup); err != nil {
120122
log.Error(err, "Failed to update status")
121123
}
124+
// Return nil (not the error) — this is a terminal user-error; returning an error
125+
// would trigger exponential back-off retries that can never self-resolve without
126+
// a spec change.
122127
return ctrl.Result{}, nil
123128
}
124129

0 commit comments

Comments
 (0)