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.

Sign up for GitHub

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

Jump to bottom

refactor: secret generator #1262

Open
wants to merge 2 commits into
base: incubation
Choose a base branch
from
Open

refactor: secret generator #1262

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a87edad
refactor: secret generator
zdtsw Sep 25, 2024
f03b56d
update: apply logger use local variable
zdtsw Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 32 additions & 35 deletions controllers/secretgenerator/secretgenerator_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,28 +57,24 @@ type SecretGeneratorReconciler struct {

// SetupWithManager sets up the controller with the Manager.
func (r *SecretGeneratorReconciler) SetupWithManager(mgr ctrl.Manager) error {
r.Log.Info("Adding controller for Secret Generation.")
log := r.Log
log.Info("Adding controller for Secret Generation.")

// Watch only new secrets with the corresponding annotation
// seems we do get multiple events triggered for the same secret creation and even deletion
predicates := predicate.Funcs{
CreateFunc: func(e event.CreateEvent) bool {
if _, found := e.Object.GetAnnotations()[annotation.SecretNameAnnotation]; found {
return true
}

return false
_, found := e.Object.GetAnnotations()[annotation.SecretNameAnnotation]
return found
},
GenericFunc: func(e event.GenericEvent) bool {
return false
},
// this only watch for secret deletion if has with annotation
// e.g. dashboard-oauth-client but not dashboard-oauth-client-generated
DeleteFunc: func(e event.DeleteEvent) bool {
if _, found := e.Object.GetAnnotations()[annotation.SecretNameAnnotation]; found {
return true
}

return false
_, found := e.Object.GetAnnotations()[annotation.SecretNameAnnotation]
return found
},
UpdateFunc: func(e event.UpdateEvent) bool {
return false
Expand All @@ -105,17 +101,25 @@ func (r *SecretGeneratorReconciler) SetupWithManager(mgr ctrl.Manager) error {
// based on the specified type and complexity. This will avoid possible race
// conditions when a deployment mounts the secret before it is reconciled.
func (r *SecretGeneratorReconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) {
log := r.Log
foundSecret := &corev1.Secret{}
err := r.Client.Get(ctx, request.NamespacedName, foundSecret)

// deletion case
if err != nil {
if k8serr.IsNotFound(err) {
// If Secret is deleted, delete OAuthClient if exists
err = r.deleteOAuthClient(ctx, request.Name)
if k8serr.IsNotFound(err) || foundSecret.GetDeletionTimestamp() != nil {
log.Info("Reconciling Secret on deletion.", "Request.Name", request.Name)
// delete OAuthClient if exists
if err = r.deleteOAuthClient(ctx, request.Name); err != nil {
return ctrl.Result{}, err
}
return ctrl.Result{}, nil
}

return ctrl.Result{}, err
}

log.Info("Reconciling Secret on creation.", "Request.Name", request.Name)
// creation case
owner := []metav1.OwnerReference{
*metav1.NewControllerRef(foundSecret, foundSecret.GroupVersionKind()),
}
Expand All @@ -129,47 +133,40 @@ func (r *SecretGeneratorReconciler) Reconcile(ctx context.Context, request ctrl.
},
}

generatedSecretKey := types.NamespacedName{
Name: generatedSecret.Name, Namespace: generatedSecret.Namespace,
}
err = r.Client.Get(ctx, generatedSecretKey, generatedSecret)
err = r.Client.Get(ctx, client.ObjectKey{Name: generatedSecret.Name, Namespace: generatedSecret.Namespace}, generatedSecret)
Comment on lines -132 to +136
Copy link

Choose a reason for hiding this comment

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

What does the types.NamespacedName change to client.ObjectKey achieve?

Copy link
Member Author

Choose a reason for hiding this comment

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

no real difference.
both generatedSecretKey and client.ObjectKey{} return types.NamespacedName type,
i went for the later one only because of inline style than introducing a one time variable

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, there is also a function helper https://github.com/kubernetes-sigs/controller-runtime/blob/b901db121e1f53c47ec9f9683fad90a546688c3e/pkg/client/interfaces.go#L35

Copy link
Member Author

Choose a reason for hiding this comment

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

nice, then it can be simplified to err = r.Client.Get(ctx, client.ObjectKeyFromObject(generatedSecret), generatedSecret)

if err != nil {
if k8serr.IsNotFound(err) {
// Generate secret random value
r.Log.Info("Generating a random value for a secret in a namespace",
"secret", generatedSecret.Name, "namespace", generatedSecret.Namespace)

// create a secret instance with values
secret, err := NewSecretFrom(foundSecret.GetAnnotations())
if err != nil {
r.Log.Error(err, "error creating secret %s in %s", generatedSecret.Name, generatedSecret.Namespace)
log.Error(err, "error setting secret values for %s "+generatedSecret.Name)
return ctrl.Result{}, err
}

generatedSecret.StringData = map[string]string{
secret.Name: secret.Value,
}

err = r.Client.Create(ctx, generatedSecret)
if err != nil {
log.Error(err, "error generating secret %s in %s", generatedSecret.Name, generatedSecret.Namespace)
return ctrl.Result{}, err
}
r.Log.Info("Done generating secret in namespace",
"secret", generatedSecret.Name, "namespace", generatedSecret.Namespace)
log.Info("Done generating secret", "secret", generatedSecret.Name, "namespace", generatedSecret.Namespace)

// check if annotation oauth-client-route exists
// this is for dashboard-oauth-client secret, not dashboard-oauth-config
if secret.OAuthClientRoute != "" {
// Get OauthClient Route
oauthClientRoute, err := r.getRoute(ctx, secret.OAuthClientRoute, request.Namespace)
if err != nil {
r.Log.Error(err, "Unable to retrieve route from OAuthClient", "route-name", secret.OAuthClientRoute)
log.Error(err, "Unable to retrieve route for OAuthClient", "route-name", secret.OAuthClientRoute)
return ctrl.Result{}, err
}
// Generate OAuthClient for the generated secret
r.Log.Info("Generating an OAuthClient CR for route", "route-name", oauthClientRoute.Name)
log.Info("Generating an OAuthClient CR for route", "route-name", oauthClientRoute.Name)
err = r.createOAuthClient(ctx, foundSecret.Name, secret.Value, oauthClientRoute.Spec.Host)
if err != nil {
r.Log.Error(err, "error creating oauth client resource. Recreate the Secret", "secret-name",
foundSecret.Name)

log.Error(err, "error creating AuthClient CR. Recreate the Secret", "secret-name", foundSecret.Name)
return ctrl.Result{}, err
}
}
Expand All @@ -178,8 +175,7 @@ func (r *SecretGeneratorReconciler) Reconcile(ctx context.Context, request ctrl.
}
}

// Don't requeue if secret is created successfully
return ctrl.Result{}, err
return ctrl.Result{}, nil
}

// getRoute returns an OpenShift route object. It waits until the .spec.host value exists to avoid possible race conditions, fails otherwise.
Expand Down Expand Up @@ -207,6 +203,7 @@ func (r *SecretGeneratorReconciler) getRoute(ctx context.Context, name string, n
}

func (r *SecretGeneratorReconciler) createOAuthClient(ctx context.Context, name string, secretName string, uri string) error {
log := r.Log
// Create OAuthClient resource
oauthClient := &oauthv1.OAuthClient{
TypeMeta: metav1.TypeMeta{
Expand All @@ -224,7 +221,7 @@ func (r *SecretGeneratorReconciler) createOAuthClient(ctx context.Context, name
err := r.Client.Create(ctx, oauthClient)
if err != nil {
if k8serr.IsAlreadyExists(err) {
r.Log.Info("OAuth client resource already exists, patch it", "name", oauthClient.Name)
log.Info("OAuth client resource already exists, patch it", "name", oauthClient.Name)
data, err := json.Marshal(oauthClient)
if err != nil {
return fmt.Errorf("failed to get DataScienceCluster custom resource data: %w", err)
Expand Down