search

servicebinding / spec

Specification for binding services to k8s workloads
https://servicebinding.io
Apache License 2.0
92 stars 35 forks source link

Secret Generation Strategies extension issues #158

Open baijum opened 3 years ago

baijum commented 3 years ago

1. Syntax difference between annotation and x-descriptors

There are different syntaxes for annotation and x-descriptors. Using a common syntax for both annotation and x-descriptors would be better for consistency. AFAIK, x-descriptors don't demand a URN. Also, I don't see a need for a URN in the x-descriptors. I can't find the meaning of the NID ("alm") used in the URN. Also, some parts of NSS (":descriptor:io.kubernetes") doesn't have an explanation. The Second line in x-descriptors doesn't conform to URN syntax ("service.binding").

2. The path used in a CSV is not a JSON Path

(e.g., "path: data.dbCredentials")

Rather, it is a dot-separated string pointing to an attribute in another Kubernetes resource.

Update: It was already reported before: #111

3. There is no normative text (RFC2119 is not followed) used in the extension.

4. The order of lookup to extract value is not defined.

(This may not be required based on the outcome of proposal #156) The order of precedence to decide which resource should be used to extract the value of the secret is undefined. For example, it could be in this order:

  1. Provisioned Service
  2. CR annotation
  3. CRD annotation
  4. X-Descriptors

5. Allowed charters for annotation sourceKey value is not defined.

Probably use the definition from here: https://kubernetes.io/docs/concepts/configuration/secret/#overview-of-secrets

The keys of data and stringData must consist of alphanumeric characters, -, _ or .

A similar definition is there in ConfigMap: https://kubernetes.io/docs/concepts/configuration/configmap/#configmap-object

Each key under the data or the binaryData field must consist of alphanumeric characters, -, _ or .

Ref. https://github.com/redhat-developer/service-binding-operator/issues/826

6. Enum values for annotation objectType (Secret & ConfigMap) is not defined.

7. The API group name should not be used as an annotation key

Currently, API group name is used as annotation key in this scenario: "In Mount an entire Secret as the binding Secret"

“service.binding":
  ”path={.status.data.dbCredentials},objectType=Secret”

Instead of service.binding there should be some suffix. For example, in the above case, the key could be service.binding/whole-secret

Probably a subdomain would be better than the primary domain. secret-generation.service.binding/whole-secret

8. Considerations for Role-Based Access Control (RBAC) are not documented.

The Secret Generation extension assumes the OLM descriptors, CRD annotations, and custom resource annotation are in the same namespace. Sometimes these resources may not be in the same namespace, and sometimes not in the same Kubernetes cluster (e.g., an external service). An RBAC rule to access these resources is cumbersome in many organizations. A similar problem exists in a typical Provisioned Service where it is limited to that particular custom resource. A Direct Secret Reference would be more appropriate in these scenarios.

9. type and provider entries are not explicitly specified

The type entry should be a mandate and the provider entry should be a recommendation. This is required for languages interface to discover configuration.

baijum commented 3 years ago

If we cannot fix these issues before GA, I would propose removing the "Secret Generation Strategies" extension from the spec. We can add it back in a future release after addressing these issues.

baijum commented 3 years ago

Allowed charters for annotation sourceKey value should be defined.

Probably use the definition from here: https://kubernetes.io/docs/concepts/configuration/secret/#overview-of-secrets

The keys of data and stringData must consist of alphanumeric characters, -, _ or .

A similar definition is there in ConfigMap: https://kubernetes.io/docs/concepts/configuration/configmap/#configmap-object

Each key under the data or the binaryData field must consist of alphanumeric characters, -, _ or .

Ref. https://github.com/redhat-developer/service-binding-operator/issues/826

Enum values for annotation objectType (Secret & ConfigMap) should be defined.

baijum commented 3 years ago

If we cannot fix these issues before GA, I would propose removing the "Secret Generation Strategies" extension from the spec. We can add it back in a future release after addressing these issues.

Along with issue #156, the "Binding Secret Generation Strategies" extension could become another spec by itself.

sbose78 commented 3 years ago

Using a common syntax for both annotation and x-descriptors would be better for consistency

+1

sbose78 commented 3 years ago

The path used in a CSV (e.g., "path: data.dbCredentials") is not a JSON Path. Rather, it is a dot-separated string pointing to an attribute in another Kubernetes resource.

I'm good if we wish to propose something more "standard". This was done for the sake of simplicity.

pedjak commented 3 years ago

If we cannot fix these issues before GA, I would propose removing the "Secret Generation Strategies" extension from the spec. We can add it back in a future release after addressing these issues.

Secret Generation Strategies is the extension even today, hence it does not block us to GA the core spec?

baijum commented 3 years ago

If we cannot fix these issues before GA, I would propose removing the "Secret Generation Strategies" extension from the spec. We can add it back in a future release after addressing these issues.

Secret Generation Strategies is the extension even today, hence it does not block us to GA the core spec?

  1. GA is not just for the core spec; rather, it's for the whole spec.
  2. An extension with lots of issues would be difficult to correct later.
  3. As I said earlier, along with issue #156, the "Binding Secret Generation Strategies" extension could become another spec by itself. We could bring this new spec under the same Kubernetes SIG.