Major refactoring & implement standalone mode #12

radito3 · 2024-01-31T16:26:27Z

Overhaul the code from a PoC to a semi-productive state.

Major changes done to the Processors, mainly the UpdateWatchConfig handler, which provisions k8s managers.

Simplify large parts of the codebase.

Handle context cancellation propagation properly.

Ensure resource cleanup on graceful shutdown - do not leave hanging goroutines.

Refactoring main points:

general refactoring and simplification
improve OOP
prefer composition over embedding
improve usage of context.Context
remove usage of most log.Fatal* calls
improve error handling
remove usage of channels in Processors as they aren't used in asynchronous code
reduce usage of goroutines when they aren't necessary
remove usage of global variables
remove unused structs, functions, variables
add graceful shutdown and resource cleanup
(WIP) add standalone mode so that the RWP can work outside of a k8s cluster

Details about context usage

Context flow between objects:

flowchart LR
root(Root Context) --> grpc(gRPC Client)
grpc --> hb(Heartbeat)
root --> proc(Processor)
proc --> mng(K8s Manager)

The root context traps the SIGINT and SIGTERM signals, so it can be used for graceful shutdown.
The gRPC client then wraps it with its own cancel() function, so it can stop on Send() or Recv() errors.
Additionally, it adds the Session ID and Binary Version header values to the context being sent to the AutoPi.
The client has a heartbeat in which it select's on the context Done() channel and a 30-second timer.

The processors accept the root context as a parameter to their Process method.
Currently, the context is only used in the UpdateWatchConfig handler, as it's needed for proper flow control of the K8s Manager(s) it spawns.
The processor builds the watched resources controllers, wraps the root context with its own cancel() function and starts the Manager with the wrapped context.
The internal cancellation will happen when the manager is stopped - in the case where a newer watch configuration is received and the manager needs to be recreated.
The manager will then block until either the context is cancelled or an error occurs within its controllers.

---
title: Flowchart for UpdateWatchConfig
---
stateDiagram-v2
[*] --> rootCtx
rootCtx : Create Root Context
rootCtx --> createDrainChan
createDrainChan : Create Drain Channel
createDrainChan --> recv
recv : Receive gRPC message
recv --> createProc
createProc : Create UpdateWatchConfig Processor
createProc --> createMng
createMng : Create K8s Manager
createMng --> startMng : Spawn new goroutine
startMng : Start K8s Manager
createMng --> sig : main goroutine
sig : Wait for termination via SIGTERM
sig --> CancelRootCtx
CancelRootCtx : Cancel Root Context
CancelRootCtx --> waitDrainCh
waitDrainCh : Wait on Drain Channel
waitDrainCh --> [*]
startMng --> drainCh : Root context cancellation propagated
drainCh : Send signal to Drain Channel
drainCh --> endG
endG : Goroutine returns

---
title: Flowchart for when the K8s manager fails to start
---
stateDiagram-v2
[*] --> begin
begin : Same as previous first steps
begin --> createMng
createMng : Create Manager
createMng --> startMng : Spawn first goroutine
startMng : Start Manager
startMng --> fail
fail : Error occurs
fail --> errToCh
errToCh : Send signal to Drain Channel
errToCh --> endG1
endG1 : Goroutine returns
createMng --> createMng2 : main goroutine
createMng2 : Receive new request and create Manager
createMng2 --> drainErr : Spawn second goroutine
drainErr : Drain channel for previous error
drainErr --> startMng2
startMng2 : Start Manager
startMng2 --> drainCh : Root context cancellation propagated
drainCh : Send signal to Drain Channel
drainCh --> endG2
endG2 : Goroutine returns
createMng2 --> sig
sig : Wait for termination via SIGTERM
sig --> CancelRootCtx
CancelRootCtx : Cancel Root Context
CancelRootCtx --> waitDrainCh
waitDrainCh : Wait on Drain Channel
waitDrainCh --> [*]

Overhaul the code from a PoC to a semi-productive state. Major changes done to the Processors, mainly the UpdateWatchConfig handler, which provisions k8s managers. Simplify large parts of the codebase. Handle context cancellation propagation properly. Ensure resource cleanup on graceful shutdown - do not leave hanging goroutines.

cmd/remote-work-processor/main.go

internal/executors/errors.go

internal/executors/factory/executor_factory.go

internal/executors/http/authorization_header.go

internal/executors/http/tls/tls_configuration_provider.go

internal/grpc/client_metadata.go

internal/grpc/processors/disable_processor.go

internal/kubernetes/controller/manager-builder.go

internal/kubernetes/controller/controller.go

Nizamski · 2024-02-02T13:02:53Z

cmd/remote-work-processor/main.go

+	for connAttempts < maxRetries {
+		select {
+		case <-rootCtx.Done():
+			break


Are you sure that this break is not applicable only for select statement?

Nizamski · 2024-02-02T13:46:40Z

cmd/remote-work-processor/main.go

-		if res.Err != nil {
-			log.Fatalf("Error occurred while processing operation: %v\n", err)
-		}
+	isInStandaloneMode, _ := strconv.ParseBool(flag.Lookup(standaloneModeOpt).Value.String())


What will happen in case standalone-mode flag is not passed as boolean, e.g unintentionally? Shouldn't we need to handle the possible error?

The flag package handles parsing errors on the flags. If something that is not a boolean is provided for --standalone-mode it will fail with a parse error.

cmd/remote-work-processor/main.go

Fix comments Exit Pod with status 1 in case the Manager can't be started (in K8s mode) Remove cahing of auth header in HTTP executor Improvement of error messages

internal/executors/http/basic_authorization_header.go

VaskoBozhurski · 2024-03-12T12:59:20Z

internal/executors/http/http_response.go

-	codes, err2 := parseSuccessResponseCodes(successResponseCodes...)
-	if err2 != nil {
-		return false, err2
+func isSuccessfulResponseCode(statusCode int, successResponseCodes ...string) (bool, error) {


What's our reason for having the successResponseCodes as string? From what I can see, we always try to convert them to integers. Any reason why we can't just do the conversion at the top-most location where they are first inputed and then only pass them as integer slices?

Looking at HttpRequestParameters we already have non-string fields there, so why can't we do the whole string to integer there? Would also mean we get an error when we are building the request parameters and not when we are all the way at checking the response.

Maybe I'm missing something?

From AutoPi, the success codes are interpreted as strings because they can be "2xx", not just "200", "404", etc.

VaskoBozhurski · 2024-03-12T13:10:16Z

internal/executors/http/oauth_header_generator.go

+	h.fetcher = NewOAuthTokenFetcher(
+		withExecutor(executor),
+		withTokenUrl(tokenUrl),
+		withRequestBody(requestBody),
+		withCertificateAuthentication(h.certAuthentication),
+		withAuthHeader(h.authHeader),
+	)


Are these with* arguments all optional? Can we make an OAuth TokenFetcher without an URL for instance? Feels weird to me to have a builder pattern where non-optional fields are in fact optional (compilation wise) as any errors would be discovered on runtime rather than on the much more preferred compile time.

Can we perhaps consider making the always required parameters as concrete parameters of the function and the optional ones (I think f.certAuthentication might be?) to be left as the optionals at the end of the function?

As an example the NewOAuthorizationHeaderGenerator seems to be written in such a way.

VaskoBozhurski · 2024-03-12T13:22:36Z

internal/kubernetes/controller/controller_builder.go

+		c.selector.LabelSelector.Matches(labels.Set(o.GetLabels())) &&
+		c.selector.FieldSelector.Matches(o)


Potential nil pointer access if withSelector is never called and c.selector is left as nil. This currently isn't as issue as the only place it is used will always call it, but it still leaves a potential landmine in any future code refactoring/modification.

VaskoBozhurski · 2024-03-12T13:24:41Z

internal/kubernetes/controller/controller_builder.go

+func (c *ControllerBuilder) WithReconcilicationPeriodInMinutes(period int32) *ControllerBuilder {
+	c.reconciliationPeriodInMinutes = period
+	return c
+}
+
+func (c *ControllerBuilder) WithSelector(selector *selector.Selector) *ControllerBuilder {
+	c.selector = selector
+	return c
+}
+
+func (c *ControllerBuilder) ManagedBy(manager *Manager) *ControllerBuilder {
+	c.manager = manager
+	return c
+}


Again, are these fields really optional or are they always required in order for the code to work?

If they are always required, I dislike the idea of having them as optional parameters just so we save some horizontal space (in return for using more vertical one) and introducing potential future runtime errors.

What would you say to adding a validation in the build() method of all builders that checks for the required fields, and if any aren't set to return an error? This would be semantically similar to the Immutables library for Java.

It would be an improvement, but it would still be a runtime check rather than a compile time error. My main gripe with required "optionals" is that you have no compile time information that you forgot to set a field or not. If you're worried about the "constructor" function having too much horizontal space taken, you can always format it as:

func example( firstRequired string, secondRequired int, thirdRequired byte, optionals ...Option ) { <code> }

VaskoBozhurski · 2024-03-12T13:28:22Z

internal/kubernetes/controller/manager_builder.go

+func (b *ManagerBuilder) SetGrpcClient(client *grpc.RemoteWorkProcessorGrpcClient) *ManagerBuilder {
+	b.grpcClient = client
+	return b
+}
+
+func (b *ManagerBuilder) BuildDynamicClient(config *rest.Config) *ManagerBuilder {
+	dc, err := dynamic.NewDynamicClient(config)
+	if err != nil {
+		log.Panicln("Failed to create dynamic client:", err)
+	}
+	b.dynamicClient = dc
+	return b
+}
+
+func (b *ManagerBuilder) BuildInternalManager(config *rest.Config, scheme *runtime.Scheme) *ManagerBuilder {
+	t := time.Duration(0)
+	options := manager.Options{
+		Scheme:                  scheme,
+		GracefulShutdownTimeout: &t,
+		WebhookServer:           nil,
+		LeaderElection:          false,
+		HealthProbeBindAddress:  "localhost:8811",
+		MetricsBindAddress:      "0",
+	}
+
+	mgr, err := ctrl.NewManager(config, options)
+	if err != nil {
+		log.Panicln("Failed to create manager:", err)
+	}
+
+	// these can fail only if the manager has been started prior to calling them
+	mgr.AddHealthzCheck("healthz", healthz.Ping)
+	mgr.AddReadyzCheck("readyz", healthz.Ping)
+	b.delegate = mgr
+	return b
+}


Looking at these, the only reason they are a struct method instead of a separate function is so they can assign the result in the end. Any reason why they are optionals again and are they really optional in our case? Any reason they can't be separate functions that have their results passed as arguments to the actual build function so that we can avoid having optional parameters that are actually always needed?

VaskoBozhurski · 2024-03-21T13:47:33Z

internal/kubernetes/controller/manager_builder.go

@@ -60,6 +60,9 @@ func (b *ManagerBuilder) BuildInternalManager(config *rest.Config, scheme *runti
 }

 func (b *ManagerBuilder) Build() Manager {
+	if b.delegate == nil || b.dynamicClient == nil || b.grpcClient == nil {
+		panic("Manager is missing required parameters")


Maybe we can have the build return a (*Manager, error) so that error is handled by the upper function WatchResources which already has a flow handling exiting the app in case of error?

radito3 added 2 commits January 31, 2024 16:49

Start gRPC stream with proper context

c5671ef

VaskoBozhurski reviewed Feb 1, 2024

View reviewed changes

Nizamski reviewed Feb 2, 2024

View reviewed changes

radito3 added 3 commits February 16, 2024 16:43

Refactoring part 2

e95b65c

Fix comments Exit Pod with status 1 in case the Manager can't be started (in K8s mode) Remove cahing of auth header in HTTP executor Improvement of error messages

Fix client cert creation in standalone mode

4c8d16e

Update env vars names

9e102da

VaskoBozhurski approved these changes Feb 22, 2024

View reviewed changes

VaskoBozhurski previously approved these changes Feb 22, 2024

View reviewed changes

Nizamski previously approved these changes Feb 23, 2024

View reviewed changes

Improve logging and fix some issues

ef30518

radito3 dismissed stale reviews from Nizamski and VaskoBozhurski via ef30518 February 26, 2024 12:59

radito3 added 8 commits March 2, 2024 11:43

Handle session auto-config better

8fd22a9

Delete redundant cache struct

0039d68

Add some more logs to task processors

1635f20

Fix k8s managers and controllers

1b3b49f

Renaming

020e6b8

Fix some TODOs

cbf5325

Fix build

b53a519

Refactoring final: Reimplement auth header caching properly

8188bd5

radito3 changed the title ~~Major refactoring & start work on standalone mode (part 1)~~ Major refactoring & implement standalone mode Mar 11, 2024

radito3 added 2 commits March 12, 2024 12:51

Small cleanup

b0d8448

Bump vulnerable dependencies

3890b05

VaskoBozhurski reviewed Mar 12, 2024

View reviewed changes

radito3 added 2 commits March 17, 2024 16:00

Fix basic auth header

54b7f33

Fix comments

77b7136

VaskoBozhurski reviewed Mar 21, 2024

View reviewed changes

fix comments pt 2

08e5471

VaskoBozhurski approved these changes Mar 21, 2024

View reviewed changes

Nizamski approved these changes Mar 25, 2024

View reviewed changes

radito3 merged commit 433c35f into main Mar 25, 2024
5 checks passed

radito3 deleted the refactor branch March 25, 2024 11:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major refactoring & implement standalone mode #12

Major refactoring & implement standalone mode #12

radito3 commented Jan 31, 2024 •

edited

Loading

Nizamski Feb 2, 2024

Nizamski Feb 2, 2024

radito3 Feb 16, 2024

VaskoBozhurski Mar 12, 2024

radito3 Mar 17, 2024

VaskoBozhurski Mar 12, 2024

VaskoBozhurski Mar 12, 2024

VaskoBozhurski Mar 12, 2024

radito3 Mar 12, 2024

VaskoBozhurski Mar 12, 2024

VaskoBozhurski Mar 12, 2024

VaskoBozhurski Mar 21, 2024

		c.selector.LabelSelector.Matches(labels.Set(o.GetLabels())) &&
		c.selector.FieldSelector.Matches(o)

Major refactoring & implement standalone mode #12

Major refactoring & implement standalone mode #12

Conversation

radito3 commented Jan 31, 2024 • edited Loading

Details about context usage

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radito3 commented Jan 31, 2024 •

edited

Loading