Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support observing child resources #209

Closed
Tracked by #213
negz opened this issue Feb 27, 2024 · 24 comments · Fixed by #217
Closed
Tracked by #213

Support observing child resources #209

negz opened this issue Feb 27, 2024 · 24 comments · Fixed by #217
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@negz
Copy link
Member

negz commented Feb 27, 2024

What problem are you facing?

It's common for folks to use this provider to create Kubernetes resources like Deployments and CronJobs. They do this by wrapping the Deployment (for example) in an Object MR.

Often these resources produce child resources. For example a CronJob will produce Jobs, and a Deployment will produce ReplicaSets and Pods. Today there's no way to observe the status of these children.

Consider a composition function that composes a CronJob, and wants expose the status of the resulting Jobs. Given that functions:

  • Must not access the API server directly
  • Often won't be running in the same cluster as the one where provider-kubernetes created the CronJob

There's no obvious way today to load up Jobs etc. You could compose observe-only Objects, but how would you know how many Objects to create and what Job names they should observe?

How could Crossplane help solve your problem?

One approach could be to add an ObjectCollection MR. Such an MR would be:

  • Inherently observe-only. It would not support creating, updating, or deleting objects
  • Would accept a GVK and a label selector, and expose all matching resources in its status

A special case version would be EventCollection, for loading events. This would be similar to an ObjectCollection but it would match on the resource the events pertained to, rather than labels.

@negz negz added the enhancement New feature or request label Feb 27, 2024
@turkenh
Copy link
Collaborator

turkenh commented Feb 27, 2024

As an initial impression this issue feels like a variant of crossplane/crossplane#4141 but for provider-kubernetes only.

I am fine with making an exception for provider kubernetes since it is a bit special as we can observe all resources with a single type different than other providers but still worth thinking whether we could solve the general class of problems instead.

@gberche-orange
Copy link

This issue also seems similar to MR referencing other resources as described into https://docs.crossplane.io/v1.15/concepts/managed-resources/#referencing-other-resources

Currently, it seems to me that the k8s provider supports the pattern Matching by external name

It would be useful to explore whether the use-case of observing child resources could be supported by supporting a variant of Matching by selector standard MR pattern:

  • whereas the Matching by selector standard pattern matches crossplane MR by labels, this variant for the provider-kubernetes would be matching resources in the target k8s api by labels (in addition to the existing Matching by external name fields aka api version/kind/namepace)

For the specific case of Jobs being created as a result of a Cronjob, this would mean looking up these objects from labels. As there would typically more than one matching resources, the current syntax for destination of looked field such as toField, would likely need to support list, such as iterators, for foreach gotemplate syntaxes

/CC @poblin-orange @l-croq @blezoray

@negz
Copy link
Member Author

negz commented Mar 5, 2024

@gberche-orange It's unclear to me whether you're suggesting label selectors be used to implement an ObjectCollection MR that represented several resources, or whether you think there's some way to use label selectors to create an (observe-only) Object MR per matched resource.

@gberche-orange
Copy link

@negz Sorry I was unclear, I'm suggesting to leverage the existing kubernetes provider Object MR and extend it to support a new selector field in relevant existing constructs above that currently only use reference by id pattern.

@negz
Copy link
Member Author

negz commented Mar 5, 2024

I'm suggesting to leverage the existing kubernetes provider Object MR and extend it to support a new selector field in relevant existing constructs above that currently only use reference by id pattern.

How would this work?

Let's say for example there's a CronJob that produces an arbitrary number of Jobs, each with predictable labels, say backup: nightly. A provider-kubernetes Object could match and adopt at most one Job by labels, right? So:

  • What would be responsible for creating an Object for each Job?
  • How would each Object know to only match (by label) a Job that wasn't already matched by another Object?

@gberche-orange
Copy link

Once the matching resources would be fetched, a transformation construct would be used to convert into the target expected format (usually a string, but also potentially a struct ?)

One idea for supporting the transformation construct is an optional jmespath expressions (eg something like [0].status.conditions[0] for extracting the first cronjon status)

https://jmespath.org/
https://jmespath.org/tutorial.html

@gberche-orange
Copy link

How would this work?

double reading the issue and the intent of observing the status of child objects, I realize my suggestion is likely off-topic and would rather relate to the depends-on, patchesFrom and connectionDetails and may be not specifically suited to child resource status

@negz
Copy link
Member Author

negz commented Mar 6, 2024

Once the matching resources would be fetched,

Fetched by what?

@pedjak
Copy link
Contributor

pedjak commented Mar 11, 2024

@negz how about that we call that new resource ObservingObjectCollection to emphasize that it does not create any objects? If I have understood the conversation so far, the API might look like this:

apiVersion: kubernetes.crossplane.io/v1alpha1
kind: ObservingObjectCollection
spec:
  apiVersion: batch/v1
  kind: Job
  selector:
    matchLabels:
        foo: bar

@negz
Copy link
Member Author

negz commented Mar 11, 2024

@pedjak Sounds good to me. I'd suggest ObservedObjectCollection though. This matches e.g. managed resource (as opposed to managing resource).

FWIW I agree with @turkenh that this feels like a variant of crossplane/crossplane#4141. In a perfect world we'd figure out a solution to #4141 that works for all providers, including this one.

That said, I suspect we'll be able to solve the problem at hand faster if we focus on just this provider. Perhaps the best way forward is to solve for this provider now, and use what we learn to inform a general design later. I think we did something similar with management policies.

@pedjak
Copy link
Contributor

pedjak commented Mar 12, 2024

@sttts is there any k8s/etcd limits that could impact the proposed solution, if we for example list under the status 100s of Objects?

@sttts
Copy link

sttts commented Mar 21, 2024

Like embedding a number of whole objects into status? Don't do that. Object size is limited. And every change of one object in the target cluster will lead to an update of a big object in the Crossplane cluster. Doesn't sound like a healthy solution.

@bobh66
Copy link
Contributor

bobh66 commented Mar 21, 2024

How about if the ObservedObjectCollection resource generates a set of Observe-only Objects for the desired set of objects? That way we reuse existing functionality and bundle it at a higher level into a collection. The ObservedObjectCollection could contain selector or reference to determine the content of the Collection. It could also calculate the resource tree using ownerReferences from a given starting point and create Observe-only Objects for all of the resources that it finds. All of the generated Objects can be owned by the ObservedObjectCollectionso that deleting the collection deletes the Objects.

@sttts
Copy link

sttts commented Mar 22, 2024

@bobh66 ‘s idea sounds a lot more ideomatic.

@negz
Copy link
Member Author

negz commented Mar 24, 2024

Agreed, I prefer @bobh66's proposal.

@bobh66 Are you suggesting that ObservedObjectCollection could accept e.g. a Deployment to observe and automatically figure out the child resources to create OO Objects for using owner refs? I had a similar thought, but it might be tricky:

  • What about multiple layers of children (e.g. Deployment -> ReplicaSets -> Pods). Do we include them all in the collection?
  • Given that object refs are only from child to parent, how do we know what type of children to look for? Do we hardcode it?

Maybe we start with the API @pedjak proposed at #209 (comment). We can expand it later if folks find simple label matching to not be powerful enough.

@negz
Copy link
Member Author

negz commented Mar 24, 2024

Consider a composition function that composes a CronJob, and wants expose the status of the resulting Jobs.

An ObservedObjectCollection creating a set of distinct Object resources will make the UX for doing this a little more complicated. A function will need to:

  1. Compose an Object to create (e.g.) a CronJob.
  2. Compose an ObservedObjectCollection to create a set of Objects representing the Jobs.
  3. Use the new extra resources feature to ask Crossplane to pass it said Objects.

If the ObservedObjectCollection included the objects inline (e.g. in status) the function could skip step 3.

I still prefer creating distinct Object resources, but I want to point out this tradeoff.

Another note - provider-kubernetes will need RBAC access to create Objects. I don't think we grant providers RBAC access to create their own MRs today. There is a facility for providers to ask for extra permissions beyond what Crossplane grants by default, though.

@pedjak
Copy link
Contributor

pedjak commented Mar 25, 2024

I am going to try to summarize the above discussion, @negz @bobh66 @turkenh please let me know if I missed anything.

A user declares an ObservedObjectCollection instance:

apiVersion: kubernetes.crossplane.io/v1alpha1
kind: ObservedObjectCollection
metadata:
  name: jobs
spec:
  apiVersion: batch/v1
  kind: Job
  # optional namespace
  namespace: my-ns
  selector:
    matchLabels:
        foo: bar
  providerConfigRef:
     name: kubernetes-provider

The provider lists periodically objects matching the selector and creates "observe-only" Objects in the local cluster:

apiVersion: kubernetes.crossplane.io/v1alpha2
kind: Object
metadata:
  name: 00647af6-a82f-4344-98b3-4e5dcbbbfa8d
spec:
  managementPolicies: ["Observe"]
  forProvider:
    manifest:
      apiVersion: batch/v1
      kind: Job
      metadata:
        name: job1
        namespace: my-ns
  providerConfigRef:
    name: kubernetes-provider
apiVersion: kubernetes.crossplane.io/v1alpha2
kind: Object
metadata:
  name: ae2d702b-e468-41fa-b427-f96ee03928aa
spec:
  managementPolicies: ["Observe"]
  forProvider:
    manifest:
      apiVersion: batch/v1
      kind: Job
      metadata:
        name: job2
        namespace: my-ns
  providerConfigRef:
    name: kubernetes-provider

Objects are owned by ObservedObjectCollection and referred in its status:

apiVersion: kubernetes.crossplane.io/v1alpha1
kind: ObservedObjectCollection
metadata:
  name: jobs
spec:
  apiVersion: batch/v1
  kind: Job
  # optional namespace
  namespace: my-ns
  selector:
    matchLabels:
        foo: bar
  providerConfigRef:
     name: kubernetes-provider
status:
  objects:
    - name: ae2d702b-e468-41fa-b427-f96ee03928aa
    - name: 00647af6-a82f-4344-98b3-4e5dcbbbfa8d
flowchart LR;
collection["ObservedObjectCollection"]
provider{{provider-kubernetes}}
cluster((k8s cluster))
object1["Object

managementPolicies: Observe"]
object2["Object

managementPolicies: Observe"]
object3["Object

managementPolicies: Observe"]

provider-->|reconciles|collection
provider-->|list objects matching selector|cluster
provider-->|creates|object1
provider-->|creates|object2
provider-->|creates|object3
collection-->|refers|object1
collection-->|refers|object2
collection-->|refers|object3
object1-->|owned by|collection
object2-->|owned by|collection
object3-->|owned by|collection

Once ObservedObjectCollection is declared in a composition, its items could be consumed by composition function pipeline using extra resource feature.

@negz
Copy link
Member Author

negz commented Mar 25, 2024

@pedjak That sounds right to me, thank you.

Out of curiosity, are the schemas for those OO Objects accurate? I thought OO objects mostly populated their status, not their spec.

@negz
Copy link
Member Author

negz commented Mar 25, 2024

@pedjak I saw there was a Slack thread with @phisco around how we'd potentially use https://github.com/crossplane-contrib/function-extra-resources to load the produced objects into the composition.

So I think this makes the overall flow, for example.

  1. XR creates CronJob Object, and Job ObjectCollection.
  2. provider-kubernetes creates CronJob
  3. provider-kubernetes creates an (OO) Object for every Job it sees
  4. XR uses function-extra-resources to load the objects from step 3 as extra resources

I saw you were asking about creating the list of extra resources dynamically, but I didn't quite follow what you meant.

Would it be possible to propagate labels from ObservedObjectCollection to the OO Objects they create? If we did that, I think you could use function-extra-resources to load all Objects with labels that matched the ObservedObjectCollection's.

@turkenh
Copy link
Collaborator

turkenh commented Mar 26, 2024

I still prefer creating distinct Object resources, but I want to point out this tradeoff.

Agreed. I am not happy with the UX, but I agree that the proposed approach is cleaner.

Realizing that we landed on "Option A" in the Future Work section of the observe-only design doc, which is a good sign that the solution could be extended to other providers as well.

@pedjak
Copy link
Contributor

pedjak commented Mar 26, 2024

Out of curiosity, are the schemas for those OO Objects accurate? I thought OO objects mostly populated their status, not their spec.

@negz correct - under the status comes the full observed object. I have omitted status part because that is going to be populated by object controller. The new controller creates the above payload and let the existing controller reconciles it.

Would it be possible to propagate labels from ObservedObjectCollection to the OO Objects they create?

It would, but I would pause on it for now, given the discussion we have here. Also, if users do not use distinct labels across multiple ObservedObjectCollection instances, there is a risk to add more objects into composition pipelines.

@phisco suggested that go templating function could be extended for extra resources usecase. If I have understood correctly, the template might look like:

apiVersion: kubernetes.crossplane.io/v1alpha2
kind: Object
metadata:
  name: {{ (index .desired.composed "observed-object-collection").resource.status.objects[0].name }}
  annotations:
     gotemplating.fn.crossplane.io/extra-resource: true

@phisco
Copy link
Collaborator

phisco commented Mar 26, 2024

See here for my suggestion about adding support for extra resources to the go templating API: crossplane-contrib/function-go-templating#77

@negz
Copy link
Member Author

negz commented Mar 26, 2024

It would, but I would pause on it for now, given crossplane/crossplane-runtime#633 (comment).

I don't follow how that discussion affects this decision.

Using labels to match collections of objects is an idiomatic Kubernetes practice, isn't it?

Is the concern about implicitly propagating labels from parent -> child in general, or is it that you feel propagating labels should block on having a solution like status.observedLabels?

If the concern is about implicitly propagating labels from parent -> child, we could take an approach more like Deployment and CronJob, where the labels the child Object should have are specified explicitly:

apiVersion: kubernetes.crossplane.io/v1alpha1
kind: ObservedObjectCollection
metadata:
  name: jobs
  labels:
    foo: bar
spec:
  apiVersion: batch/v1
  kind: Job
  namespace: my-ns
  selector:
    matchLabels:
        foo: bar
  providerConfigRef:
     name: kubernetes-provider
  # Explicitly add label foo: bar to all child Objects.
  # I'm using the 'template' pattern from Deployment/CronJob but don't feel strongly about the API shape.
  template:
    metadata:
      labels:
        foo: bar

Also, if users do not use distinct labels across multiple ObservedObjectCollection instances, there is a risk to add more objects into composition pipelines.

This is a concern with most all uses of labels in Kubernetes, but they're still the idiomatic way to group and select related sets of objects. For example if you wanted to select all Jobs that were part of a CronJob (directly, without provider-kubernetes in the middle) you'd probably use labels, right?

Quoting https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#label-selector-and-annotation-conventions:

For disciplined users managing resources within their own namespaces, it's not that hard to consistently apply schemas that ensure uniqueness. One just needs to ensure that at least one value of some label key in common differs compared to all other comparable resources. We could/should provide a verification tool to check that. However, development of conventions similar to the examples in Labels make uniqueness straightforward.

My two cents: using labels feels like an easier to understand and work with UX than needing to use Go templates (or similar) to parse the status of the ObservedObjectCollection.

@pedjak
Copy link
Contributor

pedjak commented Mar 26, 2024

Is the concern about implicitly propagating labels from parent -> child in general,

That was my concern, adding them explicitly through the template looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants