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

Change API Group and Kind, and Spec #495

Closed
wants to merge 19 commits into from

Conversation

sbose78
Copy link
Member

@sbose78 sbose78 commented May 27, 2020

TLDR;

This is how the new CR looks like

---
apiVersion: operators.coreos.com/v1alpha1
kind: ServiceBinding
metadata:
  name: example-servicebinding
  namespace: sbose
spec:
  services:
    - name: demo-database
      group: postgresql.baiju.dev
      kind: Database
      version: v1alpha1

  application:
    name: nodejs-rest-http-crud
    group: apps
    version: v1
    resource: deployments

NOTE: I've used the operators.coreos.com API Group for the time-being since we are in discussions with the OLM team on moving this project to github.com/operator-framework . However, folks are open to using a different API Group too. While I don't want to deviate too much, I am considering using an OperatorFramework-ish group & kind. Suggestions welcome! :)

What should you review?

You don't really need to start looking at all 80+ files changed.
This file is what you need to be look at, to begin with: servicebinding_types.go

Motivation

Primarily

  • Align the ServiceBinding API with the Proposal which also takes into account the new specification.
  • Open up the API for review by the OLM team. ( There's no point doing an API review for the API which is in master since it was going to change )

Also ..

  • Standardize use of Golang Kubernetes APIs. Example, used corev1.LocalObjectReference instead of name wherever application.

Changes

  • Set a Group and Kind which is Kubernetes / OperatorFramework friendly.
apiVersion: operators.coreos.com/v1alpha1
kind: ServiceBinding
  • Removed spec.backingServiceSelector

  • Replaced spec.applicationSelector with spec.application,

  • Using Application *Application instead of Application Application ( feedback by OLM team )

  • Using DetectBindingResources *bool instead of DetectBindingResources bool ( feedback by OLM team )

  • Replaced spec.backingServiceSelectors with spec.services

  • Replaced spec.customEnvVar with spec.dataMapping <--- we don't have agreement on this, we may keep this as is.

  • Replaced resourceRef in application and services to name corev1.LocalObjectReference

  • PENDING: The Application type has a label selector and a LocalObjectReference do I need to set a name and then use a label selector? I think that these should be a union type

  • PENDING: Change spec.application ( Application ) to spec.applications ( []Application ). To do so without breaking changes ini add extraFieldsModifier to binder #426 , some amount of refactoring is needed.

Pre-merge checklist

  • Update docs and samples scenarios
  • Safe to freeze new community operator releases to let the UI catch up.
  • Finalize API Group ( see NOTE in the motivation section )
  • CRD Spec Reviewed by OLM team

Testing

  1. No extra tests added. Ensured existing ones work!
  2. Validated manually by creating a binding between a nodejs app & Database.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign sbose78
You can assign the PR to them by writing /assign @sbose78 in a comment when ready.

The full list of commands accepted by this bot can be found 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

@sbose78
Copy link
Member Author

sbose78 commented May 27, 2020

@lanceliuu Could you see my comment in the pre-merge checklist above. I would need your assistance in refactoring that feature to be able to support multiple applications. A PR would be nice :)

@sbose78
Copy link
Member Author

sbose78 commented May 27, 2020

Should we use corev1.LocalObjectReference for spec.services.name? Doing so feels right since a standard Kubernetes API is being used, but at the same time, the CRD validation schema doesn't mark it as required.

services:
    - name: demo-database <--- corev1.LocalObjectReference
      group: postgresql.baiju.dev
      kind: Database
      version: v1alpha1
LocalObjectReference struct {
    // Name of the referent.
    // More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
    // TODO: Add other useful fields. apiVersion, kind, uid?
    // +optional
    Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"`
}

Suggestions ?

@baijum
Copy link
Member

baijum commented May 27, 2020

NOTE: I've used the operators.coreos.com Group for the time-being since we are in discussions with the OLM team to moving this project to github.com/operator-framework . However, folks are open to use a different Group and Kind too. While I don't want to deviate too much, I am considering using an OperatorFramework-ish group & kind. Suggestions welcome! :)

Issue #364 is discussing the same, right? I thought we decided to use service.binding group based on the discussion there.

@sbose78
Copy link
Member Author

sbose78 commented May 27, 2020

@baijum Correct, we could end up using service.binding as the group as well.

However, groups tend to be upstream or downstream organization / technology specific and service.binding is neither. operators.coreos.com is currently the Operator Framework org widely in use. I'd like to see if we can get any alignment.

@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #495 into master will decrease coverage by 0.12%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #495      +/-   ##
==========================================
- Coverage   63.07%   62.95%   -0.13%     
==========================================
  Files          27       27              
  Lines        1774     1768       -6     
==========================================
- Hits         1119     1113       -6     
  Misses        489      489              
  Partials      166      166              
Impacted Files Coverage Δ
...ntroller/servicebinding/annotations/annotations.go 0.00% <ø> (ø)
...controller/servicebinding/annotations/attribute.go 80.00% <ø> (ø)
...ntroller/servicebinding/annotations/bindinginfo.go 88.23% <ø> (ø)
...controller/servicebinding/annotations/configmap.go 85.71% <ø> (ø)
...kg/controller/servicebinding/annotations/errors.go 0.00% <ø> (ø)
.../controller/servicebinding/annotations/resource.go 65.65% <ø> (ø)
...kg/controller/servicebinding/annotations/secret.go 69.23% <ø> (ø)
pkg/controller/servicebinding/common.go 29.41% <ø> (ø)
pkg/controller/servicebinding/controller.go 0.00% <ø> (ø)
pkg/controller/servicebinding/custom_env_parser.go 80.95% <ø> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3e812b...b4dc1ab. Read the comment docs.

@Avni-Sharma
Copy link
Contributor

Should we use corev1.LocalObjectReference for spec.services.name? Doing so feels right since a standard Kubernetes API is being used, but at the same time, the CRD validation schema doesn't mark it as required.

services:
    - name: demo-database <--- corev1.LocalObjectReference
      group: postgresql.baiju.dev
      kind: Database
      version: v1alpha1
LocalObjectReference struct {
    // Name of the referent.
    // More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
    // TODO: Add other useful fields. apiVersion, kind, uid?
    // +optional
    Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"`
}

Suggestions ?

Name is required so IMO we should keep it as is, unless we have any scenario where it is optional

// Service defines the selector based on resource name, version, and resource kind
type Service struct {
metav1.GroupVersionKind `json:",inline"`
corev1.LocalObjectReference `json:",inline"`
Copy link
Contributor

Choose a reason for hiding this comment

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

We have LabelSelector for Service.
Let's have it for Services as well. We do support multiple services.

Copy link
Member Author

@sbose78 sbose78 May 27, 2020

Choose a reason for hiding this comment

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

Wondering if that might affect the way we are:

  • referring to backing services in custom env var.
  • using env prefixes per service.

// +optional
LabelSelector *metav1.LabelSelector `json:"labelSelector,omitempty"`
metav1.GroupVersionResource `json:",inline"`
ResourceRef string `json:"resourceRef,omitempty"`
corev1.LocalObjectReference `json:",inline"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this // +optional as well? In case when LabelSelector is being used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the content of the struct is actually optional

LocalObjectReference struct {
    // Name of the referent.
    // More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
    // TODO: Add other useful fields. apiVersion, kind, uid?
    // +optional
    Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"`
}

We could bubble up the information at this Application struct level too though.

ResourceRef string `json:"resourceRef"`
// Service defines the selector based on resource name, version, and resource kind
type Service struct {
metav1.GroupVersionKind `json:",inline"`
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove metav1.GroupVersionKind and just have corev1.TypedLocalObjectReference.

// TypedLocalObjectReference contains enough information to let you locate the
// typed referenced object inside the same namespace.
type TypedLocalObjectReference struct {
	// APIGroup is the group for the resource being referenced.
	// If APIGroup is not specified, the specified Kind must be in the core API group.
	// For any other third-party types, APIGroup is required.
	// +optional
	APIGroup *string `json:"apiGroup" protobuf:"bytes,1,opt,name=apiGroup"`
	// Kind is the type of resource being referenced
	Kind string `json:"kind" protobuf:"bytes,2,opt,name=kind"`
	// Name is the name of resource being referenced
	Name string `json:"name" protobuf:"bytes,3,opt,name=name"`
}

Copy link
Member Author

Choose a reason for hiding this comment

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

That is an excellent idea. We could use it for Services only though. Since name is optional in Applications , we'll not be able to use TypedLocalObjectReference.

Copy link
Member Author

Choose a reason for hiding this comment

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

..umm.. but APIGroup *string isn't supposed to accept the Version

Copy link
Contributor

Choose a reason for hiding this comment

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

for services we need group version and Kind, corev1.TypedLocalObjectReference will indeed not give the Version

Copy link
Contributor

Choose a reason for hiding this comment

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

Moreover corev1.TypedLocalObjectReference is for the same namespace and we can have services in different namespaces

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps corev1.ObjectReference can be a good fit, since it has both Namespace and APIVersion members?

Regardless, can you clarify the importance of the API version in this particular case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I gave it a shot here #495 (comment)

@@ -358,13 +358,13 @@ deploy-rbac:
.PHONY: deploy-crds
## Deploy-CRD: Deploy CRD
deploy-crds:
$(Q)kubectl apply -f deploy/crds/apps.openshift.io_servicebindingrequests_crd.yaml
$(Q)kubectl apply -f deploy/crds/operators.coreos.com_servicebindings_crd.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

could we make this a bit more name agnostic, and avoid future changes as well:

$(Q)kubectl apply -f deploy/crds/*_crd.yaml


.PHONY: deploy-clean
## Deploy-Clean: Removing CRDs and CRs
deploy-clean:
$(Q)-kubectl delete -f deploy/crds/apps_v1alpha1_servicebindingrequest_cr.yaml
$(Q)-kubectl delete -f deploy/crds/apps.openshift.io_servicebindingrequests_crd.yaml
$(Q)-kubectl delete -f deploy/crds/operators.coreos.com_servicebindings_cr.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

same question as above

Copy link
Contributor

Choose a reason for hiding this comment

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

would you also consider extracting operators.coreos.com in a variable and use it consistently across Makefile? It would make later updates easier.

@@ -414,7 +414,7 @@ endif
## Copy crd from deploy/crds to manifests-upstream/
consistent-crds-manifests-upstream:
$(Q)cd ./manifests-upstream/${OPERATOR_VERSION}/ && ln -srf ../../deploy/crds/apps_v1alpha1_servicebindingrequest_crd.yaml \
Copy link
Contributor

Choose a reason for hiding this comment

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

this line should be updated as well, since apps_v1alpha1_servicebindingrequest_crd.yaml is getting renamed.

@@ -0,0 +1,18 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

The PR diff and later git history would look nicer if we rename the files using git mv ... command, and then apply the necessary changes/ With that git will able to understand that operators.coreos.com_servicebindings_cr.yaml is the successor of apps_v1alpha1_servicebindingrequest_cr.yaml , and all pending PRs that might contain changes to the old file would be applied correctly to the new one.


// DetectBindingResources is flag used to bind all non-bindable variables from
// different subresources owned by backing operator CR.
// +optional
DetectBindingResources bool `json:"detectBindingResources,omitempty"`
}

// ServiceBindingRequestStatus defines the observed state of ServiceBindingRequest
// ServiceBindingStatus defines the observed state of ServiceBindingStatus
Copy link
Contributor

Choose a reason for hiding this comment

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

ServiceBindingStatus defines the observed state of ServiceBindingStatus

you have a loop here :) It should be ServiceBindingStatus defines the observed state of ServiceBinding

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha, done, here you go

pkg/apis/apps/v1alpha1/servicebinding_types.go Outdated Show resolved Hide resolved
Comment on lines +56 to +58
metav1.GroupVersionKind `json:",inline"`
corev1.LocalObjectReference `json:",inline"`
EnvVarPrefix *string `json:"envVarPrefix,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

could you provide description for these fields? Could we replace metav1.GroupVersionKind and corev1.LocalObjectReference with corev1.ObjectReference perhaps? That would open up a possibility to refer Services in a different namespace, if needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we could. @DhritiShikhar provided similar feedback 👍 .

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, Dhriti had a slightly different feedback.
#495 (comment)

Copy link
Member Author

@sbose78 sbose78 May 27, 2020

Choose a reason for hiding this comment

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

Made an attempt to use corev1.ObjectReference but I see all the important fields ( Kind, Name, APIVersion) are optional.

The only option I see is to use the metav1.TypeMeta in combination with name/corev1.LocalObjectReference.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for metav1.TypeMeta as it has APIVersion instead of separate group and version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider this as well: kubernetes/community#4705.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would we use

  • Group, Version, Resource for an application but
    *APIVersion & Kind for a service ?

@redhat-developer/service-binding Could you please chip in?

Please consider this as well: kubernetes/community#4705.

@isutton Would do you have a specific recommendation with regards to this ?

Copy link
Contributor

Choose a reason for hiding this comment

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

in role.yaml we define the resources on which we can perform the CRUD actions, a GVK is not specified. Since we edit/update the deployment with the envFrom: secretRef, we should use GVR schema when referring applications instead of GVK, IMO. From services we just collect information, not perform any update on it.

@@ -50,7 +50,7 @@ func TestApplicationSelectorByName(t *testing.T) {
backingServiceResourceRef := "backingServiceRef"
applicationResourceRef := "applicationRef"
f := mocks.NewFake(t, reconcilerNs)
f.AddMockedUnstructuredServiceBindingRequest(reconcilerName, backingServiceResourceRef, applicationResourceRef, deploymentsGVR, nil)
f.AddMockedUnstructuredServiceBinding(reconcilerName, backingServiceResourceRef, applicationResourceRef, deploymentsGVR, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to log an issue for changes in variable names passed to mock functions. Not necessarily to be addressed in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Some of the variable names could remain, they might make things more expressive?

Secret corev1.LocalObjectReference `json:"secretRef"`
// ApplicationObjects contains all the application objects filtered by label
// +optional
Applications []BoundApplication `json:"applications,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we cannot bind to more than one application now, we should not have a list here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@DhritiShikhar it is possible to have more than one application even if you can only declare one entry in spec.application.

Application embeds metav1.LabelSelector as follows:

// Application defines the selector based on labels and GVR
type Application struct {
	// +optional
	LabelSelector               *metav1.LabelSelector `json:"labelSelector,omitempty"`
	metav1.GroupVersionResource `json:",inline"`
	corev1.LocalObjectReference `json:",inline,omitempty"`
}

It is then used in Binder.search() to list all resources matching the given GVR in the given namespace:

	objects, err := b.dynClient.Resource(gvr).Namespace(ns).List(opts)
	if err != nil {
		return nil, ApplicationNotFound
        }

This call to List() can return multiple resources if multiple resources exist with labels declared in spec.application.matchLabels.

@xliuxu
Copy link

xliuxu commented May 28, 2020

@sbose78 Sure. I will pick up the item changing Application to []Application

@@ -26,13 +26,13 @@ const (
// BindingFail binding has failed
BindingFail = "BindingFail"
//Finalizer annotation used in finalizer steps
Finalizer = "finalizer.servicebindingrequest.openshift.io"
Finalizer = "finalizer.servicebinding.openshift.io"
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on finalizer.servicebinding.openshift.io -> finalizer.servicebinding.operators.coreos.com?

Copy link
Member Author

Choose a reason for hiding this comment

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

Being fixed in 5f8ae1e , thanks.


// DataMapping is used to map CR/CSV/CRD attributes to Custom env variables
// +optional
DataMapping []corev1.EnvVar `json:"dataMapping,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Since DataMapping is a list, should it be plural (DataMappings)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we revisit this rename?

The reason for this is is the input is a map of environment variable names and templates and its output are rendered environment variables, which makes it much closer to customEnvVars than dataMapping.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! I don't mind revisiting the name

@@ -5,19 +5,19 @@ metadata:
alm-examples: |-
[
{
"apiVersion": "apps.openshift.io/v1alpha1",
Copy link
Member

Choose a reason for hiding this comment

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

Is the version going to remain as 0.0.23?

Copy link
Member Author

Choose a reason for hiding this comment

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

This, I think is the template, but the version probably isn't used anywherre.

"group": "apps",
"resource": "deployments",
"resourceRef": "nodejs-rest-http-crud",
"version": "v1"
},
"backingServiceSelector": {
"services": {
Copy link
Member

Choose a reason for hiding this comment

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

This should be updated to reflect that services is an array

Copy link
Member Author

@sbose78 sbose78 Jun 3, 2020

Choose a reason for hiding this comment

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

Fixed in 67cd32c

examples/java_postgresql_customvar/README.md Outdated Show resolved Hide resolved
examples/nodejs_postgresql/README.md Outdated Show resolved Hide resolved
@sbose78
Copy link
Member Author

sbose78 commented Jun 3, 2020

@pedjak Thanks for the review, I'll get to your review comment shortly after I'm done handling the API changes!

@openshift-ci-robot
Copy link
Collaborator

@sbose78: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link
Collaborator

@sbose78: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/4.5-unit 68a7dea link /test 4.5-unit
ci/prow/4.5-e2e 68a7dea link /test 4.5-e2e
ci/prow/4.5-lint 68a7dea link /test 4.5-lint
ci/prow/e2e 68a7dea link /test e2e
ci/prow/unit 68a7dea link /test unit
ci/prow/lint 68a7dea link /test lint

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@sbose78
Copy link
Member Author

sbose78 commented Jun 16, 2020

I've summarized the changes in CHANGES section which contains feedback from OLM team.

  • Set a Group and Kind which is Kubernetes / OperatorFramework friendly.
apiVersion: operators.coreos.com/v1alpha1
kind: ServiceBinding
  • Removed spec.backingServiceSelector
  • Replaced spec.applicationSelector with spec.application,
  • Using Application *Application instead of Application Application ( feedback by OLM team )
  • Using DetectBindingResources *bool instead of DetectBindingResources bool ( feedback by OLM team )
  • Replaced spec.backingServiceSelectors with spec.services
  • Replaced spec.customEnvVar with spec.dataMapping <--- we don't have agreement on this, we may keep this as is.
  • Replaced resourceRef in application and services to name corev1.LocalObjectReference
  • PENDING: The Application type has a label selector and a LocalObjectReference do I need to set a name and then use a label selector? I think that these should be a union type
  • PENDING: Change spec.application ( Application ) to spec.applications ( []Application ). To do so without breaking changes ini add extraFieldsModifier to binder #426 , some amount of refactoring is needed.

The objective of this PR was to gather feedback from internal/external stakeholders. The objective has been met and summarized here in this comment and the Description. I'm closing the PR and letting the team take up the work to implement these in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants