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

apiextensions: add subresources for custom resources #55168

Merged
merged 3 commits into from Feb 22, 2018

Conversation

@nikhita
Copy link
Member

nikhita commented Nov 6, 2017

Fixes #38113
Fixes #58778

Related:

Add types:

  • Add CustomResourceSubresources type to CRD.
    • Fix proto generation for CustomResourceSubresourceStatus: #55970.
  • Add feature gate for CustomResourceSubresources.
    • Update CRD strategy: if feature gate is disabled, this feature is dropped (i.e. set to nil).
  • Add validation for CustomResourceSubresources:
    • SpecReplicasPath should not be empty and should be a valid json path under .spec. If there is no value under the given path in the custom resource, the /scale subresource will return an error on GET.
    • StatusReplicasPath should not be empty and should be a valid json path under .status. If there is no value under the given path in the custom resource, the status replica value in the /scale subresource will default to 0.
    • If present, LabelSelectorPath is optional and should be a valid json path under .status. If there is no value under LabelSelectorPath in the custom resource, the status label selector value in the /scale subresource will default to the empty string.
    • If CustomResourceSubresources feature gate is enabled, only properties is allowed under the root schema for CRD validation.

Add status and scale subresources:

  • Use helper functions from apimachinery/pkg/apis/meta/v1/unstructured/helpers.go.
  • Introduce Registry interface for storage.
  • Update storage:
    • Introduce CustomResourceStorage which acts as storage for the custom resource and its status and scale subresources. Note: storage for status and scale is only enabled when the feature gate is enabled and the respective fields are enabled in the CRD.
    • Introduce StatusREST and its New(), Get() and Update() methods.
    • Introduce ScaleREST and its New(), Get() and Update() methods.
      • Get and Update use the json paths from the CRD and use it to return an autoscaling/v1.Scale object.
  • Update strategy:
    • In PrepareForCreate,
      • Clear .status.
      • Set .metadata.generation = 1
    • In PrepareForUpdate,
      • Do not update .status.
        • If both the old and new objects have .status and it is changed, set it back to its old value.
        • If the old object has a .status but the new object doesn't, set it to the old value.
        • If old object did not have a .status but the new object does, delete it.
      • Increment generation if spec changes i.e. in the following cases:
        • If both the old and new objects had .spec and it changed.
        • If the old object did not have .spec but the new object does.
        • If the old object had a .spec but the new object doesn't.
    • In Validate and ValidateUpdate,
      • ensure that values at specReplicasPath and statusReplicasPath are >=0 and < maxInt32.
      • make sure there are no errors in getting the value at all the paths.
    • Introduce statusStrategy with its methods.
      • In PrepareForUpdate:
        • Do not update .spec.
          • If both the old and new objects have .spec and it is changed, set it back to its old value.
          • If the old object has a .spec but the new object doesn't, set it to the old value.
          • If old object did not have a .spec but the new object does, delete it.
        • Do not update .metadata.
      • In ValidateStatusUpdate:
        • For CRD validation, validate only under .status.
        • Validate value at statusReplicasPath and labelSelectorPath as above.
  • Plug into the custom resource handler:
    • Store all three storage - customResource, status and scale in crdInfo.
    • Use the storage as per the subresource in the request.
    • Use the validator as per the subresource (for status, only use the schema for status, if present).
    • Serve the endpoint as per the subresource - see serveResource, serveStatus and serveScale.
  • Update discovery by adding the /status and /scale resources, if enabled.

Add tests:

  • Add unit tests in etcd_test.go.
  • Add integration tests.
    • In subresources_test.go, use the polymporphic scale client to get and update Scale.
    • Add a test to check everything works fine with yaml in yaml_test.go.

Release note:

`/status` and `/scale` subresources are added for custom resources.
@nikhita

This comment has been minimized.

Copy link
Member

nikhita commented Nov 6, 2017


// CustomResourceSubResourceScale defines how to serve the HTTP path <CR name>/scale.
type CustomResourceSubResourceScale struct {
// required, e.g. “.spec.replicas”.

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

do we use typographical quotes?

This comment has been minimized.

@nikhita

nikhita Nov 9, 2017

Member

Fixed.

type CustomResourceSubResourceScale struct {
// required, e.g. “.spec.replicas”.
// Only JSON paths without the array notation are allowed.
SpecReplicasPath string `json:"specReplicasPath" protobuf:"bytes,1,opt,name=specReplicasPath"`

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

not opt

This comment has been minimized.

@nikhita

nikhita Nov 9, 2017

Member

This should be fixed when proto generation is fixed. Currently, update-generated-protobuf fails on CustomResourceSubResourceStatus


if subResources.Scale != nil {
if len(subResources.Scale.SpecReplicasPath) == 0 {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("scale.specReplicasPath"), "specReplicasPath cannot be empty"))

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

better: field.Invalid

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

also below

This comment has been minimized.

@nikhita

nikhita Nov 9, 2017

Member

Done.

@@ -116,3 +118,76 @@ func CRDRemoveFinalizer(crd *CustomResourceDefinition, needle string) {
}
crd.Finalizers = newFinalizers
}

// GetJSONPath returns the value in the data at JSONPath path.
func GetJSONPath(data interface{}, path string) interface{} {

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

we have public unstructured helpers now. Would they work for you? apimachinery/pkg/apis/meta/v1/unstructured/helpers.go

This comment has been minimized.

@nikhita

nikhita Nov 6, 2017

Member

yes, they probably should. I'll try with them.

This comment has been minimized.

@nikhita

nikhita Nov 9, 2017

Member

Done.

var _ rest.CategoriesProvider = &REST{}

// Categories implements the CategoriesProvider interface. Returns a list of categories a resource is part of.
func (r *REST) Categories() []string {

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

This makes every CR part of kubectl get all. Wondering, do we have a way to opt-in to that group in a CRD?

This comment has been minimized.

@nikhita

nikhita Nov 9, 2017

Member

Wondering, do we have a way to opt-in to that group in a CRD?

Maybe another field in the CRD? 🤔


scaleObject, err := scaleFromCustomResource(cr, r.scaleJSONPath)
if err != nil {
return nil, errors.NewBadRequest(fmt.Sprintf("%v", err))

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

err.Error()?

This comment has been minimized.

@nikhita

nikhita Nov 9, 2017

Member

Done.

}

if errs := scalevalidation.ValidateScale(scale); len(errs) > 0 {
return nil, false, errors.NewInvalid(scalescheme.Kind("scale"), scale.Name, errs)

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

not capital Scale?


type ScaleREST struct {
registry Registry
scaleJSONPath map[string]string

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

why a map?

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

would make all of the paths explicit here

This comment has been minimized.

@nikhita

nikhita Nov 6, 2017

Member

To have something like this:

scaleJSONPath["specReplicasPath"] = crd.Spec.SubResources.Scale.SpecReplicasPath
scaleJSONPath["statusReplicasPath"] = crd.Spec.SubResources.Scale.StatusReplicasPath
scaleJSONPath["labelSelectorPath"] = crd.Spec.SubResources.Scale.LabelSelectorPath

This comment has been minimized.

@nikhita

nikhita Nov 6, 2017

Member

would make all of the paths explicit here

👍

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

3 string fields would do as well?

This comment has been minimized.

@nikhita

nikhita Nov 6, 2017

Member

yes 👍

return nil, fmt.Errorf("spec replicas value should be a non-negative integer")
}
} else {
specReplicas = 0

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

isn't it mandatory?

customResource := customResourceObject.UnstructuredContent()

if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceSubResources) {

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

These tests should go into the CRD validation.

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

also below

This comment has been minimized.

@nikhita

nikhita Nov 9, 2017

Member

This validates the "custom resource". So I think it should not go in the CRD strategy and should instead remain the in the CR strategy.

@sttts WDYT?

This comment has been minimized.

@sttts

sttts Nov 9, 2017

Contributor

hmm, I think customResource["oneOf"] does not make sense here. We have to check that OneOf is not used in the root of the JSON schema in the CRD.

@@ -24,7 +24,7 @@ import (
apimeta "k8s.io/apimachinery/pkg/api/meta"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/discovery"
discocache "k8s.io/client-go/discovery/cached" // Saturday Night Fever
discocache "k8s.io/client-go/discovery/cached"

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

This comment has been minimized.

@DirectXMan12

DirectXMan12 Nov 6, 2017

Contributor

:-(. It was fun while it lasted

@DirectXMan12

This comment has been minimized.

Copy link
Contributor

DirectXMan12 commented Nov 6, 2017

@DirectXMan12
Copy link
Contributor

DirectXMan12 left a comment

You're using the scheme used internally to the scale client as your internal version, which I'd say is not ok.

Other comments inline

// required, e.g. “.spec.replicas”.
// Only JSON paths without the array notation are allowed.
SpecReplicasPath string
// optional, e.g. “.status.replicas”.

This comment has been minimized.

@DirectXMan12

DirectXMan12 Nov 6, 2017

Contributor

why is StatusReplicas optional? If it defaults to something otherwise, you should probably write that here (same with the other optional fields)

This comment has been minimized.

@sttts

sttts Nov 9, 2017

Contributor

If I remember right, the idea was that a scale subresource could exist without the scale being reflected in the status. So there is no defaulting of this field.

This comment has been minimized.

@nikhita

nikhita Nov 9, 2017

Member

Adding on to what @sttts said, the user cannot write to the /status .status of the scale object at all. It is completely read-only for the user. Only a controller has write access to the status fields. So we can leave it empty and not have it default to anything. This also matches our plan with CRDs.

"k8s.io/apiserver/pkg/registry/generic"
genericregistry "k8s.io/apiserver/pkg/registry/generic/registry"
"k8s.io/apiserver/pkg/registry/rest"
scalescheme "k8s.io/client-go/scale/scheme"

This comment has been minimized.

@DirectXMan12

DirectXMan12 Nov 6, 2017

Contributor

This was intended to be purely internal to the scale client. The internal types aren't supposed to be used by other people.

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

Make them internal.

This comment has been minimized.

@DirectXMan12

DirectXMan12 Nov 6, 2017

Contributor

you mean change the name? Because they are marked as having the version be __internal.

This comment has been minimized.

@sttts

sttts Nov 6, 2017

Contributor

We don't make any use of that, but Go 1.5+ has packages called internal which can only be imported if your package and the internal one shares a common root. Does not belong into this PR, but I really think we should move a lot of internal code into internal packages. These client schemes are candidates for that. So I agree, we probably should not use the scale scheme here in the registry.

This comment has been minimized.

@nikhita

nikhita Nov 9, 2017

Member

Uses external type for autoscaling now. 👍

)

// ValidateScale is used to validate scheme.Scale
func ValidateScale(scale *scheme.Scale) field.ErrorList {

This comment has been minimized.

@DirectXMan12

DirectXMan12 Nov 6, 2017

Contributor

I don't see what purpose this serves. This is never supposed to be used as an API server type.

This comment has been minimized.

@nikhita

nikhita Nov 9, 2017

Member

Removed.

@nikhita nikhita force-pushed the nikhita:customresources-subresources branch from b716144 to b80000b Nov 9, 2017

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/XXL labels Nov 9, 2017

scaleLabelSelector := &metav1.LabelSelector{}
if ls != nil {
var customResourceLabelSelector []byte
ls, ok := unstructured.NestedFieldCopy(cr.UnstructuredContent(), strings.Split(labelSelectorPath, ".")...)

This comment has been minimized.

@nikhita

nikhita Nov 9, 2017

Member

This needs fixing. Will do that.

This comment has been minimized.

@nikhita

nikhita Nov 9, 2017

Member

Oops, done in 6f36088. Should be fine after a squash.

@nikhita nikhita force-pushed the nikhita:customresources-subresources branch from b80000b to 30fe1d1 Nov 13, 2017

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Feb 22, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 22, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, deads2k, lavalamp, nikhita, sttts

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-merge-robot

This comment has been minimized.

Copy link
Contributor

k8s-merge-robot commented Feb 22, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Copy link
Contributor

k8s-merge-robot commented Feb 22, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 6e85648 into kubernetes:master Feb 22, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-verify
Details
cla/linuxfoundation nikhita authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Feb 22, 2018

🎉

@tamalsaha

This comment has been minimized.

Copy link
Member

tamalsaha commented Feb 23, 2018

Thanks @nikhita @sttts and everyone who helped to get this added!

@idvoretskyi

This comment has been minimized.

Copy link
Member

idvoretskyi commented Feb 23, 2018

@nikhita @sttts can you please link this to an issue in https://github.com/kubernetes/features

@nikhita

This comment has been minimized.

Copy link
Member

nikhita commented Feb 23, 2018

k8s-merge-robot added a commit that referenced this pull request Feb 23, 2018

Merge pull request #58283 from nikhita/kubectl-scale-unstructured
Automatic merge from submit-queue (batch tested with PRs 59463, 59719, 60181, 58283, 59966). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

kubectl scale: support Unstructured objects

Support `Unstructured` objects with kubectl scale.

So that we can use the scale subresource for custom resources (possible after #55168 is merged):

```
➜ cluster/kubectl.sh scale --replicas=5 crontabs/my-new-cron-object
crontab "my-new-cron-object" scaled
```

**Release note**:

```release-note
NONE
```

/cc sttts deads2k p0lyn0mial

aanm added a commit to cilium/cilium that referenced this pull request Mar 8, 2018

pkg/k8s: remove OneOf in CRD schema
Since k8s 1.11 enables CustomResourceSubresources features by default
cilium is no longer able to install it's CRD validation in the
kube-apiserver for having other fields in the root schema besides
"properties"

Change introduced by: kubernetes/kubernetes#55168

Signed-off-by: André Martins <andre@cilium.io>

aanm added a commit to cilium/cilium that referenced this pull request Mar 8, 2018

pkg/k8s: remove OneOf in CRD schema
Since k8s 1.11 enables CustomResourceSubresources features by default
cilium is no longer able to install it's CRD validation in the
kube-apiserver for having other fields in the root schema besides
"properties"

Change introduced by: kubernetes/kubernetes#55168

Signed-off-by: André Martins <andre@cilium.io>

tgraf added a commit to cilium/cilium that referenced this pull request Mar 8, 2018

pkg/k8s: remove OneOf in CRD schema
Since k8s 1.11 enables CustomResourceSubresources features by default
cilium is no longer able to install it's CRD validation in the
kube-apiserver for having other fields in the root schema besides
"properties"

Change introduced by: kubernetes/kubernetes#55168

Signed-off-by: André Martins <andre@cilium.io>
@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Apr 3, 2018

/lgtm
/approve

k8s-merge-robot added a commit that referenced this pull request Apr 6, 2018

Merge pull request #60021 from nikhita/sample-controller-subresources
Automatic merge from submit-queue (batch tested with PRs 60102, 59970, 60021, 62011, 62080). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

sample-controller: add status subresource support

Builds on top of #55168.

**DO NOT MERGE** until #55168 is merged. Adding a hold.
/hold

Update: It is now merged! 🎉 

This PR:

- Adds an example to show how to use the `/status` subresource with custom resources.
- Generates `UpdateStatus` for the `Foo` resource.
- Updates the comment in the controller to mention that `UpdateStatus` can now be used. Note: this is not enabled by default because subresources require the feature gate to be enabled and are not on by default.
- Updates the README to add feature gate information and examples for `CustomResourceSubresources`.
- Updates the README to remove feature gate information for CRD validation since the current example uses `apps/v1` deployments (and thus requires v1.9 anyway).

**Release note**:

```release-note
NONE
```

/assign sttts munnerz

k8s-publishing-bot added a commit to kubernetes/sample-controller that referenced this pull request Apr 6, 2018

Merge pull request #60021 from nikhita/sample-controller-subresources
Automatic merge from submit-queue (batch tested with PRs 60102, 59970, 60021, 62011, 62080). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

sample-controller: add status subresource support

Builds on top of kubernetes/kubernetes#55168.

**DO NOT MERGE** until kubernetes/kubernetes#55168 is merged. Adding a hold.
/hold

Update: It is now merged! 🎉

This PR:

- Adds an example to show how to use the `/status` subresource with custom resources.
- Generates `UpdateStatus` for the `Foo` resource.
- Updates the comment in the controller to mention that `UpdateStatus` can now be used. Note: this is not enabled by default because subresources require the feature gate to be enabled and are not on by default.
- Updates the README to add feature gate information and examples for `CustomResourceSubresources`.
- Updates the README to remove feature gate information for CRD validation since the current example uses `apps/v1` deployments (and thus requires v1.9 anyway).

**Release note**:

```release-note
NONE
```

/assign sttts munnerz

Kubernetes-commit: 7bde13f191f0791a87fe5e2575feb3d4849de536

sttts pushed a commit to sttts/sample-controller that referenced this pull request Apr 9, 2018

Merge pull request #60021 from nikhita/sample-controller-subresources
Automatic merge from submit-queue (batch tested with PRs 60102, 59970, 60021, 62011, 62080). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

sample-controller: add status subresource support

Builds on top of kubernetes/kubernetes#55168.

**DO NOT MERGE** until kubernetes/kubernetes#55168 is merged. Adding a hold.
/hold

Update: It is now merged! 🎉

This PR:

- Adds an example to show how to use the `/status` subresource with custom resources.
- Generates `UpdateStatus` for the `Foo` resource.
- Updates the comment in the controller to mention that `UpdateStatus` can now be used. Note: this is not enabled by default because subresources require the feature gate to be enabled and are not on by default.
- Updates the README to add feature gate information and examples for `CustomResourceSubresources`.
- Updates the README to remove feature gate information for CRD validation since the current example uses `apps/v1` deployments (and thus requires v1.9 anyway).

**Release note**:

```release-note
NONE
```

/assign sttts munnerz

Kubernetes-commit: 7bde13f191f0791a87fe5e2575feb3d4849de536

openshift-publish-robot pushed a commit to openshift/kubernetes-sample-controller that referenced this pull request Jan 14, 2019

Merge pull request #60021 from nikhita/sample-controller-subresources
Automatic merge from submit-queue (batch tested with PRs 60102, 59970, 60021, 62011, 62080). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

sample-controller: add status subresource support

Builds on top of kubernetes/kubernetes#55168.

**DO NOT MERGE** until kubernetes/kubernetes#55168 is merged. Adding a hold.
/hold

Update: It is now merged! 🎉

This PR:

- Adds an example to show how to use the `/status` subresource with custom resources.
- Generates `UpdateStatus` for the `Foo` resource.
- Updates the comment in the controller to mention that `UpdateStatus` can now be used. Note: this is not enabled by default because subresources require the feature gate to be enabled and are not on by default.
- Updates the README to add feature gate information and examples for `CustomResourceSubresources`.
- Updates the README to remove feature gate information for CRD validation since the current example uses `apps/v1` deployments (and thus requires v1.9 anyway).

**Release note**:

```release-note
NONE
```

/assign sttts munnerz

Kubernetes-commit: 7bde13f191f0791a87fe5e2575feb3d4849de536
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment