Skip to content

Commit 7fa58d9

Browse files
ThomasK33ThomasK33
committed
🤖 fix: harden license reconcile idempotency and status retries
Address Codex review findings by: - verifying coderd still reports at least one license before idempotent hash-based skip, so the controller re-uploads when backend license state is reset while Secret content is unchanged, - reading through `APIReader` during `RetryOnConflict` status updates so conflict retries are based on uncached latest resource versions. Also adds coverage for re-upload behavior when backend licenses are absent. --- _Generated with `mux` • Model: `openai:gpt-5.3-codex` • Thinking: `xhigh` • Cost: `$1.21`_ <!-- mux-attribution: model=openai:gpt-5.3-codex thinking=xhigh costs=1.21 -->
1 parent b5ba757 commit 7fa58d9

3 files changed

Lines changed: 197 additions & 27 deletions

File tree

‎internal/app/controllerapp/controllerapp.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ func SetupControllers(mgr manager.Manager) error {
8888

8989
reconciler := &controller.CoderControlPlaneReconciler{
9090
Client: client,
91+
APIReader: mgr.GetAPIReader(),
9192
Scheme: managerScheme,
9293
OperatorAccessProvisioner: coderbootstrap.NewPostgresOperatorAccessProvisioner(),
9394
LicenseUploader: controller.NewSDKLicenseUploader(),

‎internal/controller/codercontrolplane_controller.go‎

Lines changed: 104 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -69,9 +69,10 @@ var (
6969
errSecretValueEmpty = errors.New("secret value empty")
7070
)
7171

72-
// LicenseUploader uploads Coder Enterprise license JWTs to a coderd instance.
72+
// LicenseUploader uploads and inspects Coder Enterprise licenses in a coderd instance.
7373
type LicenseUploader interface {
7474
AddLicense(ctx context.Context, coderURL, sessionToken, licenseJWT string) error
75+
HasAnyLicense(ctx context.Context, coderURL, sessionToken string) (bool, error)
7576
}
7677

7778
// NewSDKLicenseUploader returns a LicenseUploader backed by codersdk.
@@ -82,19 +83,47 @@ func NewSDKLicenseUploader() LicenseUploader {
8283
type sdkLicenseUploader struct{}
8384

8485
func (u *sdkLicenseUploader) AddLicense(ctx context.Context, coderURL, sessionToken, licenseJWT string) error {
86+
if licenseJWT == "" {
87+
return fmt.Errorf("assertion failed: license JWT must not be empty")
88+
}
89+
90+
sdkClient, err := newSDKLicenseClient(coderURL, sessionToken)
91+
if err != nil {
92+
return err
93+
}
94+
95+
if _, err := sdkClient.AddLicense(ctx, codersdk.AddLicenseRequest{License: licenseJWT}); err != nil {
96+
return fmt.Errorf("upload coder license: %w", err)
97+
}
98+
99+
return nil
100+
}
101+
102+
func (u *sdkLicenseUploader) HasAnyLicense(ctx context.Context, coderURL, sessionToken string) (bool, error) {
103+
sdkClient, err := newSDKLicenseClient(coderURL, sessionToken)
104+
if err != nil {
105+
return false, err
106+
}
107+
108+
licenses, err := sdkClient.Licenses(ctx)
109+
if err != nil {
110+
return false, fmt.Errorf("list coder licenses: %w", err)
111+
}
112+
113+
return len(licenses) > 0, nil
114+
}
115+
116+
func newSDKLicenseClient(coderURL, sessionToken string) (*codersdk.Client, error) {
85117
if strings.TrimSpace(coderURL) == "" {
86-
return fmt.Errorf("assertion failed: coder URL must not be empty")
118+
return nil, fmt.Errorf("assertion failed: coder URL must not be empty")
87119
}
88120
if sessionToken == "" {
89-
return fmt.Errorf("assertion failed: session token must not be empty")
90-
}
91-
if licenseJWT == "" {
92-
return fmt.Errorf("assertion failed: license JWT must not be empty")
121+
return nil, fmt.Errorf("assertion failed: session token must not be empty")
93122
}
94123

95124
parsedURL, err := url.Parse(coderURL)
96125
if err != nil {
97-
return fmt.Errorf("parse coder URL: %w", err)
126+
return nil, fmt.Errorf("parse coder URL: %w", err)
98127
}
99128

100129
sdkClient := codersdk.New(parsedURL)
@@ -104,24 +133,21 @@ func (u *sdkLicenseUploader) AddLicense(ctx context.Context, coderURL, sessionTo
104133
}
105134
defaultTransport, ok := http.DefaultTransport.(*http.Transport)
106135
if !ok {
107-
return fmt.Errorf("assertion failed: http.DefaultTransport is not *http.Transport")
136+
return nil, fmt.Errorf("assertion failed: http.DefaultTransport is not *http.Transport")
108137
}
109138
// Use a dedicated transport to avoid sharing http.DefaultTransport's
110139
// connection pool across parallel test servers.
111140
sdkClient.HTTPClient.Transport = defaultTransport.Clone()
112141
sdkClient.HTTPClient.Timeout = licenseUploadRequestTimeout
113142

114-
if _, err := sdkClient.AddLicense(ctx, codersdk.AddLicenseRequest{License: licenseJWT}); err != nil {
115-
return fmt.Errorf("upload coder license: %w", err)
116-
}
117-
118-
return nil
143+
return sdkClient, nil
119144
}
120145

121146
// CoderControlPlaneReconciler reconciles a CoderControlPlane object.
122147
type CoderControlPlaneReconciler struct {
123148
client.Client
124-
Scheme *runtime.Scheme
149+
APIReader client.Reader
150+
Scheme *runtime.Scheme
125151

126152
OperatorAccessProvisioner coderbootstrap.OperatorAccessProvisioner
127153
LicenseUploader LicenseUploader
@@ -565,17 +591,62 @@ func (r *CoderControlPlaneReconciler) reconcileLicense(
565591
}
566592

567593
if nextStatus.LicenseLastApplied != nil && nextStatus.LicenseLastAppliedHash == licenseHash {
568-
if err := setControlPlaneCondition(
569-
nextStatus,
570-
coderControlPlane.Generation,
571-
coderv1alpha1.CoderControlPlaneConditionLicenseApplied,
572-
metav1.ConditionTrue,
573-
licenseConditionReasonApplied,
574-
"Configured license is already applied.",
575-
); err != nil {
576-
return ctrl.Result{}, err
594+
hasAnyLicense, hasLicenseErr := r.LicenseUploader.HasAnyLicense(ctx, nextStatus.URL, operatorToken)
595+
if hasLicenseErr != nil {
596+
var sdkErr *codersdk.Error
597+
if errors.As(hasLicenseErr, &sdkErr) {
598+
switch sdkErr.StatusCode() {
599+
case http.StatusNotFound:
600+
if err := setControlPlaneCondition(
601+
nextStatus,
602+
coderControlPlane.Generation,
603+
coderv1alpha1.CoderControlPlaneConditionLicenseApplied,
604+
metav1.ConditionFalse,
605+
licenseConditionReasonNotSupported,
606+
"Control plane does not expose the Enterprise licenses API.",
607+
); err != nil {
608+
return ctrl.Result{}, err
609+
}
610+
return ctrl.Result{}, nil
611+
case http.StatusUnauthorized, http.StatusForbidden:
612+
if err := setControlPlaneCondition(
613+
nextStatus,
614+
coderControlPlane.Generation,
615+
coderv1alpha1.CoderControlPlaneConditionLicenseApplied,
616+
metav1.ConditionFalse,
617+
licenseConditionReasonForbidden,
618+
"Operator token is not authorized to query configured licenses.",
619+
); err != nil {
620+
return ctrl.Result{}, err
621+
}
622+
return ctrl.Result{RequeueAfter: operatorAccessRetryInterval}, nil
623+
}
624+
}
625+
if err := setControlPlaneCondition(
626+
nextStatus,
627+
coderControlPlane.Generation,
628+
coderv1alpha1.CoderControlPlaneConditionLicenseApplied,
629+
metav1.ConditionFalse,
630+
licenseConditionReasonError,
631+
"Failed to query existing licenses; retrying.",
632+
); err != nil {
633+
return ctrl.Result{}, err
634+
}
635+
return ctrl.Result{RequeueAfter: operatorAccessRetryInterval}, nil
636+
}
637+
if hasAnyLicense {
638+
if err := setControlPlaneCondition(
639+
nextStatus,
640+
coderControlPlane.Generation,
641+
coderv1alpha1.CoderControlPlaneConditionLicenseApplied,
642+
metav1.ConditionTrue,
643+
licenseConditionReasonApplied,
644+
"Configured license is already applied.",
645+
); err != nil {
646+
return ctrl.Result{}, err
647+
}
648+
return ctrl.Result{}, nil
577649
}
578-
return ctrl.Result{}, nil
579650
}
580651

581652
if err := r.LicenseUploader.AddLicense(ctx, nextStatus.URL, operatorToken, licenseJWT); err != nil {
@@ -999,9 +1070,17 @@ func (r *CoderControlPlaneReconciler) reconcileStatus(
9991070
return fmt.Errorf("assertion failed: coder control plane namespaced name must not be empty")
10001071
}
10011072

1073+
statusReader := r.APIReader
1074+
if statusReader == nil {
1075+
statusReader = r.Client
1076+
}
1077+
if statusReader == nil {
1078+
return fmt.Errorf("assertion failed: status reader must not be nil")
1079+
}
1080+
10021081
if err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
10031082
latest := &coderv1alpha1.CoderControlPlane{}
1004-
if err := r.Get(ctx, namespacedName, latest); err != nil {
1083+
if err := statusReader.Get(ctx, namespacedName, latest); err != nil {
10051084
return err
10061085
}
10071086
if latest.Name != namespacedName.Name || latest.Namespace != namespacedName.Namespace {

‎internal/controller/codercontrolplane_controller_test.go‎

Lines changed: 92 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,8 +50,11 @@ type licenseUploadCall struct {
5050
}
5151

5252
type fakeLicenseUploader struct {
53-
err error
54-
calls []licenseUploadCall
53+
err error
54+
hasAnyLicenseErr error
55+
hasAnyLicense *bool
56+
hasAnyLicenseCall int
57+
calls []licenseUploadCall
5558
}
5659

5760
func (f *fakeLicenseUploader) AddLicense(_ context.Context, coderURL, sessionToken, licenseJWT string) error {
@@ -63,6 +66,18 @@ func (f *fakeLicenseUploader) AddLicense(_ context.Context, coderURL, sessionTok
6366
return f.err
6467
}
6568

69+
func (f *fakeLicenseUploader) HasAnyLicense(_ context.Context, _, _ string) (bool, error) {
70+
f.hasAnyLicenseCall++
71+
if f.hasAnyLicenseErr != nil {
72+
return false, f.hasAnyLicenseErr
73+
}
74+
if f.hasAnyLicense != nil {
75+
return *f.hasAnyLicense, nil
76+
}
77+
78+
return len(f.calls) > 0, nil
79+
}
80+
6681
func TestReconcile_NotFound(t *testing.T) {
6782
r := &controller.CoderControlPlaneReconciler{
6883
Client: k8sClient,
@@ -590,6 +605,81 @@ func TestReconcile_LicenseAppliesOnceAndTracksHash(t *testing.T) {
590605
}
591606
}
592607

608+
func TestReconcile_LicenseReuploadsWhenBackendHasNoLicenses(t *testing.T) {
609+
ctx := context.Background()
610+
611+
licenseSecret := &corev1.Secret{
612+
ObjectMeta: metav1.ObjectMeta{Name: "test-license-backend-reset-secret", Namespace: "default"},
613+
Data: map[string][]byte{
614+
coderv1alpha1.DefaultLicenseSecretKey: []byte("license-jwt-backend-reset"),
615+
},
616+
}
617+
if err := k8sClient.Create(ctx, licenseSecret); err != nil {
618+
t.Fatalf("create license secret: %v", err)
619+
}
620+
t.Cleanup(func() {
621+
_ = k8sClient.Delete(ctx, licenseSecret)
622+
})
623+
624+
cp := &coderv1alpha1.CoderControlPlane{
625+
ObjectMeta: metav1.ObjectMeta{Name: "test-license-backend-reset", Namespace: "default"},
626+
Spec: coderv1alpha1.CoderControlPlaneSpec{
627+
ExtraEnv: []corev1.EnvVar{{
628+
Name: "CODER_PG_CONNECTION_URL",
629+
Value: "postgres://example/license-backend-reset",
630+
}},
631+
LicenseSecretRef: &coderv1alpha1.SecretKeySelector{Name: licenseSecret.Name},
632+
},
633+
}
634+
if err := k8sClient.Create(ctx, cp); err != nil {
635+
t.Fatalf("create test CoderControlPlane: %v", err)
636+
}
637+
t.Cleanup(func() {
638+
_ = k8sClient.Delete(ctx, cp)
639+
})
640+
641+
provisioner := &fakeOperatorAccessProvisioner{token: "operator-token-backend-reset"}
642+
uploader := &fakeLicenseUploader{}
643+
r := &controller.CoderControlPlaneReconciler{
644+
Client: k8sClient,
645+
Scheme: scheme,
646+
OperatorAccessProvisioner: provisioner,
647+
LicenseUploader: uploader,
648+
}
649+
650+
if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: cp.Name, Namespace: cp.Namespace}}); err != nil {
651+
t.Fatalf("first reconcile control plane: %v", err)
652+
}
653+
deployment := &appsv1.Deployment{}
654+
if err := k8sClient.Get(ctx, types.NamespacedName{Name: cp.Name, Namespace: cp.Namespace}, deployment); err != nil {
655+
t.Fatalf("get reconciled deployment: %v", err)
656+
}
657+
deployment.Status.ReadyReplicas = 1
658+
deployment.Status.Replicas = 1
659+
if err := k8sClient.Status().Update(ctx, deployment); err != nil {
660+
t.Fatalf("update deployment status: %v", err)
661+
}
662+
663+
if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: cp.Name, Namespace: cp.Namespace}}); err != nil {
664+
t.Fatalf("second reconcile control plane: %v", err)
665+
}
666+
if len(uploader.calls) != 1 {
667+
t.Fatalf("expected initial upload call count 1, got %d", len(uploader.calls))
668+
}
669+
670+
backendHasNoLicenses := false
671+
uploader.hasAnyLicense = &backendHasNoLicenses
672+
if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: cp.Name, Namespace: cp.Namespace}}); err != nil {
673+
t.Fatalf("third reconcile control plane: %v", err)
674+
}
675+
if uploader.hasAnyLicenseCall == 0 {
676+
t.Fatalf("expected reconcile to query existing licenses when hash matches")
677+
}
678+
if len(uploader.calls) != 2 {
679+
t.Fatalf("expected license to be re-uploaded when backend has no licenses, got %d upload calls", len(uploader.calls))
680+
}
681+
}
682+
593683
func TestReconcile_LicenseRotationUploadsNewSecretValue(t *testing.T) {
594684
ctx := context.Background()
595685

0 commit comments

Comments
 (0)