Replace container-runtime with client-go#186
Replace container-runtime with client-go#186stlaz wants to merge 27 commits intokubernetes-sigs:mainfrom
Conversation
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>
Signed-off-by: Stanislav Láznička <slznika@microsoft.com>
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>
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>
Signed-off-by: Stanislav Láznička <slznika@microsoft.com>
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>
Signed-off-by: Stanislav Láznička <slznika@microsoft.com>
Signed-off-by: Stanislav Láznička <slznika@microsoft.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: stlaz The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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 The DetailsInstructions 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 Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #186 +/- ##
===========================================
- Coverage 56.32% 28.29% -28.04%
===========================================
Files 12 24 +12
Lines 1051 1145 +94
===========================================
- Hits 592 324 -268
- Misses 415 787 +372
+ Partials 44 34 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d32b558 to
cadee87
Compare
A controller should be a self-contained unit, immutable from outside. Signed-off-by: Stanislav Láznička <slznika@microsoft.com>
Signed-off-by: Stanislav Láznička <slznika@microsoft.com>
Signed-off-by: Stanislav Láznička <slznika@microsoft.com>
Signed-off-by: Stanislav Láznička <slznika@microsoft.com>
Signed-off-by: Stanislav Láznička <slznika@microsoft.com>
Signed-off-by: Stanislav Láznička <slznika@microsoft.com>
Signed-off-by: Stanislav Láznička <slznika@microsoft.com>
|
@stlaz: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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. I understand the commands that are listed here. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR removes controller-runtime as a dependency for the repository. The controller is rewritten using client-go in a pattern that most kube maintainers are familiar with.
This way we remove a lot of indirection in the controller logic that would make it hard to investigate failures in the future.
Logic primitives common for core kube controllers, like informers and listers, are being used instead of mystical "Getters" that shadow whether a given resource was retrieved from local cache or via a live API request.
Use of typed clients is introduced, and typed clients are generated for the SecretSync resource, making it easier for external use.
Which issue(s) this PR fixes:
Fixes #13, though the main purpose is to simplify future maintenance of the controller
Special notes for your reviewer:
Built on top of the refactorings from #173