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

Add Design for Dynamic export of ConfigMap/Secret values #4148

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

matthchr
Copy link
Member

@matthchr matthchr commented Jul 2, 2024

Related to #2555.

If applicable:

  • this PR contains documentation
  • this PR contains tests
  • this PR contains YAML Samples

@matthchr matthchr added this to the v2.9.0 milestone Jul 2, 2024
@matthchr matthchr self-assigned this Jul 2, 2024
@matthchr matthchr changed the title Add ADR for Dynamic export of ConfigMap/Secret values Add Design for Dynamic export of ConfigMap/Secret values Jul 2, 2024
@matthchr matthchr force-pushed the feature/dynamic-export branch from 116952d to 961e24b Compare July 3, 2024 00:25
Comment on lines +310 to +311
Getting access to data to export to ConfigMaps is pretty easy, we'll just pass the whole `resource` to
the expression. As shown elsewhere fields will be accessible in expressions like `resource.status.id`.
Copy link
Member

Choose a reason for hiding this comment

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

I think it may be better to pass in spec and status as the two available variables, since everyone will need to index into one or the other first. This would allow us to also add secrets as a third variable, which is only included if secrets. exists as a substring in the expression.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's also metadata though so I think just passing the whole top level object might be cleaner?

Copy link
Member

Choose a reason for hiding this comment

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

I think the tension here is between making things possible and making common things easy.

Passing just the top level object makes things possible but it forces every usage to incur a little bit of overhead. Passing multiple objects is more complex to implement, but has the benefit of making most usage simpler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Still not sure I agree. resource.spec.whatever isn't much more complicated than spec.whatever.

The advantages to us using top level names we choose are:

  1. It disambiguates between the resource and its secrets, since for secret usage we'll support secrets.myKey. If we control the top-level name of all the inputs (the main 2 right now being resource and secrets), then we can ensure there are no collisions because we own the top of the path for each input. If we instead snip that part out (for just resource? or for secrets too?) then we run the risk (though it's small) that a top level field such as secrets collides.
  2. It puts us in a better position to grow in the future by adding other top-level types going forward, such as operator (holding operator configuration?) or cluster (holding cluster details?). I don't know if we're ever going to want to add these but if we did, disambiguating between them at the top level helps with clarity.
  3. It means we're consistent between resource.x and secrets.y in terms of the pattern, as opposed to doing x and secrets.y. I think including secrets as a prefix for secret values which are not on the resource is very useful from a security/safety point of view because we're making it really obvious the value you're about to output is a secret. Obviously we only support this in the context of saving to secrets so it's not like there's risk of exposing a secret value as plaintext but still the clarity seems a win for something sensitive like secrets.

I'll update the document to discuss this and leave it as an option question at the end of the document.

Comment on lines 338 to 339
This interface must be implemented manually alongside the manual implementation of the
`KubernetesExporter` resource.
Copy link
Member

Choose a reason for hiding this comment

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

If it must be implemented alongside - that is, if it's mandatory - why not extend the existing KubernetesExporter interface with a second method, to force this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not every KubernetesExporter produces secrets. If all it produces is configmaps we don't need to have the secret specific one

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've clarified this wording some and pointed out we could add the method to the existing interface, but I think a separate interface will ultimately be cleaner. Not married to making a decision here though we can also discuss during implementation.

value: "'%s:%s'.format([secret.hostName, secret.sslPort])" # Value (format) of the secret
```

**ConfigMaps**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would we be able to export collections? Most preferably Maps since we don't support exporting slices yet.

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, though the user might need to write a CEL expression to explain what exactly they want to gather from the map

Copy link
Member Author

@matthchr matthchr Jul 11, 2024

Choose a reason for hiding this comment

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

I will update the documentation here to be explicit about how this might look in an actual CEL expression, but it is doable for both maps and arrays

Copy link
Member Author

@matthchr matthchr Jul 11, 2024

Choose a reason for hiding this comment

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

Updated the FAQ documentation. It now says:

Q: Can this be used to export a particular item from an arbitrary array or map?

A: Yes! The syntax for maps is: request.myMap['hello'], and for arrays request.myArray[0]. CEL has a concept of
macros and there are some useful ones such as filter, exists, and all
built in, so you can craft more complex
expressions like request.slice.exists_one(i, i.name=='foo') ? request.slice.filter(i, i.name == 'foo')[0].value : 0
which checks if the slice contains exactly one element whose name is foo and if so, gets that elements value,
otherwise returns 0. A similar construct can be done for maps using the same exists_one and filter macros.

Copy link
Member Author

@matthchr matthchr Jul 11, 2024

Choose a reason for hiding this comment

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

Ah, but what about if you want the whole collection..., let me think on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok we can support whole collections too. Here's how:

Q: Can this be used to export entire collections of secret or configmap values?

A: Yes. We will support two flavors of entry into the dynamicSecrets and dynamicConfigMaps collections. One
flavor is the one we've seen above, where value is a CEL expression that returns a string and the name and key
fields specify which configMap or secret and what key within that to export.

The other kind of entry is:

 operatorSpec:
   dynamicConfigMaps:
     - name: account-data # Name of the destination configMap
       valueMap: "{"key1": "foo", "key2": "bar"}" # CEL expression that returns a map[string]string

The valueMap fields result can be directly saved as keys + values in the resulting configmap or secret, so the author
of the CEL expression has direct control over what values go into their secret/configMap.

Great question @super-harsh.

@matthchr matthchr force-pushed the feature/dynamic-export branch 2 times, most recently from cfcc024 to ba97db2 Compare July 11, 2024 17:58
@theunrepentantgeek
Copy link
Member

/ok-to-test sha=ba97db2

@matthchr matthchr force-pushed the feature/dynamic-export branch from ba97db2 to f201536 Compare July 12, 2024 18:30
@matthchr
Copy link
Member Author

/ok-to-test sha=f201536

@matthchr matthchr added this pull request to the merge queue Jul 12, 2024
Merged via the queue into Azure:main with commit 578753a Jul 12, 2024
8 checks passed
@matthchr matthchr deleted the feature/dynamic-export branch July 12, 2024 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants