fix(operator): RCON Secret 改走无缓存按名读取,去掉 Secret watch,RBAC 收窄为 secrets get/create,不再缓存全命名空间 Secret
This commit is contained in:
6 files changed
+108
-16
No files matched your search
@@ -118,6 +118,9 @@ func cmdOperator(args []string, _, stderr io.Writer) int {
|
|||||||
// Uncached: the maintenance-lock check lists Jobs only when a server is
|
// Uncached: the maintenance-lock check lists Jobs only when a server is
|
||||||
// about to start, which does not justify a namespace-wide Job informer.
|
// about to start, which does not justify a namespace-wide Job informer.
|
||||||
Jobs: mgr.GetAPIReader(),
|
Jobs: mgr.GetAPIReader(),
|
||||||
|
// Uncached too: RCON Secrets are read by name, so the Role grants
|
||||||
|
// secrets:get without the list/watch an informer would need.
|
||||||
|
Secrets: mgr.GetAPIReader(),
|
||||||
Watch: watch,
|
Watch: watch,
|
||||||
}
|
}
|
||||||
if err := r.SetupWithManager(mgr); err != nil {
|
if err := r.SetupWithManager(mgr); err != nil {
|
||||||
|
|||||||
@@ -10,6 +10,7 @@ import (
|
|||||||
apierrors "k8s.io/apimachinery/pkg/api/errors"
|
apierrors "k8s.io/apimachinery/pkg/api/errors"
|
||||||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
|
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
|
||||||
"k8s.io/apimachinery/pkg/types"
|
"k8s.io/apimachinery/pkg/types"
|
||||||
|
"sigs.k8s.io/controller-runtime/pkg/client"
|
||||||
|
|
||||||
"felis.lolicon.best/internal/apis/felis/v1alpha1"
|
"felis.lolicon.best/internal/apis/felis/v1alpha1"
|
||||||
"felis.lolicon.best/internal/operator"
|
"felis.lolicon.best/internal/operator"
|
||||||
@@ -182,3 +183,64 @@ func TestFailedServerRequeuesWhenTheRetryIsDue(t *testing.T) {
|
|||||||
t.Fatalf("spent requeue = %v, want 5m", res.RequeueAfter)
|
t.Fatalf("spent requeue = %v, want 5m", res.RequeueAfter)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// noCachedSecrets stands in for the manager's cached client, which has no Secret
|
||||||
|
// informer: any Secret read through it fails.
|
||||||
|
type noCachedSecrets struct{ client.Client }
|
||||||
|
|
||||||
|
func (c noCachedSecrets) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
|
||||||
|
if _, ok := obj.(*corev1.Secret); ok {
|
||||||
|
return errors.New("secrets are not cached")
|
||||||
|
}
|
||||||
|
return c.Client.Get(ctx, key, obj, opts...)
|
||||||
|
}
|
||||||
|
|
||||||
|
// RCON Secrets are provisioned and read through Reconciler.Secrets alone, so the
|
||||||
|
// operator runs with secrets:get and no Secret informer.
|
||||||
|
func TestRconSecretsAreReadThroughTheUncachedReader(t *testing.T) {
|
||||||
|
r, c := newReconciler(t, fakeProber{players: operator.PlayerCount{Online: 2, Max: 20}}, runningServer())
|
||||||
|
r.Client = noCachedSecrets{c}
|
||||||
|
r.Secrets = c
|
||||||
|
reconcile(t, r, "survival") // provisions survival-rcon
|
||||||
|
var secret corev1.Secret
|
||||||
|
if err := c.Get(context.Background(), types.NamespacedName{Namespace: "minecraft", Name: "survival-rcon"}, &secret); err != nil {
|
||||||
|
t.Fatalf("rcon secret not provisioned: %v", err)
|
||||||
|
}
|
||||||
|
markPodReady(t, c, "survival")
|
||||||
|
reconcile(t, r, "survival")
|
||||||
|
if s := getServer(t, c, "survival"); s.Status.Phase != v1alpha1.PhaseRunning || !s.Status.Ready {
|
||||||
|
t.Fatalf("phase=%s ready=%v, want Running ready", s.Status.Phase, s.Status.Ready)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// staleOnce answers the first Secret Get NotFound, like a reader that lost the
|
||||||
|
// race to a concurrent create, then reads through.
|
||||||
|
type staleOnce struct {
|
||||||
|
client.Reader
|
||||||
|
missed bool
|
||||||
|
}
|
||||||
|
|
||||||
|
func (s *staleOnce) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
|
||||||
|
if !s.missed {
|
||||||
|
s.missed = true
|
||||||
|
return apierrors.NewNotFound(corev1.Resource("secrets"), key.Name)
|
||||||
|
}
|
||||||
|
return s.Reader.Get(ctx, key, obj, opts...)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Losing the create race re-reads the winner through the same uncached reader.
|
||||||
|
func TestRconSecretCreateRaceRereadsUncached(t *testing.T) {
|
||||||
|
r, c := newReconciler(t, fakeProber{}, runningServer(), rconSecret())
|
||||||
|
r.Client = noCachedSecrets{c}
|
||||||
|
r.Secrets = &staleOnce{Reader: c}
|
||||||
|
reconcile(t, r, "survival")
|
||||||
|
var secret corev1.Secret
|
||||||
|
if err := c.Get(context.Background(), types.NamespacedName{Namespace: "minecraft", Name: "survival-rcon"}, &secret); err != nil {
|
||||||
|
t.Fatal(err)
|
||||||
|
}
|
||||||
|
if got := string(secret.Data["password"]); got != "hunter2" {
|
||||||
|
t.Fatalf("password = %q, want the winner's hunter2", got)
|
||||||
|
}
|
||||||
|
// The pass went on to build the workload instead of parking on RconSecretUnavailable.
|
||||||
|
getSTS(t, c, "survival")
|
||||||
|
}
|
||||||
@@ -122,6 +122,12 @@ type Reconciler struct {
|
|||||||
// (internal/maintenance). It is the manager's uncached API reader, so the
|
// (internal/maintenance). It is the manager's uncached API reader, so the
|
||||||
// operator needs jobs:list and no Job informer. Nil skips the check.
|
// operator needs jobs:list and no Job informer. Nil skips the check.
|
||||||
Jobs client.Reader
|
Jobs client.Reader
|
||||||
|
// Secrets reads the RCON password Secrets, each by name. It is the manager's
|
||||||
|
// uncached API reader, so the operator holds secrets:get and no list or watch:
|
||||||
|
// a Secret informer would cache every Secret in the namespace (the felis-config
|
||||||
|
// mirror with the database URL among them) and grow with them. Nil falls back
|
||||||
|
// to the embedded client.
|
||||||
|
Secrets client.Reader
|
||||||
// Watch records the passes in flight for the liveness probe. Nil skips it.
|
// Watch records the passes in flight for the liveness probe. Nil skips it.
|
||||||
Watch *ReconcileWatch
|
Watch *ReconcileWatch
|
||||||
|
|
||||||
@@ -146,10 +152,9 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
|
|||||||
For(&v1alpha1.MinecraftServer{}).
|
For(&v1alpha1.MinecraftServer{}).
|
||||||
Owns(&appsv1.StatefulSet{}).
|
Owns(&appsv1.StatefulSet{}).
|
||||||
Owns(&corev1.Service{}).
|
Owns(&corev1.Service{}).
|
||||||
// Owns the Secrets too: the per-server RCON password is managed here,
|
// No Secret watch: a Running server is re-reconciled every
|
||||||
// and a watch is what lets a deleted Secret be noticed at all (a quiet
|
// requeueRunningProbe anyway, which recreates a deleted RCON Secret, and
|
||||||
// Running server otherwise produces no events).
|
// a stopped one gets it back on its next start.
|
||||||
Owns(&corev1.Secret{}).
|
|
||||||
WithOptions(controller.Options{MaxConcurrentReconciles: maxConcurrentReconciles}).
|
WithOptions(controller.Options{MaxConcurrentReconciles: maxConcurrentReconciles}).
|
||||||
Complete(r)
|
Complete(r)
|
||||||
}
|
}
|
||||||
@@ -522,7 +527,7 @@ func (r *Reconciler) ensureRconSecret(ctx context.Context, server *v1alpha1.Mine
|
|||||||
return "", fmt.Errorf("rcon.secretRef.name and .key are required when rcon is enabled")
|
return "", fmt.Errorf("rcon.secretRef.name and .key are required when rcon is enabled")
|
||||||
}
|
}
|
||||||
var existing corev1.Secret
|
var existing corev1.Secret
|
||||||
err := r.Get(ctx, types.NamespacedName{Namespace: server.Namespace, Name: ref.Name}, &existing)
|
err := r.secretReader().Get(ctx, types.NamespacedName{Namespace: server.Namespace, Name: ref.Name}, &existing)
|
||||||
if err == nil {
|
if err == nil {
|
||||||
if b, ok := existing.Data[ref.Key]; ok {
|
if b, ok := existing.Data[ref.Key]; ok {
|
||||||
return rconStamp(b), nil
|
return rconStamp(b), nil
|
||||||
@@ -561,7 +566,7 @@ func (r *Reconciler) ensureRconSecret(ctx context.Context, server *v1alpha1.Mine
|
|||||||
// value that would flip on the next reconcile and roll the pod twice.
|
// value that would flip on the next reconcile and roll the pod twice.
|
||||||
if apierrors.IsAlreadyExists(err) {
|
if apierrors.IsAlreadyExists(err) {
|
||||||
var winner corev1.Secret
|
var winner corev1.Secret
|
||||||
if gerr := r.Get(ctx, types.NamespacedName{Namespace: server.Namespace, Name: ref.Name}, &winner); gerr != nil {
|
if gerr := r.secretReader().Get(ctx, types.NamespacedName{Namespace: server.Namespace, Name: ref.Name}, &winner); gerr != nil {
|
||||||
return "", gerr
|
return "", gerr
|
||||||
}
|
}
|
||||||
return rconStamp(winner.Data[ref.Key]), nil
|
return rconStamp(winner.Data[ref.Key]), nil
|
||||||
@@ -583,13 +588,20 @@ func randomRconPassword() (string, error) {
|
|||||||
return hex.EncodeToString(b[:]), nil
|
return hex.EncodeToString(b[:]), nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func (r *Reconciler) secretReader() client.Reader {
|
||||||
|
if r.Secrets != nil {
|
||||||
|
return r.Secrets
|
||||||
|
}
|
||||||
|
return r.Client
|
||||||
|
}
|
||||||
|
|
||||||
func (r *Reconciler) rconPassword(ctx context.Context, server *v1alpha1.MinecraftServer) (string, error) {
|
func (r *Reconciler) rconPassword(ctx context.Context, server *v1alpha1.MinecraftServer) (string, error) {
|
||||||
ref := server.Spec.Rcon.SecretRef
|
ref := server.Spec.Rcon.SecretRef
|
||||||
if ref.Name == "" || ref.Key == "" {
|
if ref.Name == "" || ref.Key == "" {
|
||||||
return "", fmt.Errorf("rcon.secretRef.name and .key are required when rcon is enabled")
|
return "", fmt.Errorf("rcon.secretRef.name and .key are required when rcon is enabled")
|
||||||
}
|
}
|
||||||
var secret corev1.Secret
|
var secret corev1.Secret
|
||||||
if err := r.Get(ctx, types.NamespacedName{Namespace: server.Namespace, Name: ref.Name}, &secret); err != nil {
|
if err := r.secretReader().Get(ctx, types.NamespacedName{Namespace: server.Namespace, Name: ref.Name}, &secret); err != nil {
|
||||||
return "", err
|
return "", err
|
||||||
}
|
}
|
||||||
b, ok := secret.Data[ref.Key]
|
b, ok := secret.Data[ref.Key]
|
||||||
|
|||||||
@@ -150,7 +150,7 @@ func APIBuildRole(p Params) *rbacv1.Role {
|
|||||||
// status (Status().Update — `update` only) and patches spec.desiredState to
|
// status (Status().Update — `update` only) and patches spec.desiredState to
|
||||||
// Stopped for idle auto-stop (spec §8 — the one spec field it may write, using
|
// Stopped for idle auto-stop (spec §8 — the one spec field it may write, using
|
||||||
// the same merge patch as the reaper's Stop: without the grant the auto-stop
|
// the same merge patch as the reaper's Stop: without the grant the auto-stop
|
||||||
// call fails closed with a 403), and reads RCON Secrets. Jobs are list-only,
|
// call fails closed with a 403), and reads RCON Secrets by name, uncached. Jobs are list-only,
|
||||||
// through the manager's uncached API reader: before scaling a server up from zero
|
// through the manager's uncached API reader: before scaling a server up from zero
|
||||||
// the operator checks that no restore/backup/file-write Job holds its world
|
// the operator checks that no restore/backup/file-write Job holds its world
|
||||||
// (internal/maintenance). Pods are delete-only: a start that timed out is retried
|
// (internal/maintenance). Pods are delete-only: a start that timed out is retried
|
||||||
@@ -164,13 +164,13 @@ func OperatorRole(p Params) *rbacv1.Role {
|
|||||||
rule([]string{groupFelis}, []string{"minecraftservers/status"}, []string{"update"}),
|
rule([]string{groupFelis}, []string{"minecraftservers/status"}, []string{"update"}),
|
||||||
rule([]string{groupApps}, []string{"statefulsets"}, []string{"get", "list", "watch", "create", "update"}),
|
rule([]string{groupApps}, []string{"statefulsets"}, []string{"get", "list", "watch", "create", "update"}),
|
||||||
rule([]string{groupCore}, []string{"services"}, []string{"get", "list", "watch", "create", "update"}),
|
rule([]string{groupCore}, []string{"services"}, []string{"get", "list", "watch", "create", "update"}),
|
||||||
// create is here for the per-server RCON password Secret the operator
|
// get by name, through the uncached API reader (Reconciler.Secrets), of the
|
||||||
// provisions on first reconcile (internal/operator.ensureRconSecret). It is a
|
// RCON password Secret a server's spec names; create for the one the operator
|
||||||
// smaller grant than it looks: this identity already holds get/list/watch on
|
// provisions on first reconcile (internal/operator.ensureRconSecret). No
|
||||||
// every Secret in this namespace, so being able to add one grants no read it
|
// list/watch, so there is no Secret informer and nothing enumerates the
|
||||||
// did not already have. No update/delete — the password is written once and
|
// namespace's Secrets. No update/delete — the password is written once and
|
||||||
// removed by garbage collection through its controller reference.
|
// removed by garbage collection through its controller reference.
|
||||||
rule([]string{groupCore}, []string{"secrets"}, []string{"get", "list", "watch", "create"}),
|
rule([]string{groupCore}, []string{"secrets"}, []string{"get", "create"}),
|
||||||
// list only: an uncached List (no informer, so no watch) of the world-volume
|
// list only: an uncached List (no informer, so no watch) of the world-volume
|
||||||
// maintenance Jobs; the operator never creates or deletes a Job.
|
// maintenance Jobs; the operator never creates or deletes a Job.
|
||||||
rule([]string{groupBatch}, []string{"jobs"}, []string{"list"}),
|
rule([]string{groupBatch}, []string{"jobs"}, []string{"list"}),
|
||||||
|
|||||||
@@ -194,6 +194,18 @@ func TestOperatorRole_ScopeExact(t *testing.T) {
|
|||||||
t.Errorf("operator must NOT touch core/%s", res)
|
t.Errorf("operator must NOT touch core/%s", res)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
// RCON Secrets are read by name through the uncached reader and created once;
|
||||||
|
// no list/watch, so no informer mirrors the namespace's Secrets into it.
|
||||||
|
for _, v := range []string{"get", "create"} {
|
||||||
|
if !hasRule(op, groupCore, "secrets", v) {
|
||||||
|
t.Errorf("operator must have secrets:%s", v)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
for _, v := range []string{"list", "watch", "update", "patch", "delete", "deletecollection", "*"} {
|
||||||
|
if hasRule(op, groupCore, "secrets", v) {
|
||||||
|
t.Errorf("operator secrets rule must be get+create only, found %s", v)
|
||||||
|
}
|
||||||
|
}
|
||||||
// Pods are delete-only: the bounded retry of a timed-out start.
|
// Pods are delete-only: the bounded retry of a timed-out start.
|
||||||
if !hasRule(op, groupCore, "pods", "delete") {
|
if !hasRule(op, groupCore, "pods", "delete") {
|
||||||
t.Error("operator must have pods:delete (auto-restart of a timed-out start)")
|
t.Error("operator must have pods:delete (auto-restart of a timed-out start)")
|
||||||
|
|||||||
@@ -540,8 +540,11 @@ func operatorMetricsService(p Params) *corev1.Service {
|
|||||||
// the felis-operator SA and carries controlPlanePodLabels(operator), the second
|
// the felis-operator SA and carries controlPlanePodLabels(operator), the second
|
||||||
// pod the allow-rcon peer admits (the readiness prober dials RCON). It takes NO
|
// pod the allow-rcon peer admits (the readiness prober dials RCON). It takes NO
|
||||||
// config Secret: the operator reads everything from flags + the in-cluster API,
|
// config Secret: the operator reads everything from flags + the in-cluster API,
|
||||||
// so it never holds the database URL — a deliberately smaller attack surface than
|
// so it never loads the database URL — a deliberately smaller attack surface than
|
||||||
// the api. It watches the minecraft namespace (--namespace) while running in the
|
// the api. Its secrets:get in the minecraft namespace is by name and uncached
|
||||||
|
// (no list, no informer), yet namespaced RBAC cannot exclude a name, so a
|
||||||
|
// compromised operator could still fetch the felis-config mirror the Jobs and
|
||||||
|
// the reaper mount there. It watches the minecraft namespace (--namespace) while running in the
|
||||||
// control namespace, exactly the split cmd/felis/operator.go documents.
|
// control namespace, exactly the split cmd/felis/operator.go documents.
|
||||||
func OperatorDeployment(p Params) *appsv1.Deployment {
|
func OperatorDeployment(p Params) *appsv1.Deployment {
|
||||||
p = p.withDefaults()
|
p = p.withDefaults()
|
||||||
|
|||||||
Reference in new issue
Block a user