Refactor/remove private dns zone

[AldoFusterTurpin]

This PR was already opened from my fork but we need to use branches directly in the upstream.

Which issue this PR addresses:

This PR corresponds to the task Identify and implement POC of the code to introduce Clean Architecture.

What this PR does / why we need it:

This PR does not represent the direct and exact application of Clean Architecture as it would be very difficult due to the nature and complexity of the project but instead, it moves in the right direction to improve the health of the codebase to have something more similar to Clean Architecture.

I have performed a presentation talking about key topics of these PR changes and about some principles that can help the Design of the project in the long term to have a shape more similar to Clean Architecture improving what we have today.

The other PR is also part of the same task. Using both PRs I presented key topics that move the direction of the design towards Clean Architecture.

This PR contains > 1 commit so it is easier to navigate and understand. The idea is to squash them when merging but right now I want it that way in case I need to refer to a particular step in the presentation.

Test plan for issue:

Unit tests exist and new ones created. I performed small steps in each commit to not mess up things.

Is there any documentation that needs to be updated for this PR?

N/A.

---

Optional detailed explanation (covered in the demo):

Problems in pkg/cluster/removeprivatednszone.go

First block of code

• Notes:
  • Usage of anonymous functions:
    • Not easy to read and maintain.
    • Can't be independently tested.
    • Can't be reused.
  • Reinstantiation of already computed variable (m.configcli.ConfigV1().DNSes()).
    
    
    

Second block of code

• Notes:
  • We can identify clear chunks/blocks of code: get objects and perform business logic.
  • We identify the narrow scope of some variables: mcps, machineCount and nodes.
    
    
    

Third block of code

• Notes:
  • We see the narrow scope of v and the business logic performed using it.
  • We find the same chunk of code (line 105 - 120) exactly as before. DRY.
    
    
    

Fourth block of code

• Notes:
  • We lost track about where zones is coming from.
  • We just iterate over zones and trigger some side effects.
  • No business logic, it is delegated to azure.ParseResourceID(...).
    
    
    

Problems:

• Function too large to understand in a single "quick" (overview) read.

• Mix of type of work in one single function/component.

  • Access data (get from external components).
  • Perform business logic.
  • Trigger side effects.
  • Why is it defined in the manager struct (of the cluster package)?

Improvement process

• After the first refactor commit("refactor: split in narrow functions and remove duplication", commit ID: cbf7):

  • IMHO we have performed a big improvement:
    • The main function of the file is easy to follow and understand in a quick look.
    • We went from one function with 128 LOC to now having functions that are around 20 LOC each of them.
    • Each function has its own responsibility, no mix of "tasks" as before.
    • We have removed duplication of code.
    • We have improved out ability to break our component into other subcomponents (structs/interfaces/functions) to me moved away.
  • But we still have some problems with the actual code:

    • We have, clusterVersionIsAtLeast4_4 and validateMachineConfigPoolsAndGetCounter which both perform business logic and at the same time write things into a logger entry. We are forced to do that because we don't treat "cluster version < 4.4, not removing private DNS zone" as an error.

      If when given the condition if v.Lt(version.NewVersion(4, 4)) { it created an error with the above message, we would be able to treat all the branches inside this function as errors, and we would simply return that error and handle it appropiately outside this function. Doing that, we would be able to move the "write" part outside the function.

      The same happens with the function validateMachineConfigPoolsAndGetCounter, the original code doesn't treat as errors the fact that we are entering the "ifs", it simply writes it to the logger. Returning an error with those messages would be a lot better, we would get rid of the logger parameter in the function. The caller would be responsible to handle the error as "it" wants, so we wouldn't force anyone to write anywhere the error message (because now we have conceptually an error, but we don't use the errors capabilities of Go).

      Another option would be to pass a Writer interface in the function signature and implement the interface in the manager struct of the cluster to be able to pass it to the function.

    • We are overloading too much the *manager type of the cluster package. It would be a lot better to break that huge manager struct into multiple structs and move all the DNS/networking thing to a different struct or to an existing component/package. Single Responsibility Principle

• After the refactor commit("refactor: move functions from cluster package to net package", commit ID: 89be):

  • We have moved everything related to DNS/network/etc to the pkg/util/net package as it is the responsibility of that package to handle that type of things.
  • We extract m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID to a variable
  • We still have two problems:
    • clusterHasSameNumberOfNodesAndMachineConfigPools and validateMachineConfigPoolsAndGetCounter do not treat errors as such (original code was like this) they return a boolean to check from the caller if anything went wrong.
    • the above functions receive the log as parameter, forcing the function to write to I/O even if we don't want (and passing a nil argument will cause a panic).
  • Let's fix the previous problems removing the logger as a parameter and returning errors instead of booleans and handling the errors properly from the caller.

• After the last refactor commit("refactor: remove log in parameters and rename functions", commit ID: 0636):

  • We have removed the dependency of the logger parameter and we are treating errors as such.
  • We still don't modify the behavior of our outermost function *manager.removePrivateDNSZone() as it always returns nil (as the original version). It could be tempting to return the errors instead of nil, but this is a refactor and we don't know the implications this change could have, so it is not 100% safe to do so.
  • The function ClusterVersionIsGreaterThan4_3 has been renamed and refactored to not treat as an error the fact that the version is less than 4.3, this is something the caller should decide.

Update:
I needed to update pkg/cluster/removeprivatednszone.go (rebase master) due to this PR.
So the function now returns an error and a boolean. It is curious that in the previous PR we just return one of the errors (cv, err) and for the other one (v, err) we just log it but return nil. Strange, but I want to keep the same behavior in the refactoring.

[AldoFusterTurpin]

/azp run ci

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mociarain]

This is great. It's much clearer and easier for me (a newbie to Go and the repo as a whole) to follow and understand.

[AldoFusterTurpin]

/azp run ci

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mociarain]

/azp run e2e

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[mociarain]

/azp run ci

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[github-actions]

Please rebase pull request.

[mociarain]

/azp run ci,e2e

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).