Update go, dependencies and refactor controllers through splitting into multiple single responsibility controllers #156

jkhelil · 2023-10-25T15:08:34Z

Changes

Update go 1.21
Update github.com/tektoncd/operator to v0.68.0
Update k8s.io/api to v0.28.1
Update k8s.io/apiextensions-apiserver to v0.28.1
Update k8s.io/apimachinery to v0.28.1
Update k8s.io/client-go to v1.5.2 (tekton operator dependency, but replaced to v0.28.1 using replacement statement)
Update sigs.k8s.io/controller-runtime to v0.16.2
Rename ShipwrightBuildController to build controller
Add certificates controller to handle webhook certificates
Add buildstrategy controller
Update unit tests

Submitter Checklist

Includes tests if functionality changed/was added
Includes docs if changes are user-facing
Set a kind label on this PR
Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Update go 1.21
Update github.com/tektoncd/operator to v0.68.0
Update k8s.io/api to v0.28.1
Update k8s.io/apiextensions-apiserver to v0.28.1
Update k8s.io/apimachinery to v0.28.1
Update k8s.io/client-go to v1.5.2 (tekton operator dependency, but replaced to v0.28.1 using replacement statement)
Update sigs.k8s.io/controller-runtime to v0.16.2
Split ShipwrightBuildController
Introduce certificates controller to handle webhook certificates
Update unit tests

openshift-ci · 2023-10-25T15:08:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jkhelil. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

adambkaplan

Before moving forward with this PR, I want to experiment a bit with kubebuilder/controller-runtime/operator-sdk. This repo was generated from operator-sdk, and I'd like to keep it that way so it is easier for us to ship the operator. From past experience, kubebuilder is very opinionated when it comes to directory structure - I don't want to go down a path that breaks this.

adambkaplan · 2023-10-25T17:59:38Z

controllers/certificates/certificates_controller.go

+// executed, otherwise the regular deploy workflow takes place.
+func (r *CertificatesReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+	logger := r.Logger.WithValues("namespace", req.Namespace, "name", req.Name)
+	logger.Info("Starting resource reconciliation...")


Do we have a "debug" log level? This would otherwise generate a chatty log.

comment removed, we dont have debug level

adambkaplan · 2023-10-25T18:03:45Z

controllers/certificates/certificates_controller.go

+	}
+
+	// ReconcileCertManager
+	if common.BoolFromEnvVar(commonctrl.UseManagedWebhookCerts) {


Since an env var controls if we create certs or not, I think we should use the env var to determine if we run the controller in the first place.

adambkaplan · 2023-10-25T18:05:32Z

controllers/certificates/certificates_controller_test.go

+		err = r.Get(ctx, certificateName, &certmanager.Certificate{})
+		g.Expect(err).To(o.BeNil())


Shouldn't this eventually result in a not found error? Isn't the goal for the operator to delete the cert in this scenario?

You are right, well actually i reverted this because deleting the cr , will give an error on reconcile function, the object is not found, however reverting the test as it is, is given an error, looks like we can nolonger update the deletionTimestamp field.
Expected <*errors.errorString | 0xc000240500>: error: Unable to edit name: metadata.deletionTimestamp field is immutable { s: "error: Unable to edit name: metadata.deletionTimestamp field is immutable", } to be nil
To be honest i dont see how to fix this

adambkaplan · 2023-10-25T18:09:17Z

controllers/certificates/certificates_controller.go

+	return ctrl.NewControllerManagedBy(mgr).
+		For(&v1alpha1.ShipwrightBuild{}, builder.WithPredicates(predicate.Funcs{


Shouldn't this controller own/watch cert-manager components?

adambkaplan · 2023-10-25T18:15:28Z

controllers/shipwrightbuild/shipwrightbuild_controller.go

@@ -174,7 +206,7 @@ func (r *ShipwrightBuildReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	} else {
 		transformerfncs = append(transformerfncs, manifestival.InjectNamespace(targetNamespace))
 		transformerfncs = append(transformerfncs, common.DeploymentImages(images))
-		transformerfncs = append(transformerfncs, common.InjectAnnotations(CertManagerInjectAnnotationKey, fmt.Sprintf(CertManagerInjectAnnotationValueTemplate, targetNamespace), common.Overwrite, "CustomResourceDefinition"))
+		transformerfncs = append(transformerfncs, common.InjectAnnotations(commonctrl.CertManagerInjectAnnotationKey, fmt.Sprintf(commonctrl.CertManagerInjectAnnotationValueTemplate, targetNamespace), common.Overwrite, "CustomResourceDefinition"))


Perhaps add a code comment to clarify why were are adding cert manager annotations here?

Comment added

adambkaplan · 2023-10-25T18:20:33Z

controllers/certificates/certificates_controller.go

+	init := b.Status.Conditions == nil
+	if init {
+		b.Status.Conditions = make([]metav1.Condition, 0)
+		apimeta.SetStatusCondition(&b.Status.Conditions, metav1.Condition{
+			Type:    commonctrl.ConditionReady,
+			Status:  metav1.ConditionUnknown, // we just started trying to reconcile
+			Reason:  "Init",
+			Message: "Initializing Shipwright Operator",
+		})
+		if err := r.Client.Status().Update(ctx, b); err != nil {
+			return commonctrl.RequeueWithError(err)
+		}
+	}


Minor race condition risk - since two controllers are watching the same object, you might have both try to update the same object at the same time. This will result in a client "conflict" error, one will get requeued.

remove this logic from this controller, we keep it only on build controller

adambkaplan · 2023-10-25T18:21:50Z

controllers/certificates/certificates_controller.go

+	if err := r.Get(ctx, types.NamespacedName{Name: targetNamespace}, ns); err != nil {
+		if !errors.IsNotFound(err) {
+			logger.Info("retrieving target namespace %s error: %s", targetNamespace, err.Error())
+			return commonctrl.RequeueOnError(err)
+		}
+		ns.Name = targetNamespace
+
+		if err = r.Create(ctx, ns, &client.CreateOptions{Raw: &metav1.CreateOptions{}}); err != nil {
+			if !errors.IsAlreadyExists(err) {
+				logger.Info("creating target namespace %s error: %s", targetNamespace, err.Error())
+				return commonctrl.RequeueOnError(err)
+			}
+		}
+		logger.Info("created target namespace")
+	}


?Ditto race condition risk. Does it make sense to have the ShipwrightBuild controller create the target namespace, and this controller just requeue while it waits for the target namespace to be created?

fixed, the controller requeue waiting for namespace to be created

… from certificates controller shipwright build controller to reconcile shipwright build manifest certificates controller to reconcile webhook certificate

openshift-ci bot added the release-note label Oct 25, 2023

openshift-ci bot requested review from adambkaplan and HeavyWombat October 25, 2023 15:08

adambkaplan reviewed Oct 25, 2023

View reviewed changes

SaschaSchwarze0 mentioned this pull request Oct 26, 2023

Bump google.golang.org/grpc from 1.53.0 to 1.56.3 #157

Merged

bump go to 1.21 and update dependencies

e6f76ad

jkhelil force-pushed the refactor_controller branch 2 times, most recently from 6410d30 to 0732ce0 Compare October 27, 2023 08:26

refactor manager and controllers to split shipwright build controller…

92bc4f7

… from certificates controller shipwright build controller to reconcile shipwright build manifest certificates controller to reconcile webhook certificate

jkhelil force-pushed the refactor_controller branch 5 times, most recently from 5ed4097 to f0fa18f Compare October 27, 2023 09:35

jkhelil added 2 commits October 27, 2023 11:46

add buildstrategy controller

34c7593

bump go version on ci

4776244

jkhelil force-pushed the refactor_controller branch from f0fa18f to 4776244 Compare October 27, 2023 09:46

jkhelil closed this Oct 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update go, dependencies and refactor controllers through splitting into multiple single responsibility controllers #156

Update go, dependencies and refactor controllers through splitting into multiple single responsibility controllers #156

Uh oh!

jkhelil commented Oct 25, 2023 •

edited

Loading

Uh oh!

openshift-ci bot commented Oct 25, 2023

Uh oh!

adambkaplan left a comment

Uh oh!

adambkaplan Oct 25, 2023

Uh oh!

jkhelil Oct 27, 2023

Uh oh!

adambkaplan Oct 25, 2023

Uh oh!

jkhelil Oct 27, 2023

Uh oh!

adambkaplan Oct 25, 2023

Uh oh!

jkhelil Oct 27, 2023

Uh oh!

adambkaplan Oct 25, 2023

Uh oh!

adambkaplan Oct 25, 2023

Uh oh!

jkhelil Oct 27, 2023

Uh oh!

adambkaplan Oct 25, 2023

Uh oh!

jkhelil Oct 27, 2023

Uh oh!

adambkaplan Oct 25, 2023

Uh oh!

jkhelil Oct 27, 2023

Uh oh!

Uh oh!

		err = r.Get(ctx, certificateName, &certmanager.Certificate{})
		g.Expect(err).To(o.BeNil())

		return ctrl.NewControllerManagedBy(mgr).
		For(&v1alpha1.ShipwrightBuild{}, builder.WithPredicates(predicate.Funcs{

Update go, dependencies and refactor controllers through splitting into multiple single responsibility controllers #156

Update go, dependencies and refactor controllers through splitting into multiple single responsibility controllers #156

Uh oh!

Conversation

jkhelil commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

openshift-ci bot commented Oct 25, 2023

Uh oh!

adambkaplan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkhelil commented Oct 25, 2023 •

edited

Loading