-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
CephObjectStoreUser Reference Secret #11563
Comments
Hello @jameshearttech, I want to work on this issue. Can you please assign it to me? |
@jameshearttech we do store |
Probably they are asking similar to this #11336 |
Yes I guess so, @jameshearttech can you please check above PR owner about its status? |
Yes, exactly. https://rook-io.slack.com/archives/CK9CF5H2R/p1674075543129569 In my case everything is committed to Git. Storing the accesskey and accesssecret in the CephObjectUserStore resource would potentially leak those sensitive values. Btw, I don't think the docs reflect spec.accessKey and spec.secretKey if those are already implemented. |
is there any new progress? @jameshearttech @thotz @divaspathak @subhamkrai @parth-gr |
I thought this issue was related to #11336 |
Yeah, they are definitely related. If it makes sense, feel free to close this one. EDIT: Oh, wait, I see the other issue was closed. The other issue has a lot of work done on it. In my case I was able to use block instead of bucket. Regardless, I think there should be a solution to this issue. EDIT2: In my last edit I was setting up Loki and I thought that it only worked with object storage, but I found I could use file storage; however, that only works with single binary mode. We are looking to move to simple scaling mode, which requires object storage so this is now a block for me. Likewise, we are looking at implementing Thanos, which also requires object storage. |
Hi, I'd appreciate the approach of referring to existing k8s secrets. This would decouple the direct communication from Ceph to a KMS like Vault. One could implement K8s secrets by themselve and create |
I'm interested in this feature as well. I have rgw clients which are external to k8s. It would be great if I could use something like external-secrets operator to sync rgw user creds. This would be an alternative to using ldap auth. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions. |
This is a feature request we should probably keep open. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions. |
Please do not close feature request. Afaik this has not been implemented. |
A few notes on implementing this feature for someone to pick it up:
Here is an example user secret which is named % kubectl -n rook-ceph get secret rook-ceph-object-user-my-store-my-user -o yaml
apiVersion: v1
kind: Secret
data:
AccessKey: U0lVNVJQTVg0VFIwMTI1QkxGSjc=
SecretKey: aHdnbGdIVmlDcEJqTHN3WUJDakJqWlBRQ2tzeEM3dkRuRDdXMTNySw==
Endpoint: aHR0cDovL3Jvb2stY2VwaC1yZ3ctbXktc3RvcmUucm9vay1jZXBoLnN2Yzo4MA== If this secret is created in advance of the reconcile, then Rook would pick it up. Note that since this issue was originally opened, now the user and secret could be created in any namespace since #12730 was implemented. |
Removing the assigning as I'm focused on some other issue and I don't want to hold this to my name for long |
Also added it to project board 1.13 to get more attention |
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
I've been investigating key rotation for the COSI design updates as well, and after speaking with Josh on a call and collecting more information, I think we might want to revisit the design discussion for this Rook feature as well. It's tiring to get 90% of the way to completion with design and code and then have to rework it, but I think it may be worthwhile in this case. From my discussion with Josh, the biggest gap that I think we have with the implementation proposed is that users don't have a good way to determine when the key rotation has completed. Josh has over a dozen k8s clusters that he manages, and that may grow to 2 dozen in not too long. In large environments like that, being able to know definitively when a key has been rotated is important. Status/monitoring/feedback is also a major tenet of the k8s design philosophy as well, and I missed it when proposing a design for this feature. I'd like to talk about the details more in a Rook huddle next week, but the summary of my proposed new design is to not reuse the current secret for both input and output. I think it's simpler for developers and for users if we use a dedicated input secret, and Rook always uses the existing secret for output. As for monitoring, I propose to achieve that by recording the I'd like to discuss this as a general idea before we start changing the code, so we don't burn ourselves out when reworking this. I think we can reuse a large part of the work we've already done on specifying the secret formatting, so hopefully we can limit that aspect. If that sounds good, we can iron out smaller details regarding COSUser spec/status API fields. |
So for implementation:
@BlaineEXE please review is that what we need to do? |
The key reason to make this change is to make sure we are also planning a way for users to get feedback about the key rotation process. We need to also plan the
Having secretName and secretGeneration are both important. The first allows the user to change the secret used, and ensure Rook applied the correct one. The second allows the user to know when the current secret matches what Rook has applied. From my discussion with Josh, I think it is also still good to plan for a way that users can have multiple active credentials. Josh explained this to be important to him, as well as other admins. It allows a migration window for end users so that key rotation isn't something that causes application downtime. I think we should still plan to include the Also, I have another concern: Because the resource is called CephObjectStoreUser, I think we should refrain from choosing userSecretName. IMO, this naming overlap allows too much room for misunderstanding. I also think we might consider using the a new section and the If we keep it in the top level, |
If we're going to use a different secret for input, that needs to be on the spec, right? Then the status would just have the generation of that secret that has been evaluated, something like this?
And I'm not clear what the proposal is for specifying multiple sets of creds? For simplicity it's nice to have the secret values match the output secret values, but if multiple sets of creds need to be specified, how are additional creds specified? |
@BlaineEXE user secret should not provide |
CephObjectStoreUser should optionally be able to reference a secret where S3 key is defined. This enables us to specify the accesskey and accesssecret rather than those values being randomly generated. Closes: rook#11563 Signed-off-by: parth-gr <partharora1010@gmail.com>
Correct. The input secret should only contain credentials.
The secret name should also be reflected in the status in case the user changes the secret name in the spec. If a user creates a new secret each time, then the generation may always be 1, and there will be ambiguity as to whether the correct credentials are applied.
|
@BlaineEXE some more questions on the status field,
So wanted to understand what should be the status on different events,
|
These are great questions. :) The status should reflect what the current reconcile has completed, unless the status item is specifically intended to reflect something about the ongoing reconcile. Since we aren't telling the user about the reconcile, it means we should update this status only after modifying things related to it. When planning for the behavior of when this particular data gets updated in the status, we need to consider all cases:
Reconciliation routines work best when the logic used in code isn't unique for any of the cases. In this case, there is no need for us to use special logic for any of the cases. But we do need to consider all 3 of the cases I mentioned when deciding how to implement the behavior.
We should not update these status items like this on every reconcile. In the no.3 case where nothing secret-related has changed, changing the status like this will only confuse users. IMO, we can consider changing the status to be this immediately before Rook attempts to update the credentials, for cases no.1 and no.2. Doing so would be an indicator of what Rook is attempting to do (update the creds), and communicates that the underlying user credential status is undefined. However, once we update these statuses to reflect undefined internal state, we can't go back. In my experience, sometimes the RGW will time out or will return an error but will still have made some internal changes. I think it would be good for Rook to indicate undefined state like this, but we should only set it immediately before RGW credentials are going to be modified.
Yes. But let's also be clear that this also assumes the user has specified an input secret. And be sure to remember that this should not be the values of the secret at the time the status is applied. The fields should reflect the secret name and generation that were applied to Ceph, even if they are behind the current secret name/value.
Let's also be clear that this also assumes the user has specified an input secret. Failures can have different sources. If the reconcile fails before Rook attempts to update the credentials, there is no need to change these status fields. If there is an error while applying the user secret data to the user, then we should not update the fields either. This is because we have already (presumably) set the status such that it indicates an undefined result. When RGW says that it failed to do the update, we don't truly know what the internal state is.
Mostly yes, but I would use more straightforward wording: if the reconciliation is completed, and the user hasn't specified
And the user has specified an input secret. This is an interesting case. When thinking about this, I am trying to determine what will be the best for the user experience. Presumably, the user will have an alert that checks whether the status Rook is outputting matches the secret used for input creds. With that case in mind, it probably is good to set these such that they will throw an error Rook can't clear up the issue. However, case no.1 is the complicating one. What happens if the user changes the value from "secret-1" to "secret-2"? From a behavioral standpoint, Rook isn't going to be able to apply creds from a non-existent secret. That means that the underlying user's credentials are known by Rook, and they are known to be the currently-set values. Rook should not update the status in this case. The status should remain at secretName: "secret-1", secretGeneration: <previous-value>
This shouldn't happen, and we don't need to handle this condition. If the secret doesn't exist, that is already a failure. I am trying to focus on and push all of the Rook devs to write less complex and fully-idempotent reconciliation routines and reconciliation logic. With that in mind, I will propose some pseudocode below that I think matches the enumerated cases above. These are good focus points:
Notice that we don't actually have to have conditionals for every one of the cases you have mentioned. The code itself is simpler. But it is still a good exercise to enumerate all of the different input and error conditions beforehand, so we know whether the code we produce is doing the right thing in all cases. It is also good practice to indicate in code comments what Rook is choosing to do (or not to do) based on the enumerated conditions we planned. |
I did some looking locally, and it looks like not all k8s resources utilize the The next best candidate is
That doesn't make any special guarantees that the field won't change when the user hasn't made any updates, but it seems like the persistently stored version shouldn't change randomly, especially of a Secret, which doesn't have a status field. For resources that have a Status, I would bet that a Status change changes the In my test cluster, I haven't seen a secret randomly get a new I think that bodes well for a plan to use |
Is this a bug report or feature request?
What should the feature do:
What is use case behind this feature:
Environment:
The text was updated successfully, but these errors were encountered: