Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

[WIP] CRD storage support #1105

Closed
wants to merge 2 commits into from
Closed

[WIP] CRD storage support #1105

wants to merge 2 commits into from

Conversation

nilebox
Copy link
Contributor

@nilebox nilebox commented Aug 6, 2017

Implementing #1088

Key differences compared to current TPR implementation:

  • structured APIs used in REST client (TPR code manually builds absolute paths every time)
  • different apiVersion (crd.servicecatalog.k8s.io) compared to API server as a workaround for API aggregator clashing (TPRs don't work with 1.7 aggregator)

Please review individual commits.

@nilebox nilebox requested a review from arschles August 6, 2017 23:21
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 6, 2017

type store struct {
// TODO (nilebox): review the usage of "hasNamespace" flag
hasNamespace bool
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the original TPR code, if this flag is set, you wipe the namespace from the objects even if they have a namespace. I haven't copied these code blocks yet.
@arschles why do we need this?

Copy link
Contributor

Choose a reason for hiding this comment

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

because some service-catalog resources do not have namespaces (i.e. Broker), but all TPR instances need namespaces. This flag indicated if the TPRs created by the interface should go into the specified namespace of the default global namespace

hasNamespace bool
defaultNamespace string
cl restclient.Interface
resourcePlural string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

resourcePlural is an alternative to singularKind we had for TPR implementation.

out runtime.Object,
ttl uint64,
) error {
// TODO (nilebox): can't we just use meta.Accessor(obj) instead?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arschles Can you tell why can't we extract namespace and name from the object itself instead of decoding key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason could be that for GET request there is only a key, and no runtime.Object input?
So the key here is for uniform way of passing namespace and name separate from the object?

Copy link
Contributor

Choose a reason for hiding this comment

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

iirc we could use meta.Accessor

return err
}

req := t.cl.Post().Resource(t.resourcePlural).Namespace(ns).Name(name).Body(obj)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arschles see this structured API call as an alternative to

	req := t.cl.Post().AbsPath(
		"apis",
		groupName,
		tprVersion,
		"namespaces",
		ns,
		t.singularKind.URLName(),
	).Body(data)

in all other places, GET, PUT and all other requests use the same approach.
Why didn't you use this approach for TPRs? Do you require some extra stuff at the raw level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that setting up a REST client with the scheme supporting all Service Catalog kinds, this approach should work

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think your structured API call is wrong. I just used the various functions to build up the request instead of using AbsPath

glog.Errorf("initiating the raw watch (%s)", err)
return nil, err
}
return watchIface, nil
Copy link
Contributor Author

@nilebox nilebox Aug 6, 2017

Choose a reason for hiding this comment

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

In the original TPR code this result is wrapped into filter: https://github.com/kubernetes-incubator/service-catalog/blob/master/pkg/storage/tpr/storage_interface.go#L249
@arschles As far as I understand, this method is essentially a "map" function (as a functional programming term) for transforming a collection of type X type into a collection of type Y?
Not sure we actually need this here.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, the filter is essentially a map function

)

// Options is the set of options to create a new TPR storage interface
type Options struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will need a Validate() method to call per upstream guidance. As part should maybe call the RESTOptions.Validate() if it exists.

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 4, 2017

Verified

This commit was signed with the committer’s verified signature.
h3ndrk Hendrik
@nilebox
Copy link
Contributor Author

nilebox commented Sep 12, 2017

@arschles @MHBauer @krancour CRD support is working now, please review.
Things TODO as a follow-up (this PR is already huge):

  • address issue tpr-based storage needs some work #599 (the CRD storage code is based on existing TPR code, and probably has the same problems)
  • CRDs support both Namespaced- and Cluster scopes (unlike TPRs which support only Namespace-scoped resources), so instead of using hackery with predefined "global" namespace, we can set Cluster scope for ServiceBroker and other global resources
  • Prefix CRD resources with something like crd- to avoid clashing in kubectl with Service Catalog API server resources; this change is blocked by kubectl issue Cannot list custom objects defined by CRD with non-conventional pluralization kubernetes/kubernetes#51639. Currently to distinguish between Service Catalog API server and CRDs, you need to add an unambiguous suffix (a prefix of apiVersion) like:
kubectl get serviceinstances.servicecatalog

vs

kubectl get serviceinstances.crd

By default (with kubectl get serviceinstances) it redirects to Service Catalog API server, which is the desirable behavior.

@nilebox
Copy link
Contributor Author

nilebox commented Sep 12, 2017

One additional step is required for making CRDs work compared to default https://github.com/kubernetes-incubator/service-catalog/blob/master/docs/install-1.7.md:

kubectl create clusterrolebinding servicecatalog-cluster-admin --clusterrole=cluster-admin --user=system:serviceaccount:catalog:service-catalog-apiserver

there might be some more granular permission enough, but without this change the registration of CRDs was failing.

@arschles do we have a doc for TPR support? I haven't found one. I would extend it with CRD instructions then.

@nilebox nilebox changed the title WIP: CRD storage CRD storage support Sep 12, 2017
@nilebox
Copy link
Contributor Author

nilebox commented Sep 13, 2017

Made ServiceBroker and ServiceClass resources Cluster-scoped.

@nilebox
Copy link
Contributor Author

nilebox commented Sep 14, 2017

Thanks to https://github.com/liggitt/audit2rbac I've found a more granular RBAC roles and bindings to allow Service Catalog manage its CRD resources (rewrote into kubectl commands and made it more specific with resource names):

kubectl create clusterrole servicecatalog:crd-manager \
    --verb=create,update,get,list,watch \
    --resource=customresourcedefinitions
kubectl create clusterrolebinding servicecatalog:crd-manager \
    --clusterrole=servicecatalog:crd-manager \
    --user=system:serviceaccount:catalog:service-catalog-apiserver

# only after CRDs have been created
kubectl create clusterrole servicecatalog:crd-resources \
    --verb=create,update,get,list,watch \
    --resource=servicebrokers.crd.servicecatalog.k8s.io,serviceclasses.crd.servicecatalog.k8s.io,serviceinstances.crd.servicecatalog.k8s.io,serviceinstancecredentials.crd.servicecatalog.k8s.io
kubectl create clusterrolebinding servicecatalog:servicecatalog-crd-resources \
    --clusterrole=servicecatalog:crd-resources \
    --user=system:serviceaccount:catalog:service-catalog-apiserver

apiextCfg, err := restclient.InClusterConfig()
if err != nil {
glog.Errorf("Failed to get kube client config (%s)", err)
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not return nil, fmt.Errorf("failed to get kube client config: %v", err)? Should not log and return the error - will likely be logged up the call stack so more than once.
There are more places like that.

Update: ok, you are following existing style. Should probably be a separate cleanup PR to fix all the places.

}
res := req.Do()
if res.Error() != nil {
glog.Errorf("executing DELETE for %s/%s (%s)", ns, name, res.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a return fmt.Errorf(....)? Why log anything when this function can return the error?
I guess you've been following the existing style, but this is can be improved, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Think about it - there is no way caller gets any information about this err. This is not right.

p.s. same in other files, will not comment there.

} else {
glog.Infof("Created CRD '%s'", crd.Name)

// There can be a delay, so poll until it's ready to go...
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what this polling is waiting for - it should be always be possible to instantly get after create?
It should probably check conditions to see if names have been accepted and that the resource has been established but it is not doing any of that now.

go func(crd crdv1beta1.CustomResourceDefinition, client crdclient.CustomResourceDefinitionInterface) {
defer wg.Done()
if _, err := cl.Create(&crd); err != nil {
errMsg <- fmt.Sprintf("%s: %s", crd.Name, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a return here and eliminate else nesting?


if allErrMsg != "" {
glog.Errorf("Failed to create Custom Resource:\n%s", allErrMsg)
return ErrCRDInstall{fmt.Sprintf("Failed to create Custom Resource: %s", allErrMsg)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as in other places - either log or return an error, not both :)

var unknown runtime.Unknown
if err := req.Do().Into(&unknown); err != nil {
glog.Errorf("doing request (%s)", err)
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

This drives me crazy :) How one is supposed to debug this later? Collect pieces of information from multiple lines?

"k8s.io/apimachinery/pkg/api/meta"
"k8s.io/apimachinery/pkg/runtime"
restclient "k8s.io/client-go/rest"
"k8s.io/kubernetes/pkg/api/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be an import from client-go?

}

// getAllNamespaces uses cl to get all namespaces
func getAllNamespaces(cl restclient.Interface) (*v1.NamespaceList, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a typed client for namespaces - why not use it?

}

func (e errCouldntGetResourceVersion) Error() string {
return fmt.Sprintf("couldn't get resource version (%s)", e.err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think %s and %v format error differently and %v is the right way to do it?

}

if t.hasNamespace && ns == t.defaultNamespace {
// TODO nilebox: The problem with distinguishing between "-n default" and "--all-namespaces"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like an issue, a ticket should be created for this?

@nilebox
Copy link
Contributor Author

nilebox commented Sep 24, 2017

@ash2k I would like to leave the error logging/propagation discussion for the follow-up PRs. It was copy-pasted from TPR support code, and I suppose that you can find the same pattern in the entire codebase.
Will address other comments in the next few days.

@nilebox
Copy link
Contributor Author

nilebox commented Oct 3, 2017

TODO: adopt fix #1275

@nilebox nilebox changed the title CRD storage support [WIP] CRD storage support Oct 22, 2017
@nilebox
Copy link
Contributor Author

nilebox commented Nov 17, 2017

Closing as I think that we need to revisit the requirements and decide whether we want to support CRDs in Service Catalog.

If anyone wants to pick up this pull request - feel free.

@nilebox nilebox closed this Nov 17, 2017
@n3wscott
Copy link
Contributor

I will. #1971

@amckague amckague deleted the nilebox/crd branch August 9, 2018 05:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants