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

ARO-3258: propagate errors of ARO PullSecret controller to ARO operator #3947

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

ARO-3258: propagate errors of ARO PullSecret controller to ARO operator #3947

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3367bf6
Propagate errors of ARO PullSecret controller to ARO cluster operator
lbossis Nov 5, 2024
c3ca7b2
Added few asserts
lbossis Nov 7, 2024
ade1f7a
Remove whitespace from the import section to please lint
lbossis Nov 8, 2024
f2991ab
Code review comments have been addressed
lbossis Nov 15, 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
45 changes: 26 additions & 19 deletions pkg/operator/controllers/pullsecret/pullsecret_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (

"github.com/Azure/ARO-RP/pkg/operator"
arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
"github.com/Azure/ARO-RP/pkg/operator/controllers/base"
"github.com/Azure/ARO-RP/pkg/operator/predicates"
"github.com/Azure/ARO-RP/pkg/util/pullsecret"
)
Expand All @@ -43,15 +44,16 @@ var rhKeys = []string{"registry.redhat.io", "cloud.openshift.com", "registry.con

// Reconciler reconciles a Cluster object
type Reconciler struct {
log *logrus.Entry

client client.Client
base.AROController
}

func NewReconciler(log *logrus.Entry, client client.Client) *Reconciler {
return &Reconciler{
log: log,
client: client,
AROController: base.AROController{
Log: log,
Client: client,
Name: ControllerName,
},
}
}

Expand All @@ -65,36 +67,39 @@ func NewReconciler(log *logrus.Entry, client client.Client) *Reconciler {
// - If the pull Secret object (which is not owned by the Cluster object)
// changes, we'll see the pull Secret object requested.
func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) {
instance := &arov1alpha1.Cluster{}
err := r.client.Get(ctx, types.NamespacedName{Name: arov1alpha1.SingletonClusterName}, instance)
instance, err := r.GetCluster(ctx)
if err != nil {
r.Log.Error(err)
return reconcile.Result{}, err
}

if !instance.Spec.OperatorFlags.GetSimpleBoolean(operator.PullSecretEnabled) {
r.log.Debug("controller is disabled")
r.Log.Debug("controller is disabled")
return reconcile.Result{}, nil
}

r.log.Debug("running")
r.Log.Debug("running")
userSecret := &corev1.Secret{}
err = r.client.Get(ctx, pullSecretName, userSecret)
err = r.Client.Get(ctx, pullSecretName, userSecret)
if err != nil && !kerrors.IsNotFound(err) {
r.Log.Error(err)
return reconcile.Result{}, err
}

// reconcile global pull secret
// detects if the global pull secret is broken and fixes it by using backup managed by ARO operator
if instance.Spec.OperatorFlags.GetSimpleBoolean(operator.PullSecretManaged) {
operatorSecret := &corev1.Secret{}
err = r.client.Get(ctx, types.NamespacedName{Namespace: operator.Namespace, Name: operator.SecretName}, operatorSecret)
err = r.Client.Get(ctx, types.NamespacedName{Namespace: operator.Namespace, Name: operator.SecretName}, operatorSecret)
if err != nil {
r.Log.Error(err)
return reconcile.Result{}, err
}

// fix pull secret if its broken to have at least the ARO pull secret
userSecret, err = r.ensureGlobalPullSecret(ctx, operatorSecret, userSecret)
if err != nil {
r.Log.Error(err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably want to SetDegraded here if we can't ensure the global pull secret. Here's an example: https://github.com/Azure/ARO-RP/pull/3177/files#diff-17756e345e334809c4f7ea3663119196d4de8e8085aca1b0da6a96e708d5d251R84

Copy link
Collaborator

Choose a reason for hiding this comment

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

Resolving as we've agreed in the JIRA to not set degraded as part of this PR as it could impact user's ability to rotate their pull secret.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can safely SetDegraded here without impacting any functionality. The controller will still continue to execute as it would normally, and the controller Degraded status will (as of right now) not be cascaded to the ARO Cluster Operator itself, so this controller being degraded will not e.g. block cluster upgrades.

return reconcile.Result{}, err
}
}
Expand All @@ -104,10 +109,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
// - list of Red Hat pull-secret keys in status.
instance.Status.RedHatKeysPresent, err = r.parseRedHatKeys(userSecret)
if err != nil {
r.Log.Error(err)
return reconcile.Result{}, err
}

err = r.client.Update(ctx, instance)
err = r.Client.Update(ctx, instance)
return reconcile.Result{}, err
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should validate if err is nil or not here, and:

  • if err is nil, call the ClearConditions function to reset the controller back to Available: True, Progressing: False, Degraded: False
  • if err is not nil, call SetDegraded to set the degraded status.

}

Expand Down Expand Up @@ -175,22 +181,23 @@ func (r *Reconciler) ensureGlobalPullSecret(ctx context.Context, operatorSecret,
// delete possible existing userSecret, calling deletion every time and ignoring when secret not found
// allows for simpler logic flow, when delete and create are not handled separately
// this call happens only when there is a need to change, it has no significant impact on performance
err := r.client.Delete(ctx, secret)
r.log.Info("Global Pull secret Not Found, Creating Again")
err := r.Client.Delete(ctx, secret)
r.Log.Info("Global Pull secret Not Found, Creating Again")
if err != nil && !kerrors.IsNotFound(err) {
r.Log.Error(err)
return nil, err
}

err = r.client.Create(ctx, secret)
err = r.Client.Create(ctx, secret)
if err == nil {
r.log.Info("Global Pull secret Created")
r.Log.Info("Global Pull secret Created")
}
return secret, err
}

err = r.client.Update(ctx, secret)
err = r.Client.Update(ctx, secret)
if err == nil {
r.log.Info("Updated Existing Global Pull secret")
r.Log.Info("Updated Existing Global Pull secret")
}
return secret, err
}
Expand All @@ -205,7 +212,7 @@ func (r *Reconciler) parseRedHatKeys(secret *corev1.Secret) (foundKeys []string,
// parse keys and validate JSON
parsedKeys, err := pullsecret.UnmarshalSecretData(secret)
if err != nil {
r.log.Info("pull secret is not valid json - recreating")
r.Log.Info("pull secret is not valid json - recreating")
return foundKeys, err
}

Expand Down
25 changes: 13 additions & 12 deletions pkg/operator/controllers/pullsecret/pullsecret_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"testing"

"github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -285,11 +286,11 @@ func TestPullSecretReconciler(t *testing.T) {
ctx := context.Background()

clientFake := ctrlfake.NewClientBuilder().WithObjects(tt.instance).WithObjects(tt.secrets...).Build()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also add validation in these tests to ensure that the conditions on the controller are what we expect (e.g. Degraded:True when an error occurs). You can see an example of this in the dnsmasq machine config controller tests: https://github.com/Azure/ARO-RP/blob/master/pkg/operator/controllers/dnsmasq/machineconfig_controller_test.go#L36

assert.NotNil(t, clientFake)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to add these extra assert calls here? The test works as-is without them added.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should be helpful to manifest nil object occurrence whenever that object is supposed to be not nil and thus to render test case failure early on.


r := NewReconciler(logrus.NewEntry(logrus.StandardLogger()), clientFake)
assert.NotNil(t, r)

r := &Reconciler{
log: logrus.NewEntry(logrus.StandardLogger()),
client: clientFake,
}
if tt.request.Name == "" {
tt.request.NamespacedName = pullSecretName
}
Expand All @@ -301,7 +302,8 @@ func TestPullSecretReconciler(t *testing.T) {
}

s := &corev1.Secret{}
err = r.client.Get(ctx, types.NamespacedName{Namespace: "openshift-config", Name: "pull-secret"}, s)
assert.NotNil(t, s)
err = r.Client.Get(ctx, types.NamespacedName{Namespace: "openshift-config", Name: "pull-secret"}, s)
if err != nil {
t.Error(err)
}
Expand All @@ -315,6 +317,7 @@ func TestPullSecretReconciler(t *testing.T) {
}

cluster := &arov1alpha1.Cluster{}
assert.NotNil(t, cluster)
err = clientFake.Get(ctx, types.NamespacedName{Name: arov1alpha1.SingletonClusterName}, cluster)
if err != nil {
t.Fatal("Error found")
Expand Down Expand Up @@ -363,9 +366,8 @@ func TestParseRedHatKeys(t *testing.T) {

for _, tt := range test {
t.Run(tt.name, func(t *testing.T) {
r := &Reconciler{
log: logrus.NewEntry(logrus.StandardLogger()),
}
r := NewReconciler(logrus.NewEntry(logrus.StandardLogger()), nil)
assert.NotNil(t, r)

out, err := r.parseRedHatKeys(tt.ps)
utilerror.AssertErrorMessage(t, err, tt.wantErr)
Expand Down Expand Up @@ -760,16 +762,15 @@ func TestEnsureGlobalPullSecret(t *testing.T) {
for _, tt := range test {
t.Run(tt.name, func(t *testing.T) {
ctx := context.Background()
assert.NotNil(t, ctx)

clientBuilder := ctrlfake.NewClientBuilder()
if tt.initialSecret != nil {
clientBuilder = clientBuilder.WithObjects(tt.initialSecret)
}

r := &Reconciler{
client: clientBuilder.Build(),
log: logrus.NewEntry(logrus.StandardLogger()),
}
r := NewReconciler(logrus.NewEntry(logrus.StandardLogger()), clientBuilder.Build())
assert.NotNil(t, r)

s, err := r.ensureGlobalPullSecret(ctx, tt.operatorPullSecret, tt.pullSecret)
utilerror.AssertErrorMessage(t, err, tt.wantError)
Expand Down
Loading