Skip to content
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: implement secret adoption #633

Merged
merged 3 commits into from
Jul 16, 2024
Merged

feat: implement secret adoption #633

merged 3 commits into from
Jul 16, 2024

Conversation

sgaist
Copy link
Collaborator

@sgaist sgaist commented Jul 12, 2024

Describe your changes

This PR implements secrets adoption where secrets used for AmaltheaSession will get deleted when the session itself is deleted.

Following discussion, the method selected is through a finalizer.

Issue ticket number and link

Closes #632
Part of SwissDataScienceCenter/renku#3679

@sgaist sgaist requested review from olevski and a team as code owners July 12, 2024 09:17
olevski
olevski previously approved these changes Jul 16, 2024
Copy link
Member

@olevski olevski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

If you want to add the other secrets in this PR I will re-approve. If not we can do it later when we work on those specific parts.

We also need to add the rbac for deleting secrets in the helm chart. We can do that in a followup as well.

Comment on lines +235 to +255
func (cr *AmaltheaSession) AllSecrets() v1.SecretList {
secrets := v1.SecretList{}

if cr.Spec.Ingress != nil && cr.Spec.Ingress.TLSSecretName != nil {
secrets.Items = append(secrets.Items, v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: cr.Namespace,
Name: *cr.Spec.Ingress.TLSSecretName,
},
})
}

return secrets
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could be more.

I looked around and there is:

  • CloningConfigSecretRef
  • ConfigSecretRef
  • For data sources there could be a secret too

But it is all the same to me we can add these in when we add the functionality for cloning code and data sources. Whatever you prefer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also the authentication related secret however I was thinking about adding them while we are implementing the corresponding features so that if for some reason we change the definition we don't have to change the code as well.

sgaist added 3 commits July 16, 2024 14:06
If an AmaltheaSession adopts the secrets used for its functionality,
they will be deleted when the same happens to the CRD.
This allows to keep all the code related the CRD in one place.

Currently only the Ingress is concerned but later on, the Authentication,
Project and Storage objects will also be concerned.
@sgaist
Copy link
Collaborator Author

sgaist commented Jul 16, 2024

@olevski I've added the RBAC directly so we don't have to do it later.

@olevski olevski self-requested a review July 16, 2024 13:56
@olevski olevski merged commit 2e84a16 into main Jul 16, 2024
17 checks passed
@olevski olevski deleted the feature/secrets-adoption branch July 16, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Secrets adoption
2 participants