✨ Initial support for Extension #598
Conversation
|
Looking for feedback on the initial definition of |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #598 +/- ##
===========================================
- Coverage 84.53% 74.35% -10.18%
===========================================
Files 20 24 +4
Lines 931 1205 +274
===========================================
+ Hits 787 896 +109
- Misses 100 260 +160
- Partials 44 49 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
api/v1alpha1/extension_types.go
Outdated
| // Pause reconciliation on this Extension | ||
| Paused *bool `json:"paused,omitempty"` |
There was a problem hiding this comment.
It is somewhat frowned upon to use bools in kubernetes APIs because there are many cases where a 3rd (or more) state creeps in.
Should we try to make this an enum-style string field with a oneOf validation?
api/v1alpha1/extension_types.go
Outdated
| //+kubebuilder:validation:MaxLength:=64 | ||
| //+kubebuilder:validation:Pattern:=^[a-z0-9]+([\.-][a-z0-9]+)*$ | ||
| // +optional | ||
| // Location of installation TBD?? | ||
| DefaultNamespace string `json:"defaultNamespace,omitempty"` |
There was a problem hiding this comment.
Since extension is namespace-scoped, I wonder if we should start without this field in the API and just always use the install namespace as the "default namespace" to begin with.
There was a problem hiding this comment.
+1 to using the install namespace as the default namespace. I'm not sure if there has been more discussion on implementing it yet, but maybe this can be repurposed as WatchNamespaces?
There was a problem hiding this comment.
The App CR has defaultNamespace. I was thinking it would be helpful here, but I'm totally fine waiting to add that later.
There was a problem hiding this comment.
I've definitely seen requirements about the operator in the namespace having different permissions than what is required to apply the operator to the cluster, so it seems useful to be able to manage the operator installation without a service account that can apply rbac in that namespace.
There was a problem hiding this comment.
Maybe I'm missing the context of this, but if we kept this field wouldn't that mean that if it's set to something other than the namespace the extension cr is applied to, we will template the namespace of manifests in the bundle to whatever this field is set to? I'm saying I see a use case for that.
api/v1alpha1/extension_types.go
Outdated
| //+kubebuilder:validation:MaxLength:=64 | ||
| //+kubebuilder:validation:Pattern:=^[a-z0-9]+([\.-][a-z0-9]+)*$ | ||
| // ServiceAccount name used to install this extension | ||
| ServiceAccountName string `json:"serviceAccountName,omitempty"` |
There was a problem hiding this comment.
Based on #595 it seems like this would be a required field
api/v1alpha1/extension_types.go
Outdated
| DefaultNamespace string `json:"defaultNamespace,omitempty"` | ||
|
|
||
| // Defintion of package to be installed | ||
| Package ExtensionPackage `json:"package,omitempty"` |
There was a problem hiding this comment.
Looks like #596 was updated to prefer a "source" pattern. So something like:
| Package ExtensionPackage `json:"package,omitempty"` | |
| Source ExtensionSource `json:"source,omitempty"` |
I imagine this would need to be a required field or have some validation to check that the source is "one of" X,Y,Z
There was a problem hiding this comment.
Source is required. Only 1 of its field may be specified, and is required. We can do that with CEL validation.
There was a problem hiding this comment.
We may want to consider implementing the Source as a discriminated union. See:
api/v1alpha1/extension_types.go
Outdated
| // Pause resolution of this Extension | ||
| ExtensionPausedTrue ExtensionPaused = "True" | ||
| // Peform resolution of this Extension | ||
| ExtensionPausedFalse ExtensionPaused = "False" |
There was a problem hiding this comment.
Sorry, should have been more specific earlier. What about something with:
PausedandActiveenum valuesActivewould be default.
In the future, this would give us space to easily introduce another meaningful enum value.
There was a problem hiding this comment.
The spec named the field paused. Having the field and a value be the same makes little sense. Do you have a suggestion for new name?
There was a problem hiding this comment.
I really like paused true/false and I'm not sure I'm convinced that in the future we'd want to expand paused with more values... but some brainstorming if we want to:
managed: active/paused
management.state: active/paused
reconcile: active/paused
reconciliation: active/paused
There was a problem hiding this comment.
I wrote this as resolution rather than reconciliation, so thinking managed might be more appropriate.
There was a problem hiding this comment.
I don't have a really strong opinion here. What I want to avoid is a situation where there is some other knob that we decide we need to add later that isn't completely orthogonal to spec.paused. I can't think of anything off hand, though. This does seem fairly cut and dry.
Not exactly the same, but the OLMv0 CatalogSource API has some fields that become inert based on the value of some other fields. Perhaps the main issue there is that we didn't add validation at the time that would prevent setting a field if something else made it inert.
|
Ignoring the build failures (which will be fixed) is this looking like something we want to proceed with? |
tmshort
force-pushed
the
extension
branch
3 times, most recently
from
ac9ad41 to
e722512
Compare
There was a problem hiding this comment.
What's the thought on order of operations for fully switching over to Carvel? With this PR, we would have ClusterExtensions and Extensions both managing BundleDeployments.
And @varshaprasad96 is working on replacing BD with Carvel App.
Not sure what you all are thinking, but my hunch is that coding and review would be fastest with the following order:
- Add Extension API with no controller
- Varsha adds Extension controller with App support (essentially pretending like ClusterExtension API and controller do not exist).
- We remove ClusterExtension API and controller.
I'm worried that we'll spend unnecessary time writing code, updating tests, reviewing, and discussing/handling edge cases if we approach this as keeping both or implementing Extension+BundleDeployment.
@joelanford |
|
IMO, all of the changes that are made in this PR that make it possible to reuse code for both ClusterExtensions and Extensions is essentially adding an extra layer of abstraction that we won't want or need once we remove the ClusterExtension API. Even just the time that folks will spend reviewing seems pretty steep (>5000 LoC change in this PR right now). If we're going to have a Extension+BundleDeployment controller in place in this transition, it seems like it might be easier if we order things somewhat like this:
Any maybe 2 and 3 can be reversed? |
|
Adding some thoughts:
How long do we want to support Rukpak and Kapp controller side-by-side? Based on the previous discussions, as I understand we want kapp-ctrl to be under feature gate, and still support Rukpak before making a complete swap. If that is going to be the case, the initial sequence of steps seem more apt - #598 (review), to avoid supporting an intermediate Extension+BD at all, which users can find confusing - a namespace scoped API supporting 2 appliers underneath, that support different scoping. Though there would be some rework, it would keep things clearer in terms of a. ClusterExtension+Rukpak If there is going to be a complete immediate swap without needing to support both Rukpak and Kapp, we could do #598 (comment). Would prefer supporting Carvel first with bare minimal specs in Extension API, so that we can break down and follow up with requirements to complete (3) in parallel. |
Copies ClustersExtension functionality `ServiceAccountName` doesn't do anything yet. Signed-off-by: Todd Short <[email protected]>
cmd/manager/main.go
Outdated
| if features.OperatorControllerFeatureGate.Enabled(features.EnableExtensionApi) { | ||
| if err = (&controllers.ExtensionReconciler{ | ||
| Client: cl, | ||
| BundleProvider: catalogClient, | ||
| Scheme: mgr.GetScheme(), | ||
| Resolver: resolver, | ||
| }).SetupWithManager(mgr); err != nil { | ||
| setupLog.Error(err, "unable to create controller", "controller", "Extension") | ||
| os.Exit(1) | ||
| } | ||
| } |
There was a problem hiding this comment.
One of the implications with not even adding a reconciler for the Extension API to the controller manager is that if this feature gate is disabled, creating an Extension resource will silently do nothing. From a UX perspective I think it would make sense to still add the reconciler but when the feature gate is disabled we simply add a status condition that states something along the lines of "Reconciliation skipped. feature gate for the Extension API is disabled. The Extension API is an alpha feature that is still in development. To enable reconciliation use the flag --feature-gates=EnableExtensionApi=true".
Doing this would at least signal to an end user that it is currently in a "do nothing" state and how to enable it.
There was a problem hiding this comment.
That being said, I'm also not sure of any API versioning constraints that come with adding a status condition for this. Definitely something to think about regarding the pros/cons
There was a problem hiding this comment.
Yeah, I'm a bit torn. on where to put the feature gate.
There was a problem hiding this comment.
The ideal scenario would be to not have the Extension CRD applied if the feature gate is disabled - but I don't think that's something which is possible here, since the CRDs are applied before the controller pod is created. Instead of setting up the reconciler by default and adding the overhead of watching Extension resources, what do you all think about adding a log/warning during startup, mentioning the value of the feature gate flag, and explicitly stating something on the lines of - "feature gate flag for ExtensionAPI set to false, no action will be taken by operator controller if Extension object is created". That should to some extent satisfy the UI difficulties without having us to take more effort on it for now and at the same time not be a surprise for the user that nothing is happening when Extension is created?
There was a problem hiding this comment.
@varshaprasad96 I think that assumes a user who might create the Extension resource would have an understanding to go and look at the logs of the operator-controller pod and explicitly find that singular log message.
Throwing a message on the status is more immediately visible IMO and to me doesn't feel like a bunch of overhead. I'm mainly approaching this from what I would want as a user and I would much prefer seeing a status be set than having to find the pod that is running the controller manager, get the logs, and search the logs for any indication as to why nothing happened
There was a problem hiding this comment.
@everettraven has a point, if your extension isn't running, you're going to look at your extension first, and not necessarily a pod to which you don't have log access to.
There was a problem hiding this comment.
Some thoughts on why this suggestion:
I think that assumes a user who might create the Extension resource would have an understanding to go and look at the logs of the operator-controller pod and explicitly find that singular log message.
I would expect that users who create Extension resources have the understanding of troubleshooting at this stage, which includes checking logs when something doesn't work as expected - this is particularly considering the fact that afaik there is no one trying to use v1 in production in near future, at least till we make kapp-ctrl switch -scheduled soon.
That being said, I understand and completely agree that having just a log line - that too in an o-c pod that the user trying to install the package, ideally should not concern with - is not the perfect UX. This is to avoid - both the overhead of having an additional reconciler watch extension and other relevant resources on cluster which is not necessary, and us spending time on an API field that could turn out to be temporary in very near future. On a quick search upstream found an example of one of the optional controllers enabled only when the feature gate is present.
Signed-off-by: Todd Short <[email protected]>
| //nolint:unparam | ||
| func (r *ExtensionReconciler) reconcile(_ context.Context, ext *ocv1alpha1.Extension) (ctrl.Result, error) { | ||
| // Don't do anything if Paused | ||
| if ext.Spec.Managed == ocv1alpha1.ManagedStatePaused { |
There was a problem hiding this comment.
Can we add a log here stating no reconciliation done because the managedState is set to paused?
There was a problem hiding this comment.
For follow-up, check in other popular Kubernetes projects and see if there is a pattern where a Paused condition is set.
| // Generate reconcile requests for all extensions affected by a catalog change | ||
| func extensionRequestsForCatalog(c client.Reader, logger logr.Logger) handler.MapFunc { | ||
| return func(ctx context.Context, _ client.Object) []reconcile.Request { | ||
| // no way of associating an extension to a catalog so create reconcile requests for everything |
There was a problem hiding this comment.
Given we are not mapping catalog and extension, and creating reconcile requests for all extensions, I don't think we need this mapping func. Watching all catalogs should be enough (in the current state).
There was a problem hiding this comment.
I believe this is still necessary as the comment says, there's no way to indicate ownership due to the namespace scope of Extension. I can look, but this is how ClusterExtension does it.
There was a problem hiding this comment.
I think I figured out the correct method.
| labels: | ||
| app.kubernetes.io/name: clusterrole | ||
| app.kubernetes.io/instance: extension-viewer-role | ||
| app.kubernetes.io/component: rbac | ||
| app.kubernetes.io/created-by: operator-controller | ||
| app.kubernetes.io/part-of: operator-controller | ||
| app.kubernetes.io/managed-by: kustomize |
There was a problem hiding this comment.
Nit: drop all these labels (here and in the editor role). These are setup by SDK/Kubebuilder, right? I don't think they provide any real value.
There was a problem hiding this comment.
If I remove them, they might be re-added?
There was a problem hiding this comment.
I'm guessing this is part of operator-sdk create api? So no they would not be re-added.
There was a problem hiding this comment.
Not sure, but I'll try; there definitely a build process that regenerates some of these types of things.
| - apiGroups: | ||
| - olm.operatorframework.io | ||
| resources: | ||
| - extensions/status | ||
| verbs: | ||
| - get |
There was a problem hiding this comment.
Nit: Is get a valid verb on the CRD status subresource? I'm fairly sure it isn't and this rule accomplishes nothing. (Is this another SDK/Kubebuilder-ism?)
There was a problem hiding this comment.
Yes, I believe kubebuilder did this, and might put it back...
There was a problem hiding this comment.
Why would kubebuilder put on an invalid verb for a resource such as this?
There was a problem hiding this comment.
client.Status().Get() does not seem to exist... so perhaps you can't get it.
There was a problem hiding this comment.
I'll dig in kubebuilder code. But I bet just an uninformed addition to the generation that seemed like a good idea.
There was a problem hiding this comment.
Wow, it has been there for a very long time (5 years). I suspect it was overlooked originally and is otherwise completely inert so no one has ever noticed.
| - apiGroups: | ||
| - olm.operatorframework.io | ||
| resources: | ||
| - extensions/status | ||
| verbs: | ||
| - get |
There was a problem hiding this comment.
Same question as above about get on the status subresource.
| resources: | ||
| - extensions/status | ||
| verbs: | ||
| - get |
| labels: | ||
| app.kubernetes.io/name: extension | ||
| app.kubernetes.io/instance: extension-sample | ||
| app.kubernetes.io/part-of: operator-controller | ||
| app.kubernetes.io/managed-by: kustomize | ||
| app.kubernetes.io/created-by: operator-controller |
There was a problem hiding this comment.
Do these actually get included in the kustomize output? It looks like not because they are not listed in the kustomization.yaml file (neither are the ClusterExtension roles).
There was a problem hiding this comment.
No idea, but they were generated... we can remove them later if deemed necessary?
There was a problem hiding this comment.
I guess I'd suggest adding them to the kustomization file?
(It looks like I have a few issues to open in Kubebuilder 😅 )
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Ideally, we would not need reconcile-time validators at all because we instead have validation done via CRD schema validation.
I won't push to remove this validation code now, but I think we should capture a follow-up that removes it and then beefs up tests of the CRD validation (if they aren't already beefy) to make sure our CRD schema validation functions catch everything we expect. If anything slips through, it likely fails anyway somewhere else during reconciliation.
| } | ||
| require.NoError(t, cl.Create(ctx, namespace)) | ||
|
|
||
| t.Log("By injecting creating a client with the bad clusterextension CR") |
There was a problem hiding this comment.
| t.Log("By injecting creating a client with the bad clusterextension CR") | |
| t.Log("By injecting creating a client with the bad extension CR") |
| Source: ocv1alpha1.ExtensionSource{ | ||
| Package: &ocv1alpha1.ExtensionSourcePackage{ | ||
| Name: pkgName, | ||
| Version: "1.2.3-123abc_def", // bad semver that matches the regex on the CR validation |
There was a problem hiding this comment.
This passes our CRD schema validation, but does not parse with the masterminds parser? If that's the case, how hard would it be to update the CRD pattern to disallow this?
There was a problem hiding this comment.
This was from cluster extension (originally operator), we might have to look at the original PR to understand why... my recollection was complexity of the regex, maybe?
There was a problem hiding this comment.
Looking further into the test, it purposefully bypasses CR validation, and exercises the validation code.
There was a problem hiding this comment.
Seems... kinda useless if we already have CRD validations and tests for them. But happy to revisit in a follow-up.
| p.Channel = r.Spec.Channel | ||
| p.Name = r.Spec.PackageName | ||
| p.Version = r.Spec.Version | ||
|
|
There was a problem hiding this comment.
Add UpgradeConstraintPolicy here too?
There was a problem hiding this comment.
Not needed yet, we have no processing for it in Extension
There was a problem hiding this comment.
I was thinking just in terms of symmetry with what you'd get from Extension.GetPackageSpec().
There was a problem hiding this comment.
Should we consider removing GetPackageSpec and ExtensionInterface in general? It looks like all the functions right now receive concrete types. Someday, and that day may never come, we will add an interface. But until that day, we should be fine without it.
| func (r *ClusterExtension) GetUpgradeConstraintPolicy() UpgradeConstraintPolicy { | ||
| return r.Spec.UpgradeConstraintPolicy | ||
| } |
There was a problem hiding this comment.
Drop this since UpgradeConstraintPolicy is now part of the package struct?
There was a problem hiding this comment.
The interface function is not being used, so I'm removing it
Signed-off-by: Todd Short <[email protected]>
There was a problem hiding this comment.
I left a few questions and suggestions.
Ask for the next time - can we please split large PRs into smaller ones? For this PR split could look something like this:
- Refactor existing controllers to extract generic parts
- Add a new
Extensiontype & generate manifests for it - Introduce a new controller for
Extension
It is easier to review smaller PRs.
P.S: I'm guilty of making large PRs myself, but I'm trying :)
| func (r *ClusterExtension) GetUID() types.UID { | ||
| return r.ObjectMeta.GetUID() | ||
| } |
There was a problem hiding this comment.
Do we need to do that? ObjectMeta is embedded and I believe GetUID() should be available without overriding. Or am I missing something?
| type ExtensionInterface interface { | ||
| GetPackageSpec() *ocv1alpha1.ExtensionSourcePackage | ||
| GetUID() types.UID | ||
| } | ||
|
|
||
| func ExtensionArrayToInterface(in []ocv1alpha1.Extension) []ExtensionInterface { | ||
| ei := make([]ExtensionInterface, len(in)) | ||
| for i := range in { | ||
| ei[i] = &in[i] | ||
| } | ||
| return ei | ||
| } | ||
|
|
||
| func ClusterExtensionArrayToInterface(in []ocv1alpha1.ClusterExtension) []ExtensionInterface { | ||
| ei := make([]ExtensionInterface, len(in)) | ||
| for i := range in { | ||
| ei[i] = &in[i] | ||
| } | ||
| return ei | ||
| } |
There was a problem hiding this comment.
Where is interface and these conversion functions are being used? Is it a leftover from before scope reduction? If so - should we get rid of this for now?
| p.Channel = r.Spec.Channel | ||
| p.Name = r.Spec.PackageName | ||
| p.Version = r.Spec.Version | ||
|
|
There was a problem hiding this comment.
Should we consider removing GetPackageSpec and ExtensionInterface in general? It looks like all the functions right now receive concrete types. Someday, and that day may never come, we will add an interface. But until that day, we should be fine without it.
|
Big +1 to @m1kola 's request to have smaller incremental PRs. I think reviews/merges would move a lot faster and will be key to unblocking others as well. |
There was a problem hiding this comment.
I think this PR is still a bit large to be able to engage in high fidelity review, and I'm feeling a bit of review fatigue with all of the moving parts.
However, I would like to unblock further work, so perhaps we could merge what we have and then follow back up with smaller PRs that cover the following:
- Focused discussion and possibly PR about
spec.managedstaying as an enum to converting back to a bool. I know I suggested the enum, but the more I think about it, the more I agree with Andy that this feature is unlikely to ever expand beyond two values. - Some cleanup from kubebuilder-generated stuff. I submitted some issues in upstream kubebuilder about it
- Remove reconcile-time validations and tests. We should be able to have robust testing for our CRD validations that happen at admission time
- Remove unused fields from extension reconciler (e.g. BundleProvider, Scheme, etc.). We can add them when we need them.
- Remove ExtensionInterface and associated functions. Current plan is to remove ClusterExtension, so no need to introduce an abstraction that will very shortly obsolete when we do remove ClusterExtension. If plans change and ClusterExtension sticks around, we can re-assess the need for an abstraction.
- Mirror the paused/active spec value to the status.
- Make the
ExtensionSourcestruct use the union discriminator pattern, and add associated CRD schema validation. - Refactor tests to be more table-driven.
|
@joelanford are we good to merge, with issues created for those items listed? |
There was a problem hiding this comment.
@tmshort Just a small update comment on samples based on recent changes made in the PR.
| paused: false | ||
| serviceAccountName: extension-sa | ||
| defaultNamespace: extension-sample | ||
| package: |
There was a problem hiding this comment.
Was working based of this PR, realized that this sample need to be updated according to the recent changes made in the spec.
There was a problem hiding this comment.
Basically this:
apiVersion: olm.operatorframework.io/v1alpha1
kind: Extension
metadata:
labels:
app.kubernetes.io/name: extension
app.kubernetes.io/instance: extension-sample
app.kubernetes.io/part-of: operator-controller
app.kubernetes.io/managed-by: kustomize
app.kubernetes.io/created-by: operator-controller
name: extension-sample
namespace: extension-sample
spec:
paused: false
serviceAccountName: extension-sa
source:
package:
name: argcocd-operator
version: <>There was a problem hiding this comment.
Are you talking about the labels, or defaultNamespace?
There was a problem hiding this comment.
Not the labels, just the spec. Removal of default namespace, embedding package inside source and also renaming paused to managed.
I was trying to use this sample to test extensions with App CR, which is when realized that some of the fields are not upto date with the what is present in CRD.
|
You have the all clear from me to merge. We can also capture Varsha's sample update in a follow up as well as far as I'm concerned. |
|
I've captured the follow-ups in the epic issue. Adding this one to the queue now. |
Fix #596
Based on ClusterExtension
Description
Reviewer Checklist