Secret Generation Strategies extension issues

[baijum]

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. redhat-developer/service-binding-operator#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]

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]

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. redhat-developer/service-binding-operator#826

---

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

[baijum]

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]

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

+1

[sbose78]

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]

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]

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 Binding Secret Generation Extensions #156, the "Binding Secret Generation Strategies" extension could become another spec by itself. We could bring this new spec under the same Kubernetes SIG.