Skip to content

Proposal: Images Sub-specification #2158

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

Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
257 changes: 257 additions & 0 deletions doc/proposals/images-subspec.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,257 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest/recommend improve the first comment to open this PR in order to make the goal more clear. Following some ideas.

Description of the change:
Proposal for new operator-sdk future to allow users to inform images and the common secondary resources required for it be created as all logic which would be required to follow up the best practices to manage them.

Motivation for the change:
Many operators as for example x and x will at the end have CRD's which will represent a deployment/pod of
an image:tag. So, I am as oper user I'd like to have a feature to make this process easier.

Copy link
Contributor

@camilamacedo86 camilamacedo86 Nov 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS.: The goal would be in a simple way make clear what is the problem that you are trying to solve with, in order to avoid misunderstands and let it open for contributions too. The community, users and others may bring good ideas.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll go ahead and do this.

title: Images Sub-specification
authors:
- '@zachncst'
reviewers:
- TBD
approvers:
- TBD
creation-date: 2019-11-04
last-updated: 2019-11-05
status: provisional
help-from:
- '@camilamacedo86'
- '@kevinrizza'
- '@shawn-hurley'
see-also:
- 'https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/contributors/design-proposals/related-images.md'
---

# Images Sub Spec

## Release Signoff Checklist

- \[ \] Enhancement is `implementable`
- \[ \] Design details are appropriately documented from clear requirements
- \[ \] Test plan is defined
- \[ \] Graduation criteria for dev preview, tech preview, GA
- \[ \] User-facing documentation is created in
[operator-sdk/doc][operator-sdk-doc]

## Tag Line

> Who likes hard coding image locations anyways?

## Summary

When an operator is installed, the images that the operator installs as operands in the
form of pods, deployments or other kubernetes resources is not always clearly
defined and editable by a user of an operator. This proposal would define
an operator-sdk ImageSpec and ImagesListSpec object structs that operator developers
could use to enhance the creation of operator CRDs or improve existing
operators. The spec objects would let operator users override image locations
for that custom resource.

Additionally, the proposal would provide tooling to generate scaffold code for
creating new operators. When creating a new operator, an operator developer
would be able to pass flags that would create a base spec object that includes
the ImagesListSpec.

Copy link
Contributor

@camilamacedo86 camilamacedo86 Nov 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO: Would make more sense have a feature which makes the process to implement the common requirement to deploy an image and manage its deployment/pod following the good practices instead of make it be mandatory.

Following my suggestion.

  • Add an image flag for operator-sdk add api --api-version=app.example.com/v1alpha1 --kind=AppService --image=ngnix:latest

Then, when it be used the CR/CRD and API with few standard specs will be scaffold.

  • Add an flag tooperator-sdk add controller --api-version=app.example.com/v1alpha1 --kind=AppService --image-gen

Then, it would scaffold the controller with watches and a reconcile impl with the standard/basic code to have a deployment and manage its status.

Reasons:

  • Give flexibility to the users and allow them to change the code according to their needs
  • We will scaffold the code, and then it also can be improved to be integrated/used with other projects as kubebuilder for example
  • A lot of images also will require specs for its env vars. E.g. PostgreSQL requires user, database name, PWD. If we keep it fixed, will it be really useful?

OutOfScope: it could be improved to accept more flags or use a config.yaml file, for example, to define if should or not expose a service, use env vars, should create or not a secret, etc .. For these reasons, we could just start from just a flag as POC but it cannot be fixed/mandatory and etc. It could be improved and grow a lot in the future.

c/c @joelanford

## Motivation

This proposal is driven by a few motivations:

- Makes operators more transparent with what is being installed on the cluster.
- Formalizes a best practice of operator development.
- The depencency between operator version and operator resource version is broken.
- Allows for out of band updates of the underlying images.
- Registry information for the images can easily be overridden to point at
private registries.
- Provides some (potential) standardization to the CRD.

## Goals

- Provide an easy mechanism of defining and overriding images and related image
information (like pull secrets).
- Increase transparency with what operators are installing.
- Simple, reusable specs to build operators; similar to PodSpec in Kubernetes.

Copy link
Contributor

@camilamacedo86 camilamacedo86 Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May:

Suggested change
- Provide an easy mechanism to allow the operator to generate a scaffold CR/CRD, types and controller managing all common secondary resources expected to have a deployment of an image
- Increase the quality of the operators by leading and help users do its implementation following its design and concepts.
- Reduce the effort or users develop solutions with.

### Non-Goals

- OLM management of the images spec is not in this proposal.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we scaffold the code we will provide the facility and all other features will work for this. So, I do not think that it is required.

Suggested change
- OLM management of the images spec is not in this proposal.

- Mapping/re-mapping other container values (ports, etc.).
- Additional pod information, like env vars.

## Proposal

Implementing the proposal would include:

- Add an optional section to the CRD spec named images that contains definitions of
all images used by the operator. An example:

```yaml
apiVersion: app.example.com/v1alpha1
Copy link
Contributor

@camilamacedo86 camilamacedo86 Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO:

  • We should not create a YAML file now with the images and etc.
  • We could use a flag to inform the image and scaffold a common implementation instead of that.

Reasons:

  • Give flexibility for the user do changes
  • Make transparent what the project is dong instead of having a block magic box that does things
  • We should began by doing this process for only 1 image in order to keep it simple and then, after we have it working and all improvements that will happen by following an agile process and its interactions we could see better if it would be good or not allow it to be done for N/(a list of) images by just one action.

Note that this approach will not just allow us to grow with consistency but make easier the process to implement it since if we have it working as expected for 1 image should not be a big deal improve the facility to N if we identify that it is would be a good feature.

kind: AppService
metadata:
name: example-appservice
spec:
size: 3
images:
- name: nginx
image: nginx:1.7.9
- name: example
iamge: my-registry:5000/example:1.0.0
```

Copy link
Contributor

@camilamacedo86 camilamacedo86 Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other reason for this file not be created yet is because of kubebuilder add all API, version kinds generated by in the PROJECT file already as the following example.

version: "2"
domain: my.domain
repo: test
resources:
- group: webapp
  version: v1
  kind: Guestbook
- group: webapp
  version: v1
  kind: Guestbook2

So, after the integration, we might use it to add the images as well. For example.

version: "2"
domain: my.domain
repo: test
resources:
- group: webapp
  version: v1
  kind: Guestbook
- group: webapp
  version: v1
  kind: Guestbook2
  image: image:tag 

NOTE: It is just one idea to illustrate the N possibilities that it may have to be improved, because of this, ì think we should start by just using the flag and then, after all, will be easier improve and/or adopted a kind of config by yaml file, kustomize, and etc ..

- The images spec property is required to be read by the operator to deploy resources
it creates (pods, deployments, statefulsets) where additional images are used.
- Add scorecard tests to verify the images spec is being used.
- Modify any operator-sdk clients to generate the images spec.

### User Stories

Copy link
Contributor

@camilamacedo86 camilamacedo86 Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that the proposals in this repo are not exactly in this way. But, IHMO should describe the User Stories by following its concept. E.g 👍

  • As a user, I'd like to inform an image and see the operator-SDK scaffolding the CR/CRD with the common expected specs and validations generated by it.
  • As a user, I'd like to inform an image and see the operator-SDK scaffolding the controller with the watches and reconcile actions implemented to create a Deployment and Service generated by it.

See https://www.mountaingoatsoftware.com/agile/user-stories. Then, we should add acceptance criteria. See here an example over how I think that we should do it.

Note that in the template we have the field ### Implementation Details/Notes/Constraints to describe and add notes over how we are suggesting technically the implementation be done, however, the rest of the doc in POV should be focused in the business rules. It helps us understand the goal and have a good definition of what are the behaviours expected when the proposal be implemented.

#### Create a common ImageListSpec and ImageSpec Interface.

Follow a kubernetes api pattern and provide an ImageSpec and ImageListSpec structs that
operator CRDs can reuse. I imagine operator-sdk would publish these like the
[PodSpec](https://github.com/kubernetes/api/blob/master/core/v1/types.go#L2831) found for kubernetes.

OpenAPIv3 Def for ImageSpec:

```yaml
type: Object
properties:
name:
type: string
description: Name of image used for lookup.
image:
type: string
description: Location of the image.
pullPolicy:
type: string
description: Pull Policy for the image.
pullSecrets:
type: array
description: Array of pull secret names to use.
items:
types: string
required: ['name', 'image']
```

OpenAPIv3 Def for ImageListSpec:

```yaml
images:
type: array
items:
type: object
properties:
name:
type: string
description: Name of image used for lookup.
image:
type: string
description: Location of the image.
pullPolicy:
type: string
description: Pull Policy for the image.
pullSecrets:
type: array
description: Array of pull secret names to use.
items:
types: string
required: ['name', 'image']
```

#### Add an image flag to sdk add api

Add a flag to preset images for operator-sdk add api command.

```bash
operator-sdk add api --api-version=app.example.com/v1alpha1 \
--kind=AppService --image=ngnix:latest
operator-sdk add api --api-version=app.example.com/v1alpha1 \
--kind=AppService \
--image=ngnix:latest \
--image=redis:latest
```

This flag will create an AppServiceSpec struct with prefilled data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd keep just the impl to gen the basic code and keep a smaller scope. If it is accepted and implemented, then we can have a better idea over how should be the next steps or additional needs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

```golang
type AppServiceSpec struct {

// Object map of image to name to an image location, pull
// secret names, and pull policy for the image.
Images operatorsdk.ImagesSpec

// INSERT ADDITIONAL SPEC FIELDS - desired state of cluster
// Important: Run "operator-sdk generate k8s" to regenerate code after modifying this file
// Add custom validation using kubebuilder tags: https://book-v1.book.kubebuilder.io/beyond_basics/generating_crd.html

}
```

#### Add a generate flag for operator-sdk specs

Generate flag added to operator-sdk add controller.

**Blanket generate:**
```bash
operator-sdk add controller --api-version=app.example.com/v1alpha1 \
--kind=AppService \
--generate
```

The generate flag will create the controller with scaffolding using the Images
property to create resources to be deployed. The generate command could be a
global flag that enables any type of operatorsdk features that generate
controller code. And we can include an option to subselect a generation.

**Subselecting a generation:**
```bash
operator-sdk add controller --api-version=app.example.com/v1alpha1 \
--kind=AppService \
--generate=images
```

### Future Work

#### Validation
Copy link
Contributor

@camilamacedo86 camilamacedo86 Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we scaffold the code all the other features will work as expected and we do not need to add any specific test in the scorecard. IMHO the scorecard should be 100% independent and be allowed to be used in any operator project done with SDK or NOT. I mean, It should ensure the quality of the project no matter how the user implemented it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your scaffold idea moves away from the original proposal and I disagree.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of a scorecard to me is to grade you on how well you have done something. Getting a grade on your operator should include following good practices and standardizations. I think looking for this feature should totally be in scope.


Operator score card and linters could perform validation of the use of
ImageSpecs in the controllers.


### Risks and Mitigations

- A user could override the images in an operator with images
that will not work with the operator. This would just result in a broken
operator. The status would have to reflect a failure because of wrong software
version.
- Providing default values for the images can be tricky. The default keyword in
the openapi v3 schema is in beta. Using enum is another method of providing a
default value but requires input by the creator of the CR. Mutating webhooks
is another method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we following my suggestion described above I think we will not have these risks.

### Upgrade / Downgrade Strategy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### Upgrade / Downgrade Strategy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not need to add all topics of the templates.
That ones which are not required or not make sense for the proposal can be removed/ignored.


NA.

### Version Skew Strategy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### Version Skew Strategy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not need to add all topics of the templates.
That ones which are not required or not make sense for the proposal can be removed/ignored.


NA.

## Implementation History
Copy link
Contributor

@camilamacedo86 camilamacedo86 Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not need to add all topics of the templates.
That ones which are not required or not make sense for the proposal can be removed/ignored.

Suggested change
## Implementation History


Major milestones in the life cycle of a proposal should be tracked in `Implementation History`.

## Drawbacks

- Requires changes from the operator developers.
- Different operators may already solve this problem in different ways.
- Not all CRDs require an image (adding a user to a database for example).
- Adds a requried data to an operator CRD that is otherwise freeform.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we following my suggestion described above I think we will not have Drawbacks as well.

## Alternatives

There is a similar proposal for the CSV definition [related
images](https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/contributors/design-proposals/related-images.md)
that OLM will use. This approach has a few problems for operands:

- Directly couples the image versions with the operator.
- Requires OLM support, or custom kubernetes runtimes (CRI-O), to override the image
locations at runtime without operator support.
- Images cannot be overriden by a user.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this proposal is accepted here I think we could aligned it with OLM team and move at least at the first moment doing it just in SDK. Also, IHMO it shows to be more part of the SDK scope than OLM.


[operator-sdk-doc]: ../../doc