search

open-feature / open-feature-operator

A Kubernetes feature flag operator
https://openfeature.dev
Apache License 2.0
164 stars 33 forks source link

feat: Add image pull secrets #655

Closed cpitstick-latai closed 1 month ago

cpitstick-latai commented 1 month ago

DISCLAIMER: THIS DOES NOT BUILD AND I DO NOT KNOW WHY.

Tested with make helm-package and get this error:

Error: trouble configuring builtin PatchStrategicMergeTransformer with config: `
paths:
- exclude-ns.yaml
- manager.yaml
- exclude-validatingwebhook.yaml
`: MalformedYAMLError: yaml: line 9: did not find expected node content in File: manager.yaml
make: *** [Makefile:157: release-manifests] Error 1
toddbaert commented 1 month ago

I think I see the problem, I'll give a fix and shot and push up my change. Thanks for this!

toddbaert commented 1 month ago

@cpitstick-latai I pushed some updates to your branch, I hope you don't mind (I had to sign off your commit).

In addition, I added a single pullSecret, used by both the templates at the runtime components (we have a few runtime components that pull images). As we discussed, we can't in any clean way support a normal array of imagePullSecrets but I think this should cover most use-cases.

I tested this locally (setting and not setting it) and it works as expected (conveniently, an empty imagePullSecret is completely ignored.

cpitstick-latai commented 1 month ago

This is great. Feel free to take this over and submit it!

cpitstick-latai commented 1 month ago

One request I'll have is to add supporting the array of imagePullSecrets to the backlog for OpenFeature. Inevitably the build process will change and evolve, and when it does, this use-case will just become part of it.

Kavindu-Dodan commented 1 month ago

One request I'll have is to add supporting the array of imagePullSecrets to the backlog for OpenFeature. Inevitably the build process will change and evolve, and when it does, this use-case will just become part of it.

@toddbaert should we work on this now 🤔 Given the limitation on helm operators in kuztomize layer, a quick solution would be to rename imagePullSecret to imagePullSecretNames where imagePullSecretNames can caontain a comma separated names to be input in the secret array.

Otherwise latest change looks good

toddbaert commented 1 month ago

One request I'll have is to add supporting the array of imagePullSecrets to the backlog for OpenFeature. Inevitably the build process will change and evolve, and when it does, this use-case will just become part of it.

@toddbaert should we work on this now 🤔 Given the limitation on helm operators in kuztomize layer, a quick solution would be to rename imagePullSecret to imagePullSecretNames where imagePullSecretNames can caontain a comma separated names to be input in the secret array.

Otherwise latest change looks good

I wouldn't be opposed to that, but I think long term, a solution to the kustomize+helm mess is fundamentally in order. If you want to push the comma-separated solution onto this that's fine as well, it wouldn't take too much, but in a sense it's still a bit hacky since it's not really how to appropriately represent arrays in yaml.

Not sure what @cpitstick-latai thinks.

cpitstick-latai commented 1 month ago

@toddbaert As to the idea for an imagePullSecrets comma-separated array, I would rather have the enhanced functionality than more limited, even if hacky. We're already arguing over two separate hacks due to the way this build system is broken. So, as long as it's well documented and works properly with 0-to-n secrets, I'm OK with that solution.

But you should additionally create a backlog Epic to revisit the kustomize-helm build system, as I doubt this is the last time that will become a pain point!

toddbaert commented 1 month ago

One request I'll have is to add supporting the array of imagePullSecrets to the backlog for OpenFeature. Inevitably the build process will change and evolve, and when it does, this use-case will just become part of it.

@toddbaert should we work on this now 🤔 Given the limitation on helm operators in kuztomize layer, a quick solution would be to rename imagePullSecret to imagePullSecretNames where imagePullSecretNames can caontain a comma separated names to be input in the secret array.

Otherwise latest change looks good

@Kavindu-Dodan and I discussed it a bit further and we don't think this is possible with the current system after all, since we could not handle the secrets used only in templates/manifests; we can only handle the runtime ones with this solution.

I will likely merge this as is and release, but in addition I've opened: https://github.com/open-feature/open-feature-operator/issues/665