Skip to content

Commit 3c9cd6c

Browse files
myasnikovdaniilmyasnikovdaniil
committed
fix(api): fail a pool create when the platform values cannot be read
loadWorkerPoolDefaults treated every error reading the cozystack-values Secret as a platform that sets nothing, so a failed read stored the pool without the defaults and told nobody. The image source and the installer are hashed into names the MachineDeployment points at, so putting the defaults in later re-images the pool. Skip quietly only when the Secret does not exist. Any other read error returns ServiceUnavailable, which a caller can repeat once the apiserver answers, and a Secret whose content cannot be parsed returns an internal error; both stop the create before anything is stored. Also document that a pool written straight as a HelmRelease, as the ComputePlane module and the node group adoption migration do, never passes through the API and is not filled in. Assisted-by: LLM Signed-off-by: Myasnikov Daniil <[email protected]>
1 parent 5fc2a81 commit 3c9cd6c

4 files changed

Lines changed: 109 additions & 22 deletions

File tree

‎packages/core/platform/values.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -774,6 +774,11 @@ backupStrategyController: {}
774774
# exists is not touched and does not roll, and moving one onto these defaults is
775775
# done by hand (see the header of the kubernetes-worker-image values). Nothing is
776776
# filled in for a key left unset here.
777+
#
778+
# A pool that is written straight as a HelmRelease never passes through
779+
# cozystack-api and is not filled in. The ComputePlane module does that, and so did
780+
# the migration that adopted the node groups of the kubernetes app. Such a pool still
781+
# takes kubernetesRegistries when it renders.
777782
kubernetesWorkerImage: {}
778783
# Pod scheduling configuration
779784
scheduling:

‎pkg/registry/apps/application/rest.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,9 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
220220
// Before the null normalization, which would otherwise warn that the chart
221221
// default applies for a null this fills with the platform's, and before
222222
// validation, so that the chain below judges the spec that gets stored.
223-
r.applyCreateDefaults(ctx, app)
223+
if err := r.applyCreateDefaults(ctx, app); err != nil {
224+
return nil, err
225+
}
224226

225227
// Omission permits a chart default; explicit null suppresses it in Helm.
226228
// Normalize only repairable fields before admission and persistence.

‎pkg/registry/apps/application/rest_create_defaults.go‎

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,11 @@ import (
2020
"bytes"
2121
"context"
2222
"encoding/json"
23+
"fmt"
2324

2425
corev1 "k8s.io/api/core/v1"
2526
apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
27+
apierrors "k8s.io/apimachinery/pkg/api/errors"
2628
"k8s.io/klog/v2"
2729
"sigs.k8s.io/controller-runtime/pkg/client"
2830
"sigs.k8s.io/yaml"
@@ -64,14 +66,20 @@ type platformWorkerValues struct {
6466
// into the spec it is an ordinary field and nothing existing changes.
6567
//
6668
// A field the spec already carries, empty or not, is left alone, which is how a pool
67-
// opts out. Failing to read the platform values never fails the create.
68-
func (r *REST) applyCreateDefaults(ctx context.Context, app *appsv1alpha1.Application) {
69+
// opts out. A platform that has no values Secret has set nothing and the create goes
70+
// on. Any other failure to read it fails the create: the pool would be stored without
71+
// the defaults, with no sign to the client, and since they are hashed into names the
72+
// MachineDeployment points at, putting them in later re-images it.
73+
func (r *REST) applyCreateDefaults(ctx context.Context, app *appsv1alpha1.Application) error {
6974
if r.kindName != kubernetesNodesKind {
70-
return
75+
return nil
76+
}
77+
defaults, ok, err := r.loadWorkerPoolDefaults(ctx)
78+
if err != nil {
79+
return err
7180
}
72-
defaults, ok := r.loadWorkerPoolDefaults(ctx)
7381
if !ok {
74-
return
82+
return nil
7583
}
7684

7785
spec := map[string]any{}
@@ -80,38 +88,42 @@ func (r *REST) applyCreateDefaults(ctx context.Context, app *appsv1alpha1.Applic
8088
dec.UseNumber()
8189
if err := dec.Decode(&spec); err != nil {
8290
klog.Warningf("Not applying worker pool defaults to %s/%s: cannot read its spec: %v", app.Namespace, app.Name, err)
83-
return
91+
return nil
8492
}
8593
if spec == nil {
8694
spec = map[string]any{}
8795
}
8896
}
8997

9098
if !defaults.fill(spec) {
91-
return
99+
return nil
92100
}
93101
raw, err := json.Marshal(spec)
94102
if err != nil {
95103
klog.Warningf("Not applying worker pool defaults to %s/%s: cannot encode its spec: %v", app.Namespace, app.Name, err)
96-
return
104+
return nil
97105
}
98106
app.Spec = &apiextv1.JSON{Raw: raw}
107+
return nil
99108
}
100109

101-
func (r *REST) loadWorkerPoolDefaults(ctx context.Context) (workerPoolDefaults, bool) {
110+
func (r *REST) loadWorkerPoolDefaults(ctx context.Context) (workerPoolDefaults, bool, error) {
102111
secret := &corev1.Secret{}
103112
key := client.ObjectKey{Namespace: platformValuesNamespace, Name: platformValuesSecret}
104113
if err := r.c.Get(ctx, key, secret); err != nil {
105-
klog.Warningf("Not applying worker pool defaults: cannot read Secret %s: %v", key, err)
106-
return workerPoolDefaults{}, false
114+
if apierrors.IsNotFound(err) {
115+
return workerPoolDefaults{}, false, nil
116+
}
117+
klog.Errorf("Cannot read the platform worker pool defaults from Secret %s: %v", key, err)
118+
return workerPoolDefaults{}, false, apierrors.NewServiceUnavailable(fmt.Sprintf("cannot read the platform worker pool defaults from Secret %s, try again: %v", key, err))
107119
}
108120
var values platformWorkerValues
109121
if err := yaml.Unmarshal(secret.Data[platformValuesKey], &values); err != nil {
110-
klog.Warningf("Not applying worker pool defaults: Secret %s does not hold readable %s: %v", key, platformValuesKey, err)
111-
return workerPoolDefaults{}, false
122+
klog.Errorf("Secret %s does not hold readable %s: %v", key, platformValuesKey, err)
123+
return workerPoolDefaults{}, false, apierrors.NewInternalError(fmt.Errorf("secret %s does not hold readable %s, so the platform worker pool defaults cannot be applied: %w", key, platformValuesKey, err))
112124
}
113125
d := values.Cluster.WorkerImage
114-
return d, d.Catalog == "true" || d.FactoryURL != "" || d.InstallerRepository != ""
126+
return d, d.Catalog == "true" || d.FactoryURL != "" || d.InstallerRepository != "", nil
115127
}
116128

117129
// fill sets every default the spec does not already carry and reports whether it

‎pkg/registry/apps/application/rest_create_defaults_test.go‎

Lines changed: 75 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,19 +19,22 @@ package application
1919
import (
2020
"context"
2121
"encoding/json"
22+
"errors"
2223
"strings"
2324
"testing"
2425

2526
helmv2 "github.com/fluxcd/helm-controller/api/v2"
2627
corev1 "k8s.io/api/core/v1"
2728
apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
29+
apierrors "k8s.io/apimachinery/pkg/api/errors"
2830
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2931
"k8s.io/apimachinery/pkg/runtime"
3032
"k8s.io/apimachinery/pkg/runtime/schema"
3133
"k8s.io/apiserver/pkg/endpoints/request"
3234
"k8s.io/apiserver/pkg/warning"
3335
"sigs.k8s.io/controller-runtime/pkg/client"
3436
"sigs.k8s.io/controller-runtime/pkg/client/fake"
37+
"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
3538

3639
appsv1alpha1 "github.com/cozystack/cozystack/pkg/apis/apps/v1alpha1"
3740
"github.com/cozystack/cozystack/pkg/config"
@@ -120,6 +123,19 @@ func newCreateDefaultsREST(t *testing.T, kind string, objs ...client.Object) *RE
120123
}
121124
}
122125

126+
// failSecretReads makes every Get of a Secret return err and leaves the rest of the
127+
// REST's client as it was.
128+
func failSecretReads(r *REST, err error) {
129+
r.c = interceptor.NewClient(r.c.(client.WithWatch), interceptor.Funcs{
130+
Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
131+
if _, ok := obj.(*corev1.Secret); ok {
132+
return err
133+
}
134+
return c.Get(ctx, key, obj, opts...)
135+
},
136+
})
137+
}
138+
123139
func newPoolApp(spec string) *appsv1alpha1.Application {
124140
app := &appsv1alpha1.Application{
125141
TypeMeta: metav1.TypeMeta{APIVersion: "apps.cozystack.io/v1alpha1", Kind: kubernetesNodesKind},
@@ -142,8 +158,12 @@ func TestApplyCreateDefaults(t *testing.T) {
142158
kind string
143159
secret *corev1.Secret
144160
spec string
161+
// getErr, when set, is what reading the platform values Secret returns.
162+
getErr error
145163
// want is the spec after the call; "" means the spec must still be nil.
146164
want string
165+
// wantErr is a part of the error the call must return; "" means no error.
166+
wantErr string
147167
}{
148168
{
149169
name: "fills every default into an empty spec",
@@ -242,10 +262,25 @@ func TestApplyCreateDefaults(t *testing.T) {
242262
name: "leaves the spec alone when the platform values Secret is missing",
243263
},
244264
{
245-
name: "leaves the spec alone when the platform values are unreadable",
246-
secret: platformValuesFixture("_cluster: [not, a, map"),
247-
spec: `{"diskSize":"30Gi"}`,
248-
want: `{"diskSize":"30Gi"}`,
265+
name: "fails when the platform values are unreadable",
266+
secret: platformValuesFixture("_cluster: [not, a, map"),
267+
spec: `{"diskSize":"30Gi"}`,
268+
want: `{"diskSize":"30Gi"}`,
269+
wantErr: "does not hold readable values.yaml",
270+
},
271+
{
272+
name: "fails when the platform values Secret cannot be read",
273+
secret: platformValuesFixture(workerDefaultsAll),
274+
spec: `{"diskSize":"30Gi"}`,
275+
getErr: apierrors.NewServiceUnavailable("apiserver is restarting"),
276+
want: `{"diskSize":"30Gi"}`,
277+
wantErr: "cannot read the platform worker pool defaults",
278+
},
279+
{
280+
name: "fails when the platform values Secret is forbidden",
281+
secret: platformValuesFixture(workerDefaultsAll),
282+
getErr: apierrors.NewForbidden(schema.GroupResource{Resource: "secrets"}, platformValuesSecret, errors.New("no access")),
283+
wantErr: "cannot read the platform worker pool defaults",
249284
},
250285
{
251286
name: "leaves a talos that is not an object to validation",
@@ -272,10 +307,20 @@ func TestApplyCreateDefaults(t *testing.T) {
272307
objs = append(objs, tc.secret)
273308
}
274309
r := newCreateDefaultsREST(t, kind, objs...)
310+
if tc.getErr != nil {
311+
failSecretReads(r, tc.getErr)
312+
}
275313
app := newPoolApp(tc.spec)
276314

277-
r.applyCreateDefaults(context.Background(), app)
315+
err := r.applyCreateDefaults(context.Background(), app)
278316

317+
if tc.wantErr != "" {
318+
if err == nil || !strings.Contains(err.Error(), tc.wantErr) {
319+
t.Fatalf("error = %v, want one containing %q", err, tc.wantErr)
320+
}
321+
} else if err != nil {
322+
t.Fatalf("applyCreateDefaults: %v", err)
323+
}
279324
if tc.want == "" {
280325
if app.Spec != nil {
281326
t.Fatalf("spec = %s, want it untouched (nil)", app.Spec.Raw)
@@ -296,7 +341,9 @@ func TestApplyCreateDefaults_KeepsNumbersAsWritten(t *testing.T) {
296341
r := newCreateDefaultsREST(t, kubernetesNodesKind, platformValuesFixture(workerDefaultsAll))
297342
app := newPoolApp(`{"maxReplicas":12345678901234567890,"minReplicas":1e400}`)
298343

299-
r.applyCreateDefaults(context.Background(), app)
344+
if err := r.applyCreateDefaults(context.Background(), app); err != nil {
345+
t.Fatalf("applyCreateDefaults: %v", err)
346+
}
300347

301348
for _, want := range []string{`"maxReplicas":12345678901234567890`, `"minReplicas":1e400`, `"osImage":{"builtin":{}}`} {
302349
if !strings.Contains(string(app.Spec.Raw), want) {
@@ -419,7 +466,7 @@ func TestUpdate_OfAnExistingObjectDoesNotApplyDefaults(t *testing.T) {
419466
assertJSONEqual(t, `{"cluster":"myk8s","maxReplicas":5}`, []byte(storedValues(t, r, "myk8s-md0")))
420467
}
421468

422-
// A failed read of the platform values must not fail the create.
469+
// A platform with no values Secret has set nothing, and the create goes on.
423470
func TestCreate_SucceedsWithoutPlatformValues(t *testing.T) {
424471
r := newCreateDefaultsREST(t, kubernetesNodesKind)
425472
ctx := request.WithNamespace(context.Background(), "tenant-foo")
@@ -430,3 +477,24 @@ func TestCreate_SucceedsWithoutPlatformValues(t *testing.T) {
430477

431478
assertJSONEqual(t, `{"cluster":"myk8s"}`, []byte(storedValues(t, r, "myk8s-md0")))
432479
}
480+
481+
// A Secret that cannot be read is not the same as a platform that set nothing: storing
482+
// the pool without the defaults would pick its image source for good, so the create is
483+
// refused with a 503, which a caller can repeat once the apiserver answers, and
484+
// nothing is stored.
485+
func TestCreate_FailsWhenThePlatformValuesCannotBeRead(t *testing.T) {
486+
r := newCreateDefaultsREST(t, kubernetesNodesKind, platformValuesFixture(workerDefaultsAll))
487+
failSecretReads(r, errors.New("etcdserver: request timed out"))
488+
ctx := request.WithNamespace(context.Background(), "tenant-foo")
489+
490+
_, err := r.Create(ctx, newPoolApp(`{"cluster":"myk8s"}`), nil, &metav1.CreateOptions{})
491+
if !apierrors.IsServiceUnavailable(err) {
492+
t.Fatalf("Create error = %v, want a ServiceUnavailable", err)
493+
}
494+
495+
hr := &helmv2.HelmRelease{}
496+
getErr := r.c.Get(context.Background(), client.ObjectKey{Namespace: "tenant-foo", Name: r.releaseConfig.Prefix + "myk8s-md0"}, hr)
497+
if !apierrors.IsNotFound(getErr) {
498+
t.Fatalf("HelmRelease lookup = %v, want NotFound: the refused create must store nothing", getErr)
499+
}
500+
}

0 commit comments

Comments
 (0)