Standardize CRD Spec

[moolen]

Request for comments

The idea is to use this draft PR for discussing only the CRD Spec mentioned in #47.

❗ Technology discussions are off-topic

CHANGELOG:

• rename backend to store
• remove inline backend from ExternalSecret
• rename externalSecretClassName to className
• integrate dataFrom into spec

TODO:

• how would one sync secrets within kubernetes (probably similar to #474 w/ service accounts)?

[moolen]

not sure if this is a good idea maybe we should mount this under k8s.io, like external-secrets.k8s.io

[Flydiverny]

external-secrets.k8s.io sounds nice 😏 unless someone gets unhappy with the use of *.k8s.io

[moolen]

I stumbled upon this slack thread in sig-architecture which points to the api-review process. From my understanding we really shouldn't use k8s.io. If we plan to move the effort towards a Kubernetes SIG we should plan to use x-k8s.io.

[dirtycajunrice]

I feel it is a disservice to generalize "many projects in the wild" and then almost line for line copy the itscontained/secret-manger crd with no attribution to the source.

[Flydiverny]

I feel it is a disservice to generalize "many projects in the wild" and then almost line for line copy the itscontained/secret-manger crd with no attribution to the source.

I think its fair to add a list of links to existing work which were used for inspiration.

[Flydiverny]

Some random thoughts 🙃

[Flydiverny]

external-secrets.k8s.io sounds nice 😏 unless someone gets unhappy with the use of *.k8s.io

[dirtycajunrice]

@Flydiverny I like the idea of a universal injector in addition to the secret sync to allow injection via multiple sources to runtime env or file, without the need of using (potentially) 3+ separate upstream injectors with varying installation and usage

[moolen]

[...] almost line for line copy the itscontained/secret-manger crd with no attribution to the source.

I added all projects directly involved with the subject. I don't know if it's just me, but this sounds very polemic to me. If you take a close look at the commit history you will see that the origins of the spec lie in KES. From there i tried to improve by eliminating the tedious elements, repetitions and added a bunch of items from my wishlist. Eventually i went through the itscontained spec and aligned the spec in regards to the naming (Backend vs. Store for example - which was initially a optional thing). @dirtycajunrice I never wanted to sell "your stuff" as mine or even had the intention to do so.

I pushed a bunch of changes that resulted from the discussion so far. I see a bunch of existing topics and would like to propose the following:

• move the injection (via Init Container / Volume frontend #81 here and here i think?) topic to the "backlog" (maybe a section in the spec?)

For the ongoing discussions (let me try to summarize this) we have:

• one backend vs. multiple backends per ExternalSecret
  • related: the controller className or controller field should be in ExternalSecretSpec vs. StoreSpec
• merging/patching existing secrets with targetRef here

[dirtycajunrice]

@moolen We are here collaborating together - OSS is built off of a foundation of improving existing ideas. I just think it all goes down the toilet when the chain of idea attribution is broken (why we put kes and cert-manager front and center as attribution on our readme). No harm no foul 🤓.

Injection does not fit into MVP but was stated just so it was kept in sight. Agree with backlog

[knelasevero]

Hey! So, to add to the discussion, also prefer one backend per ES, basically because of the same reasons already stated here.

If we backlog 'merging/patching existing secrets' what does that mean for the CRD? We foresee it somehow?

Right now at Container Solutions we are moving our ES solution forward in the direction of the CRD draft at least in the obviously good things like secret refreshing and multiple key values per secret (things that were on our backlog anyways). Even if we decide to move to a solution built from scratch later on, I think it is good so I have some people with momentum to move to the new effort. Anyway, we want to help wherever needed!

[Flydiverny]

I like the latest updates 👍

[RichiCoder1]

A message field with the stringified error would be great for diagnostics.

[mcavoyk]

Agree, any reason not to go with a condition array block here for readiness?

[moolen]

Sure, conditions would fit in here nicely!

What about:

status:
  # represents the current phase of secret sync:
  # * Pending | ES created, controller did not (yet) sync the ES or other dependencies are missing (e.g. secret store or configmap template)
  # * Syncing | ES is being actively synced according to spec
  # * Failing | Secret can not be synced, this might require user intervention
  # * Failed  | ES can not be synced right now and will not able to
  # * Completed | ES was synced successfully (one-time use only)
  phase: Syncing
  lastSyncTime: "2020-09-23T16:27:53Z"
  failureReason: "..."
  failureMessage: "..."
  
  conditions:
  - type: InSync
    status: "True" # False if last sync was not successful
    reason: "SecretSynced"
    message: "Secret was synced"
    lastTransitionTime: "2019-08-12T12:33:02Z"
    lastHeartbeatTime: "2020-09-23T16:27:53Z"

[mcavoyk]

Looks good, a few comments

• lastHeartbeatTime doesnt need to be used here, that is generally only for conditions which need explicit heart beats
• failureReason, failureMessage already fit within the condition, those would better fit with events in this case rather than explicitly in the base status.

[moolen]

Regarding lastHeartbeatTime: i think this is the case here. The InSync condition should be updated with every sync. We should consider renaming this to lastSyncTime.

failure* as attributes are duplicates, true. Ill remove them.

[Flydiverny]

In #413 there's a PR to recreate the target resource / secret if its deleted. Is this something we should cover here? I think it makes sense and adds a behaviour similar to deployments and pods.

[moolen]

@Flydiverny i added a behavior section for the ExternalSecret resource. If i understand correctly, the underlying issue there is rate limiting of the AWS parameter store - this is a really important thing to note!

[Flydiverny]

@Flydiverny i added a behavior section for the ExternalSecret resource. If i understand correctly, the underlying issue there is rate limiting of the AWS parameter store - this is a really important thing to note!

Yepp, thats one of the underlying issues people have. Sometimes people also want to just trigger a refresh manually, especially interesting when polling is disabled. I guess in the end it boils down to not being able to have "live" updates due to API rate limit/cost or implementation limits.

[moolen]

First of all i'd like to thank all of you for your feedback, comments and effort you brought into this. It's good to see that here are a lot of people that want to drive this forward. Thank you!

I think this CRD spec is good enough to be implemented in a PoC. I'd like to leave this PR open for now, i don't think there's a lot of value having this CRD Spec in the repo 😅. Let's continue the discussion about merging efforts in #47.

[jonatasbaldin]

Hey peeps, I'm implementing the CRDs to bootstrap the project and got a question regarding the SecretStore authentication.

The Container Solutions project uses:

...
kind: SecretStore
...
auth: 
  secretRef: 
    name: externalsecret-operator-credentials-asm

Where, inside the externalsecret-operator-credentials-asm Secret, we have the following data:

apiVersion: v1
kind: Secret
metadata:
  name: credentials-asm
  labels:
    type: asm
type: Opaque
stringData:
  credentials.json: |-
    {
      "accessKeyID": "",
      "secretAccessKey": "",
      "sessionToken": ""
    }

Each Store backend unmarshals the JSON data (with the hardcoded credentials.json), verifiy the fields and authenticates (or fails to).

The iscontained project uses:

A specific struct for each Store backend, with the required fields, like:

type AWSAuth struct {
	AccessKeyID *smmeta.SecretKeySelector `json:"accessKeyID,omitempty"`
	SecretAccessKey *smmeta.SecretKeySelector `json:"secretAccessKey,omitempty"`
	Role *smmeta.SecretKeySelector `json:"role,omitempty"`
}

This is possible because there's no type field in the CRD, each Store backend has its own struct under SecretStore.spec, like SecretStore.spec.aws.

---

I'd like to have inputs on what you think is the best approach.

I believe the iscontained approach could be better:

• The end user doesn't need to understand why there's a Secret with a credentials.json key inside
• The CRD would tell exactly which information is required for authentication, without needing to unmarshal/validate a different JSON content for each Store backend
• On top of this, I think using the SecretStore.spec.aws approach could be better than having a SecretStore.spec.type, since the authentication fields are different for each type. This requires a change to the CRD every time a new Store backend is added, but the code would have better validation and be more explicit

Any thoughts?

[Flydiverny]

Wouldn't it be possible even with the type field to have an type specific auth struct?
Losing the added credentials.json sounds nice tho :)
The was some discussion on types vs different keys here #477 (comment)

[jonatasbaldin]

@Flydiverny

With the defined CRD spec:

...
spec:
  store:
    type: vault
    parameters: # provider specific k/v pairs
      server: "https://vault.example.com"
      path: path/on/vault/store
    auth: {} # provider specific k/v pairs

We need to have the following:

• type would be a string field
• parameters would be a runtime.RawExtension to allow different parameters for different Store backends, not allowing any validation on the CRD level
• auth would be a Secret reference with different fields for each Store backend (like accessKey/secretKey (AWS), projectID (GCP) etc)

The main problem becomes the lack of validation on the CRD level and the need to implement logic on each backend to decode the Secret reference and find the specific key/value pairs for its authentication.

---

If we implement like iscontained is doing:

...
spec:
  aws:
    region: ...

    authSecretRef:
      accessKeyID:
        key: access-key
        name: aws-key
      secretAccessKey:
        key: secret-key
        name: aws-key

We have:

• Validation at the CRD level for each Store backend, so we can easily prevent the creation of a resource without the required keys
• Easily link the authentication data to a Secret reference
• Keep each Store backend code specific and more explicit

---

Even though we would need to add new types inside the CRD each type a new Store backend is implemented, I believe the overall architecture/code base would be more explicit and clean than requiring each backend to decode an interface when authenticating.

What do you think? @Flydiverny @moolen @knelasevero

[moolen]

@jonatasbaldin

If we implement like iscontained is doing: [...]
i totally agree!

I would like to move this CRD Spec document to the (yet to be created) v2 Repository. This way it should be easier to propose changes.
Ack?

[jonatasbaldin]

@moolen yeah, let's do it!

We can work on the external-secrets repository for now, I think it would be easier than creating a new empty branch here. I should push the bootstrap project 'til end of the week, I can also attach the CRD draft there :)

[Flydiverny]

Could have a separate repo for just the specs as well if we want?

[jonatasbaldin]

Could have a separate repo for just the specs as well if we want?

Yeah, 💯 !

[jonatasbaldin]

Should we have a default value for refreshInterval? This can be a sensitive field, since it might make users reach their provider API limits.

If not, we need to mark this as required (and might give some advice on how to estimate it).

[knelasevero]

Default reconcile loops happen between 10h (if we don't change anything to the generated stuff), right?

[moolen]

I moved the CRD Spec to a dedicated repository. I fixed the store / provider naming. What's not yet addressed:

• [1] type & runtime.RawExtension vs. typed provider Standardize CRD Spec #477 (comment)
• [2] default value for refreshInterval Standardize CRD Spec #477 (comment)

Let's move discussion over to the new repo.

[moolen]

Lemme close this here, see you on the other side 👋