start moving the code towards beta quality by stlaz · Pull Request #173 · kubernetes-sigs/secrets-store-sync-controller

stlaz · 2026-01-20T14:05:17Z

What type of PR is this?

/kind feature
/kind api-change
/kind cleanup

What this PR does / why we need it:
This PR aims to improve the code quality and maintainability of the whole project. It depends on changes from #168.

Outstanding fixes:

no more adding controller-specific empty annotations and labels to the secret, validation is kept
cleaning up function arguments, flattening unnecessarily deep OOP hierarchies
fixing undocumented and questionable behavior of private key fetches
decomposing the Reconcile func of the main controller
complete rewrite of the conditions system

Which issue(s) this PR fixes:

Special notes for your reviewer:

This is to move towards the https://github.com/kubernetes-sigs/secrets-store-sync-controller/milestone/6 milestone, even though code quality improvements are not specifically called out there.

/cc @aramase

k8s-ci-robot · 2026-01-20T14:05:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: stlaz
Once this PR has been reviewed and has the lgtm label, please assign enj for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2026-01-20T14:05:26Z

This issue is currently awaiting triage.

If secrets-store-sync-controller contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

codecov-commenter · 2026-01-20T14:07:47Z

Codecov Report

❌ Patch coverage is 76.40449% with 42 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.85%. Comparing base (9f4597d) to head (3cc6587).
⚠️ Report is 76 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/controller/secretsync_controller.go	76.84%	15 Missing and 7 partials ⚠️
pkg/util/secretutil/secret.go	63.15%	10 Missing and 4 partials ⚠️
pkg/token/token_manager.go	81.81%	2 Missing and 2 partials ⚠️
cmd/main.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #173       +/-   ##
===========================================
- Coverage   56.32%   42.85%   -13.48%     
===========================================
  Files          12       11        -1     
  Lines        1051      742      -309     
===========================================
- Hits          592      318      -274     
+ Misses        415      395       -20     
+ Partials       44       29       -15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

stlaz · 2026-01-27T08:52:29Z

rebased on current main

stlaz · 2026-01-27T12:15:59Z

/hold
there's a good chance the labels/annotations validation might've broken a usecase where we'd sync into an already existing secret

Annotations are currently unused, there's no point having these around at the moment. Keep the validation only, simplify reconcile. Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

Remove the need for error handling by simply concatenating the uid/gen strings. Also remove the "v1" prefix from the final hash. The version was wrong and therefore likely redundant. Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

The TokenClient was not actually client and did not provide any value on its own. This commit takes out the only function of it that had any value and extracts it outside, taking the tokenManager as an argument rather than wrapping it in another object. Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

- pick the first one instead of the last to reduce PEM decoding overhead - don't unnecessarily decode all keys with all three decoding functions - document why we cannot simply use k8s keyutil PEM decoding utils Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

There were several issues with the condition system: 1. disappearing conditions - Some conditions would appear and disappear. 2. nonsensical conditions - "Unknown" does not make sense for a condition type. What would it even mean? - "Create" doesn't really mean anything either, neither does "Update". Compare to the new "SecretCreated" and "SecretUpdated". 3. role confusion - It is unlikely the person viewing the conditions will be able to read the controller's logs, making most of the hardcoded condition messages not actionable, and therefore meaningless. Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

Improves the QoL by actually printing the failing condition, too. Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

stlaz · 2026-01-28T11:50:03Z

/hold cancel
fixed now

config/validatingadmissionpolicies/update_owners_check_old_object.yaml

The policies would race during secret creation, causing different causes for secret creation. This might cause condition hotloops as we're failing to create the secret for different reasons. This also hardcodes the denied secret types to just "kubernetes.io/service-account-token" to protect the users from insecure configurations. Any type that's not allowed is otherwise denied. We may want to think about a backoff mechanism for retries in the future to avoid the controller's sensitivity to these situations. However, validating the same field of the same resource during the same conditions should likely be handled by a single policy, as long as we own the check at least. Signed-off-by: Stanislav Láznička <slznika@microsoft.com> type policies fixup

The version was wrong (v1, expected v1alpha1) and having this value versioned would only bring problems. Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

This was just confusing. Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

k8s-ci-robot requested a review from aramase January 20, 2026 14:05

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 20, 2026

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 20, 2026

enj moved this to Subprojects - Needs Triage in SIG Auth Jan 20, 2026

enj added this to SIG Auth Jan 20, 2026

stlaz force-pushed the refactorings branch 12 times, most recently from 173b477 to e20e4e0 Compare January 27, 2026 08:52

stlaz force-pushed the refactorings branch from e20e4e0 to f511908 Compare January 27, 2026 11:25

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 27, 2026

stlaz added 2 commits January 27, 2026 15:48

only add the controller-specific label with empty value

0c4545e

Annotations are currently unused, there's no point having these around at the moment. Keep the validation only, simplify reconcile. Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

remove redundant and unused arguments to serverSidePatchSecret

51b9174

Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

stlaz added 9 commits January 27, 2026 15:50

handle secret type validation and defaulting in the API

a3f7c5f

decompose the reconcile method by extracting secret fetch logic

64b5ca3

Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

remove unused reconciler in computeSecretDataObjectHash()

b9d7456

Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

remove superfluous 'cat' from README

a3d8b6e

Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

fix the bash e2e test broken by the refactoring

fab4bbf

Improves the QoL by actually printing the failing condition, too. Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

stlaz force-pushed the refactorings branch 3 times, most recently from 104d8e4 to 063243f Compare January 28, 2026 11:39

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2026

stlaz commented Jan 29, 2026

View reviewed changes

config/validatingadmissionpolicies/update_owners_check_old_object.yaml Show resolved Hide resolved

stlaz force-pushed the refactorings branch from 063243f to 0d8e75c Compare January 29, 2026 14:47

stlaz added 6 commits January 30, 2026 09:29

Remove the version from the controller's field manager value

824b611

The version was wrong (v1, expected v1alpha1) and having this value versioned would only bring problems. Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

controller: unexport needlessly exported constants

8e4014a

Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

remove VAPs duplicating the API validation

bde08db

Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

verify-codegen: also check the generated manifest in staging

4303747

Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

fix wrong assumptions about label/annotation sizes

fbaacca

Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

stlaz force-pushed the refactorings branch from 0d8e75c to fbaacca Compare January 30, 2026 08:29

remove unused config/validatingadmissionpolicies directory

3cc6587

This was just confusing. Signed-off-by: Stanislav Láznička <slznika@microsoft.com>

stlaz mentioned this pull request Feb 12, 2026

Replace container-runtime with client-go #186

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

start moving the code towards beta quality#173

start moving the code towards beta quality#173
stlaz wants to merge 18 commits intokubernetes-sigs:mainfrom
stlaz:refactorings

stlaz commented Jan 20, 2026 •

edited

Loading

Uh oh!

k8s-ci-robot commented Jan 20, 2026

Uh oh!

k8s-ci-robot commented Jan 20, 2026

Uh oh!

codecov-commenter commented Jan 20, 2026 •

edited

Loading

Uh oh!

stlaz commented Jan 27, 2026

Uh oh!

stlaz commented Jan 27, 2026

Uh oh!

stlaz commented Jan 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

stlaz commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue(s) this PR fixes:

Uh oh!

k8s-ci-robot commented Jan 20, 2026

Uh oh!

k8s-ci-robot commented Jan 20, 2026

Uh oh!

codecov-commenter commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stlaz commented Jan 27, 2026

Uh oh!

stlaz commented Jan 27, 2026

Uh oh!

stlaz commented Jan 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stlaz commented Jan 20, 2026 •

edited

Loading

codecov-commenter commented Jan 20, 2026 •

edited

Loading