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

🐛 Wrap service account error to hide k8s error #1698

Merged
merged 1 commit into from
Feb 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 20 additions & 0 deletions internal/authentication/tokengetter.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ package authentication

import (
"context"
"fmt"
"sync"
"time"

authenticationv1 "k8s.io/api/authentication/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
corev1 "k8s.io/client-go/kubernetes/typed/core/v1"
Expand All @@ -19,6 +21,21 @@ type TokenGetter struct {
mu sync.RWMutex
}

type ServiceAccountNotFoundError struct {
ServiceAccountName string
ServiceAccountNamespace string
Err error
}

func (e *ServiceAccountNotFoundError) Unwrap() error {
return e.Err
}

// Error implements the error interface for ServiceAccountNotFoundError.
func (e *ServiceAccountNotFoundError) Error() string {
return fmt.Sprintf("service account \"%s\" not found in namespace \"%s\": unable to authenticate with the Kubernetes cluster.", e.ServiceAccountName, e.ServiceAccountNamespace)
}

type TokenGetterOption func(*TokenGetter)

const (
Expand Down Expand Up @@ -86,6 +103,9 @@ func (t *TokenGetter) getToken(ctx context.Context, key types.NamespacedName) (*
Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To(int64(t.expirationDuration / time.Second))},
}, metav1.CreateOptions{})
if err != nil {
if errors.IsNotFound(err) {
return nil, &ServiceAccountNotFoundError{ServiceAccountName: key.Name, ServiceAccountNamespace: key.Namespace}
}
return nil, err
}
return &req.Status, nil
Expand Down
4 changes: 3 additions & 1 deletion internal/authentication/tokengetter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,15 @@ func TestTokenGetterGet(t *testing.T) {
"test-namespace-3", "test-token-3", "failed to get token"},
{"Testing error when getting token from fake client", "test-service-account-4",
"test-namespace-4", "error when fetching token", "error when fetching token"},
{"Testing service account not found", "missing-sa",
"test-namespace-5", "", "service account \"missing-sa\" not found in namespace \"test-namespace-5\": unable to authenticate with the Kubernetes cluster."},
}

for _, tc := range tests {
got, err := tg.Get(context.Background(), types.NamespacedName{Namespace: tc.namespace, Name: tc.serviceAccountName})
if err != nil {
t.Logf("%s: expected: %v, got: %v", tc.testName, tc.want, err)
assert.EqualError(t, err, tc.errorMsg)
assert.EqualError(t, err, tc.errorMsg, "Error message should match expected output")
} else {
t.Logf("%s: expected: %v, got: %v", tc.testName, tc.want, got)
assert.Equal(t, tc.want, got, tc.errorMsg)
Expand Down
7 changes: 7 additions & 0 deletions internal/controllers/clusterextension_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import (

ocv1 "github.com/operator-framework/operator-controller/api/v1"
catalogd "github.com/operator-framework/operator-controller/catalogd/api/v1"
"github.com/operator-framework/operator-controller/internal/authentication"
"github.com/operator-framework/operator-controller/internal/bundleutil"
"github.com/operator-framework/operator-controller/internal/conditionsets"
"github.com/operator-framework/operator-controller/internal/contentmanager"
Expand Down Expand Up @@ -206,6 +207,12 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl
installedBundle, err := r.InstalledBundleGetter.GetInstalledBundle(ctx, ext)
if err != nil {
setInstallStatus(ext, nil)
var saerr *authentication.ServiceAccountNotFoundError
if errors.As(err, &saerr) {
setInstalledStatusConditionUnknown(ext, saerr.Error())
setStatusProgressing(ext, errors.New("installation cannot proceed due to missing ServiceAccount"))
return ctrl.Result{}, err
}
setInstalledStatusConditionUnknown(ext, err.Error())
setStatusProgressing(ext, errors.New("retrying to get installed bundle"))
return ctrl.Result{}, err
Expand Down
55 changes: 55 additions & 0 deletions internal/controllers/clusterextension_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/operator-framework/operator-registry/alpha/declcfg"

ocv1 "github.com/operator-framework/operator-controller/api/v1"
"github.com/operator-framework/operator-controller/internal/authentication"
"github.com/operator-framework/operator-controller/internal/conditionsets"
"github.com/operator-framework/operator-controller/internal/controllers"
"github.com/operator-framework/operator-controller/internal/finalizers"
Expand Down Expand Up @@ -332,6 +333,60 @@ func TestClusterExtensionResolutionAndUnpackSuccessfulApplierFails(t *testing.T)
require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{}))
}

func TestClusterExtensionServiceAccountNotFound(t *testing.T) {
cl, reconciler := newClientAndReconciler(t)
reconciler.InstalledBundleGetter = &MockInstalledBundleGetter{
err: &authentication.ServiceAccountNotFoundError{
ServiceAccountName: "missing-sa",
ServiceAccountNamespace: "default",
}}

ctx := context.Background()
extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))}

t.Log("Given a cluster extension with a missing service account")
clusterExtension := &ocv1.ClusterExtension{
ObjectMeta: metav1.ObjectMeta{Name: extKey.Name},
Spec: ocv1.ClusterExtensionSpec{
Source: ocv1.SourceConfig{
SourceType: "Catalog",
Catalog: &ocv1.CatalogSource{
PackageName: "test-package",
},
},
Namespace: "default",
ServiceAccount: ocv1.ServiceAccountReference{
Name: "missing-sa",
},
},
}

require.NoError(t, cl.Create(ctx, clusterExtension))

t.Log("When reconciling the cluster extension")
res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey})

require.Equal(t, ctrl.Result{}, res)
require.Error(t, err)
require.IsType(t, &authentication.ServiceAccountNotFoundError{}, err)
t.Log("By fetching updated cluster extension after reconcile")
require.NoError(t, cl.Get(ctx, extKey, clusterExtension))

t.Log("By checking the status conditions")
installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled)
require.NotNil(t, installedCond)
require.Equal(t, metav1.ConditionUnknown, installedCond.Status)
require.Contains(t, installedCond.Message, fmt.Sprintf("service account %q not found in namespace %q: unable to authenticate with the Kubernetes cluster.",
"missing-sa", "default"))

progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeProgressing)
require.NotNil(t, progressingCond)
require.Equal(t, metav1.ConditionTrue, progressingCond.Status)
require.Equal(t, ocv1.ReasonRetrying, progressingCond.Reason)
require.Contains(t, progressingCond.Message, "installation cannot proceed due to missing ServiceAccount")
require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{}))
}

func TestClusterExtensionApplierFailsWithBundleInstalled(t *testing.T) {
cl, reconciler := newClientAndReconciler(t)
reconciler.Unpacker = &MockUnpacker{
Expand Down
3 changes: 2 additions & 1 deletion internal/controllers/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,15 @@ func newClient(t *testing.T) client.Client {

type MockInstalledBundleGetter struct {
bundle *controllers.InstalledBundle
err error
}

func (m *MockInstalledBundleGetter) SetBundle(bundle *controllers.InstalledBundle) {
m.bundle = bundle
}

func (m *MockInstalledBundleGetter) GetInstalledBundle(ctx context.Context, ext *ocv1.ClusterExtension) (*controllers.InstalledBundle, error) {
return m.bundle, nil
return m.bundle, m.err
}

var _ controllers.Applier = (*MockApplier)(nil)
Expand Down
Loading