WIP: HIVE-2433: add a new privatelink controller #2290
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jstuever: This pull request references HIVE-2433 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jstuever The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2290 +/- ##
=======================================
Coverage 58.54% 58.54%
=======================================
Files 182 182
Lines 25829 25830 +1
=======================================
+ Hits 15122 15123 +1
Misses 9431 9431
Partials 1276 1276
|
| privatelink.ControllerName: privatelink.Add, | ||
| awsprivatelink.ControllerName: awsprivatelink.Add, |
There was a problem hiding this comment.
Right away we'll want logic to mutex these two, probably here-ish, possibly at the behest of a FeatureGate; but I'd be just as happy doing it with an env var since we'll probably cut over as soon as the new thing is proven for AWS.
| logger log.FieldLogger) error { | ||
|
|
||
| curr := &hivev1.ClusterDeployment{} | ||
| errGet := client.Get(context.TODO(), types.NamespacedName{Namespace: cd.Namespace, Name: cd.Name}, curr) |
| return reconcile.Result{}, nil | ||
| } | ||
| privateLinkEnabled = cd.Spec.Platform.AWS.PrivateLink.Enabled | ||
| clientActuator, err = r.GetActuator(configv1.AWSPlatformType, logger) |
There was a problem hiding this comment.
I suspect it'll wind up being easier to make all of the actuators part of the reconciler struct vs trying to pass them around as func args. It also may make it easier to mock them out for UT.
There was a problem hiding this comment.
The (now) spokeActuator is specific to each individual CD, having cloud-specific code and client service account credentials. We could create actuators for each cloud and pass around a client instead.
There was a problem hiding this comment.
As discussed, two actuators:
- Hub actuator for the RH-owned infra that hive is running on. Only AWS supported for the current pass.
- Spoke actuator tied to the cloud the spoke is running on, with two cloud clients:
- RH-owned infra for the "link"
- Customer-owned infra for the, uhh, "endpoint"?
...and we can't do what I suggested here because the reconciler is a singleton for the controller instance, and sharing it among CDs would be Bad™.
| // Add finalizer if not already present | ||
| if !controllerutils.HasFinalizer(cd, finalizer) { | ||
| logger.Debug("adding finalizer to ClusterDeployment") | ||
| controllerutils.AddFinalizer(cd, finalizer) |
There was a problem hiding this comment.
We're going to have to think about how to transition a pre-existing CD managed by the awsprivatelink controller, which is going to include replacing the hive.openshift.io/aws-private-link finalizer with this one.
There was a problem hiding this comment.
We discussed having the privatelink and awsprivatelink operators be mutually exclusive, with the active one always migrating the finalizer and conditions to itself. In other words, the privatelink operator, when active, would migrate the awsprivatelink finalizer and conditions to privatelink finalizer and conditions. At the same time, when the awsprivatelink operator is active, it would migrate the privatelink finalizer and conditions back to awsprivatelink finalizer and conditions. This would enable admins to switch between the two to test and easily revert while also making sure the migration happens.
| return reconcile.Result{}, nil | ||
| } | ||
|
|
||
| // SpokePlatformValidate validates a cluster deployment in relation to the spoke platform. |
There was a problem hiding this comment.
I know these are stubs right now, but we'll eventually want to document more of the contract. For example, reading through where this one is called by the reconciler, it seems like it'll be expected to update the cd status rather than the caller being responsible for that. (Though I'm not sure that's the best contract, as it would result in some duplication in the impls...)
There was a problem hiding this comment.
With the status being platform specific, the privatelink controller wouldn't know what information to add. At a minimum, the actuators would have to provide the data. We could have them return a status and have the controller apply that.
There was a problem hiding this comment.
Cool cool, whatever shakes out cleanest; but whatever that is, let's make sure this comment spells it out. I don't want future-us to have to crack open impls and read code to find out which objects get updated therein.
|
@jstuever: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| if err != nil { | ||
| return reconcileResult, err | ||
| } | ||
|
|
||
| if !reconcileResult.IsZero() { | ||
| return reconcileResult, nil | ||
| } |
There was a problem hiding this comment.
or:
| if err != nil { | |
| return reconcileResult, err | |
| } | |
| if !reconcileResult.IsZero() { | |
| return reconcileResult, nil | |
| } | |
| if err != nil || !reconcileResult.IsZero() { | |
| return reconcileResult, err | |
| } |
| // Incorporate the AWSPrivateLink configmap hash | ||
| confighash = computeHash("", confighash, awsPlConfigHash) | ||
|
|
||
| // Incorporate the PrivateLink configmap hash | ||
| confighash = computeHash("", confighash, plConfigHash) |
There was a problem hiding this comment.
the hashes arg is variadic, so we can combine these:
| // Incorporate the AWSPrivateLink configmap hash | |
| confighash = computeHash("", confighash, awsPlConfigHash) | |
| // Incorporate the PrivateLink configmap hash | |
| confighash = computeHash("", confighash, plConfigHash) | |
| // Incorporate the [AWS]PrivateLink configmap hashes | |
| confighash = computeHash("", confighash, awsPlConfigHash, plConfigHash) |
That said, we could check which controller is active by inspecting the controllersConfig and only process (above) and incorporate the hash (here) for the active one. The benefit being that we don't reroll all the hive pods if the inactive one changes. I don't know if we want to try to be that clever though :)
| @@ -142,6 +142,7 @@ func (r *ReconcileHiveConfig) deployHiveAdmission(hLog log.FieldLogger, h resour | |||
|
|
|||
| addConfigVolume(&hiveAdmDeployment.Spec.Template.Spec, managedDomainsConfigMapInfo, hiveAdmContainer) | |||
| addConfigVolume(&hiveAdmDeployment.Spec.Template.Spec, awsPrivateLinkConfigMapInfo, hiveAdmContainer) | |||
| addConfigVolume(&hiveAdmDeployment.Spec.Template.Spec, privateLinkConfigMapInfo, hiveAdmContainer) | |||
There was a problem hiding this comment.
TODO: Adapt webhook validation to consume from the new configmap, when applicable. (And of course ultimately to do whatever necessary validation on the GCP incarnation of the config when it exists.)
And again, we could decide to only add the configmap for the active controller here.
Add a new privatelink controller to eventually replace the awsprivatelink controller with a more modular approach. This will enable private link functionality to be extended to other platforms as well as between different platforms.