Skip to content

Commit

Permalink
fix: refactor the secretg generator controlle4
Browse files Browse the repository at this point in the history
The commit is meant to:
- make the code easier to understand, reducing complexity caused by
  nested if/else and error conditions (align happy path to the left)
- remove shadowed error vars to avoid reporting misleading errors
- add some basic unit test for the reconcile loop
  • Loading branch information
lburgazzoli committed Oct 22, 2024
1 parent b91bd29 commit 0cb1dba
Show file tree
Hide file tree
Showing 2 changed files with 216 additions and 52 deletions.
115 changes: 63 additions & 52 deletions controllers/secretgenerator/secretgenerator_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ func (r *SecretGeneratorReconciler) SetupWithManager(ctx context.Context, mgr ct
// 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 := logf.FromContext(ctx).WithName("SecretGenerator")
foundSecret := &corev1.Secret{}
err := r.Client.Get(ctx, request.NamespacedName, foundSecret)
if err != nil {
Expand All @@ -116,70 +115,82 @@ func (r *SecretGeneratorReconciler) Reconcile(ctx context.Context, request ctrl.
return ctrl.Result{}, err
}

owner := []metav1.OwnerReference{
*metav1.NewControllerRef(foundSecret, foundSecret.GroupVersionKind()),
}
// Generate the secret if it does not previously exist
generatedSecret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: foundSecret.Name + "-generated",
Namespace: foundSecret.Namespace,
Labels: foundSecret.Labels,
OwnerReferences: owner,
Name: foundSecret.Name + "-generated",
Namespace: foundSecret.Namespace,
},
}

generatedSecretKey := types.NamespacedName{
Name: generatedSecret.Name, Namespace: generatedSecret.Namespace,
err = r.Client.Get(ctx, client.ObjectKeyFromObject(generatedSecret), generatedSecret)
if err == nil || !k8serr.IsNotFound(err) {
return ctrl.Result{}, err
}
err = r.Client.Get(ctx, generatedSecretKey, generatedSecret)

err = r.generateSecret(ctx, foundSecret, generatedSecret)
if err != nil {
if k8serr.IsNotFound(err) {
// Generate secret random value
log.Info("Generating a random value for a secret in a namespace",
"secret", generatedSecret.Name, "namespace", generatedSecret.Namespace)
return ctrl.Result{}, err
}

Check warning on line 134 in controllers/secretgenerator/secretgenerator_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/secretgenerator/secretgenerator_controller.go#L133-L134

Added lines #L133 - L134 were not covered by tests

secret, err := NewSecretFrom(foundSecret.GetAnnotations())
if err != nil {
log.Error(err, "error creating secret %s in %s", generatedSecret.Name, generatedSecret.Namespace)
return ctrl.Result{}, err
}
// Don't requeue if secret is created successfully
return ctrl.Result{}, nil
}

generatedSecret.StringData = map[string]string{
secret.Name: secret.Value,
}
func (r *SecretGeneratorReconciler) generateSecret(ctx context.Context, foundSecret *corev1.Secret, generatedSecret *corev1.Secret) error {
log := logf.FromContext(ctx).WithName("SecretGenerator")

err = r.Client.Create(ctx, generatedSecret)
if err != nil {
return ctrl.Result{}, err
}
log.Info("Done generating secret in namespace",
"secret", generatedSecret.Name, "namespace", generatedSecret.Namespace)
// check if annotation oauth-client-route exists
if secret.OAuthClientRoute != "" {
// Get OauthClient Route
oauthClientRoute, err := r.getRoute(ctx, secret.OAuthClientRoute, request.Namespace)
if err != nil {
log.Error(err, "Unable to retrieve route from OAuthClient", "route-name", secret.OAuthClientRoute)
return ctrl.Result{}, err
}
// Generate OAuthClient for the generated secret
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 {
log.Error(err, "error creating oauth client resource. Recreate the Secret", "secret-name",
foundSecret.Name)

return ctrl.Result{}, err
}
}
} else {
return ctrl.Result{}, err
}
// Generate secret random value
log.Info("Generating a random value for a secret in a namespace",
"secret", generatedSecret.Name, "namespace", generatedSecret.Namespace)

generatedSecret.Labels = foundSecret.Labels

generatedSecret.OwnerReferences = []metav1.OwnerReference{
*metav1.NewControllerRef(foundSecret, foundSecret.GroupVersionKind()),
}

// Don't requeue if secret is created successfully
return ctrl.Result{}, err
secret, err := NewSecretFrom(foundSecret.GetAnnotations())
if err != nil {
log.Error(err, "error creating secret %s in %s", generatedSecret.Name, generatedSecret.Namespace)
return err
}

Check warning on line 157 in controllers/secretgenerator/secretgenerator_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/secretgenerator/secretgenerator_controller.go#L155-L157

Added lines #L155 - L157 were not covered by tests

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

err = r.Client.Create(ctx, generatedSecret)
if err != nil {
return err
}

Check warning on line 166 in controllers/secretgenerator/secretgenerator_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/secretgenerator/secretgenerator_controller.go#L165-L166

Added lines #L165 - L166 were not covered by tests

log.Info("Done generating secret in namespace",
"secret", generatedSecret.Name, "namespace", generatedSecret.Namespace)

// check if annotation oauth-client-route exists
if secret.OAuthClientRoute == "" {
return nil
}

// Get OauthClient Route
oauthClientRoute, err := r.getRoute(ctx, secret.OAuthClientRoute, foundSecret.Namespace)
if err != nil {
log.Error(err, "Unable to retrieve route from OAuthClient", "route-name", secret.OAuthClientRoute)
return err
}

Check warning on line 181 in controllers/secretgenerator/secretgenerator_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/secretgenerator/secretgenerator_controller.go#L177-L181

Added lines #L177 - L181 were not covered by tests

// Generate OAuthClient for the generated secret
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 {
log.Error(err, "error creating oauth client resource. Recreate the Secret", "secret-name",
foundSecret.Name)

return err
}

Check warning on line 191 in controllers/secretgenerator/secretgenerator_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/secretgenerator/secretgenerator_controller.go#L184-L191

Added lines #L184 - L191 were not covered by tests

return nil

Check warning on line 193 in controllers/secretgenerator/secretgenerator_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/secretgenerator/secretgenerator_controller.go#L193

Added line #L193 was not covered by tests
}

// getRoute returns an OpenShift route object. It waits until the .spec.host value exists to avoid possible race conditions, fails otherwise.
Expand Down
153 changes: 153 additions & 0 deletions controllers/secretgenerator/secretgenerator_controller_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
package secretgenerator_test

import (
"context"
"github.com/onsi/gomega/gstruct"
"github.com/opendatahub-io/opendatahub-operator/v2/controllers/secretgenerator"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"

appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"
"k8s.io/apimachinery/pkg/runtime"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

"testing"

. "github.com/onsi/gomega"
)

func newFakeClient(objs ...client.Object) (client.Client, error) {

Check failure on line 25 in controllers/secretgenerator/secretgenerator_controller_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint

newFakeClient - result 1 (error) is always nil (unparam)
scheme := runtime.NewScheme()
utilruntime.Must(corev1.AddToScheme(scheme))
utilruntime.Must(appsv1.AddToScheme(scheme))

fakeMapper := meta.NewDefaultRESTMapper(scheme.PreferredVersionAllGroups())
for gvk := range scheme.AllKnownTypes() {
fakeMapper.Add(gvk, meta.RESTScopeNamespace)
}

return fake.NewClientBuilder().
WithScheme(scheme).
WithRESTMapper(fakeMapper).
WithObjects(objs...).
Build(),
nil
}

func TestGenerateSecret(t *testing.T) {
g := NewWithT(t)
ctx := context.Background()

secretName := "foo"
secretNs := "ns"

// secret expected to be found
existingSecret := corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: secretName,
Namespace: secretNs,
Labels: map[string]string{
"foo": "bar",
},
Annotations: map[string]string{
annotations.SecretNameAnnotation: "bar",
annotations.SecretTypeAnnotation: "random",
},
},
}

// secret to be generated
generatedSecret := corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: secretName + "-generated",
Namespace: secretNs,
},
}

cli, err := newFakeClient(&existingSecret)
g.Expect(err).ShouldNot(HaveOccurred())

r := secretgenerator.SecretGeneratorReconciler{
Client: cli,
}

_, err = r.Reconcile(ctx, reconcile.Request{
NamespacedName: types.NamespacedName{
Name: existingSecret.Name,
Namespace: existingSecret.Namespace,
},
})

g.Expect(err).ShouldNot(HaveOccurred())

err = cli.Get(ctx, client.ObjectKeyFromObject(&generatedSecret), &generatedSecret)
g.Expect(err).ShouldNot(HaveOccurred())

g.Expect(generatedSecret.StringData).To(
HaveKey(existingSecret.Annotations[annotations.SecretNameAnnotation]))
g.Expect(generatedSecret.Labels).To(
gstruct.MatchAllKeys(gstruct.Keys{
"foo": Equal("bar"),
}),
)
}

func TestExistingSecret(t *testing.T) {
g := NewWithT(t)
ctx := context.Background()

secretName := "foo"
secretNs := "ns"

// secret expected to be found
existingSecret := corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: secretName,
Namespace: secretNs,
Labels: map[string]string{
"foo": "bar",
},
Annotations: map[string]string{
annotations.SecretNameAnnotation: "bar",
annotations.SecretTypeAnnotation: "random",
},
},
}

// secret to be generated
generatedSecret := corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: secretName + "-generated",
Namespace: secretNs,
},
}

cli, err := newFakeClient(&existingSecret, &generatedSecret)
g.Expect(err).ShouldNot(HaveOccurred())

r := secretgenerator.SecretGeneratorReconciler{
Client: cli,
}

_, err = r.Reconcile(ctx, reconcile.Request{
NamespacedName: types.NamespacedName{
Name: existingSecret.Name,
Namespace: existingSecret.Namespace,
},
})

g.Expect(err).ShouldNot(HaveOccurred())

err = cli.Get(ctx, client.ObjectKeyFromObject(&generatedSecret), &generatedSecret)
g.Expect(err).ShouldNot(HaveOccurred())

// assuming an existing secret is left untouched
g.Expect(generatedSecret.Labels).To(BeEmpty())
g.Expect(generatedSecret.StringData).To(BeEmpty())
}

0 comments on commit 0cb1dba

Please sign in to comment.