-
Notifications
You must be signed in to change notification settings - Fork 300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: initial implementation of secrets sync controller #1466
feat: initial implementation of secrets sync controller #1466
Conversation
Bumps [k8s.io/klog/v2](https://github.com/kubernetes/klog) from 2.100.1 to 2.120.1. - [Release notes](https://github.com/kubernetes/klog/releases) - [Changelog](https://github.com/kubernetes/klog/blob/main/RELEASE.md) - [Commits](kubernetes/klog@v2.100.1...v2.120.1) --- updated-dependencies: - dependency-name: k8s.io/klog/v2 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
Bumps [actions/dependency-review-action](https://github.com/actions/dependency-review-action) from 3.1.5 to 4.0.0. - [Release notes](https://github.com/actions/dependency-review-action/releases) - [Commits](actions/dependency-review-action@c74b580...4901385) --- updated-dependencies: - dependency-name: actions/dependency-review-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
Hi @mandreap. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
/retitle feat: initial implementation of secrets sync controller |
0c1db60
to
fd62f2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments are based on some initial testing. I haven't done a complete review yet of the changes in this PR.
I've a gist with the steps documented: https://gist.github.com/aramase/46bd3d4270d9c44b59e8c2afe56fbc09
secret-sync-controller/charts/secretsync/templates/create_update_secrets_types.yaml
Outdated
Show resolved
Hide resolved
secret-sync-controller/charts/secretsync/templates/update_owners_check_old_object.yaml
Outdated
Show resolved
Hide resolved
secret-sync-controller/config/validatingadmissionpolicies/create_update_secrets_types.yaml
Outdated
Show resolved
Hide resolved
secret-sync-controller/config/validatingadmissionpolicies/update_owners_check_old_object.yaml
Outdated
Show resolved
Hide resolved
secret-sync-controller/charts/secretsync/templates/auth_proxy_role.yaml
Outdated
Show resolved
Hide resolved
secret-sync-controller/charts/secretsync/templates/auth_proxy_role_binding.yaml
Outdated
Show resolved
Hide resolved
e22f2dc
to
18b99da
Compare
18b99da
to
87b14a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review part 1 on the manifests and charts
Going to review the controller implementation next.
secret-sync-controller/config/rbac/auth_proxy_client_clusterrole.yaml
Outdated
Show resolved
Hide resolved
secret-sync-controller/config/rbac/auth_proxy_role_binding.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous review was only for the controller implementation. Still need to look at all the files that are after controller.
139775a
to
7625c37
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another round of review for controller and token manager implementation.
213c6e0
to
aa8476a
Compare
aa8476a
to
7137c90
Compare
/assign nilekhc |
c4afd9b
to
b4154ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mandreap Only comment to fix the licenses before we can merge.
The previous comments I added have mostly been addressed. I'm ok with merging this PR to the feature branch as-is (after the license is fixed) in an effort to make progress on moving these changes to https://github.com/kubernetes-sigs/secrets-store-sync-controller.
/* | ||
Copyright 2023. | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mandreap This license is still incorrect. Please update it to https://github.com/kubernetes-sigs/secrets-store-csi-driver/pull/1466/files#r1571866927.
b4154ee
to
3f27b06
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
Thank you for the pre-alpha implementation.
The next step for this feature is going to be moving to the new repo and targetting the alpha release so users can try it out!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aramase, mandreap The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
a4ed0fb
into
kubernetes-sigs:feature/secrets-sync-controller
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR proposes an implementation for the Secret Sync Controller. This proposal is a diversion from the current design of the CSI driver. Based on feedback, some of the users want the CSI driver to sync the secret store objects as Kubernetes secrets without the mount instead of the tight coupling between the mount and the sync as it is today.
To support this, we could extract the sync controller from the CSI driver and have it as a standalone deployment. Just syncing as Kubernetes secrets is a cluster-scope operation and doesn’t require the controller or CSI pods to be run on all nodes. The controller would need to watch for Create/Update events for the SS (Secret Sync) and create the Kubernetes secrets by making an RPC call to the provider.
For more information, see the design proposal.