-
Notifications
You must be signed in to change notification settings - Fork 57
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
Support structured config #575
base: master
Are you sure you want to change the base?
Conversation
PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. |
/run-acceptance-tests |
Please view the PR build - https://github.com/pulumi/pulumi-kubernetes-operator/actions/runs/8839237664 |
This is awesome, thank you for the contribution! One initial thought -- still need to take a deeper look.
This is a great idea. What do you think about using Pulumi's native marshalers? That would give you a little validation on top, and it might also make your flattening logic unnecessary (although I'm not 100%). The encoding package should do the trick, and you would unmarshal into a ProjectStack. |
@blampe Thanks. About your suggestion maybe I haven't expressed myself properly. My initial idea, and that's the implementation is actually doing, is to put in a ConfigMap just the However, it looks like a good idea to me 🤔 ; so we could move the whole stack file ( |
Ah, I had misunderstood what all was in the ConfigMap, I didn't realize it was just the config key. In that case you could marshal the config.Map instead of the whole ProjectStack as I had suggested earlier. I didn't mean to increase the scope of your change! |
@blampe Don't worry, maybe I haven't expressed myself properly in the PR description (English is not my first language so sometimes talking about complex stuff is just...complex 😅). About your suggestion, I'm trying to refactor the code to use
Looking at the code that deserializes a YAML to a ProjectStack instance (in the Automation API code), config keys are namespaced (with the project name) before the config.Map's unmarshal 😕 ...and I can't use the same function because it's private. So config keys must be namespaced in the ConfigMap. Not sure if we need to enforce that here because this behavior will be applied anyway during the config-set phase (and speaking for myself I don't namespace my configs because of this) |
hey friends, any news here? we really need this feature |
Sorry for the delay @ljtfreitas, I plan on playing around with this tomorrow! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies again for the delay. I was able to spend some hands-on time with this and have a much better feel for things.
The biggest issue I see is that I think we'll want ConfigRefs
to be an array. Let me know if you think that sounds reasonable.
I'm still playing around with some of the config merging logic locally, in case there's some room to simplify things.
@@ -0,0 +1,90 @@ | |||
module structured-config-refs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, did you include a Go program here because you're using Go for your own projects?
I'm a big fan of YAML projects for tests, but this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I was a bit biased here in fact 😅, I wrote the example/test program in Go because I wanted to be sure that we could deserialize a structured, complex config in the way that we usually do in Go
@@ -14,6 +15,7 @@ import ( | |||
|
|||
"github.com/pulumi/pulumi-kubernetes-operator/pkg/apis/pulumi/shared" | |||
pulumiv1 "github.com/pulumi/pulumi-kubernetes-operator/pkg/apis/pulumi/v1" | |||
"sigs.k8s.io/yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 We've had issues with marshaling when not using the k8s fork, so this is good.
deploy/crds/pulumi.com_stacks.yaml
Outdated
description: (optional) ConfigRefs is the configuration for this stack, | ||
which can be specified through ConfigRef. If this is omitted, configuration | ||
is assumed to be checked in and taken from the source repository. | ||
If present, ConfigRefs values will be merged with values passed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should clarify which one has precedence -- e.g. ConfigRefs will overwrite Configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but ConfigRefs will not overwrite Configs, right? in fact both fields will be merged
I guess the description could be improved to explain the ConfigRefs
will overwrite Configs
with the same key name
deploy/crds/pulumi.com_stacks.yaml
Outdated
additionalProperties: | ||
description: ConfigRef identifies a resource from which config information | ||
can be loaded. Environment variables, files on the filesystem, | ||
Kubernetes Secrets, ConfigMaps, structured and config literal | ||
values strings are currently supported. | ||
properties: | ||
configmap: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConfigRefs will need to be an array to allow supporting more than one ref with predictable precedence.
For example the user might want to specify more than one secret ref, and if each of those refs contains the same key, then the value from the last ref should win.
As a map, the user can't specify the same ref type more than once, and the map's iteration order isn't guaranteed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. It could be an array, but do we really need this? 🤔 ConfigRefs
would have the same semantics as EnvRefs
field.
In EnvRefs, one could specify more than one secret, or more than one env var, with the same key name and the order will not be guaranteed; but this would look like a misconfiguration to me (using the same secret key twice), the behavior seems reasonable.
also, the ConfigRefs
spec above supports to specify the same ref type more than once, like:
configRefs:
first-secret:
type: Secret
secret:
name: my-secret
key: key-name
second-secret:
type: Secret
secret:
name: my-another-secret
key: key-name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EnvRefs
makes sense as a map because the keys are just variable names, and I agree you would never need to specify the same variable twice.
ConfigRefs
is different because each ref can potentially be its own bag of key/values. All of those key/values need to be coalesced and we want that process to be well-defined.
also, the ConfigRefs spec above supports to specify the same ref type more than once, like:
Yes, you're right, I wasn't clear on the key when I wrote that. I'm still worried about the iteration order, though. Consider the case where you have some "base" config that you want individual stacks to be able to override:
configRefs:
base:
type: Structured
value:
my-config: base-value
override:
type: Structured
value:
my-config: override-value
I would expect the override value to always be applied, but that's not guaranteed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it makes sense. I will change it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about the best way to do it 🤔 ...maybe an "array of maps"?
configRefs:
- base:
type: Structured
value:
my-config: base-value
- override:
type: Structured
value:
my-config: override-value
Or an object with a "name" field (or "key" or something)?
configRefs:
- name: base
type: Structured
value:
my-config: base-value
- name: override
type: Structured
value:
my-config: override-value
I prefer the first one but any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blampe just added a new test case for this scenario
sort.Slice(configValues, func(i, j int) bool { | ||
return configValues[i].Key < configValues[j].Key | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to double check this, but I believe keys can be specified in flat or exploded form, so for example object.nums[0]: 1
and object: { nums: [1] }
. I think you already handle this by flattening, but it would be worth a test to verify that 2 refs specifying the same key in different ways will be merged together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the second example a valid syntax? As a json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just doubled checked -- these are two ways of describing the same thing:
stack:deepobject:
nums:
- 1
stack:flatobject: '{"nums": [1]}
It doesn't look like stack:object.nums[0]: 1
is valid.
I think it would be totally fine to say this is an edge case we don't handle, given that it's unlikely to come up.
@@ -205,6 +207,386 @@ var _ = Describe("Stack Controller", func() { | |||
}) | |||
}) | |||
|
|||
Context("configuring a stack using ConfigRefs", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you enormously for adding tests!
Stack: stackName, | ||
GitSource: &shared.GitSource{ | ||
ProjectRepo: baseDir, | ||
RepoDir: "test/testdata/config-refs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just test/testdata/structured-config-refs
for all of these? This is testing we can read the configs, seems worth also confirming the structure was handled correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this test is trying to read a file, and in a similar way that I implemented with Literal
, I used the same existing behavior to FileRef
(the operator already use it to load envvars and secrets from files), which means that just loads a single value. So this test couldn't use test/testdata/structured-config-refs
Do you believe that would be worth implementing a file
behavior specifically to load configs? So we could support structured values as well.
structuredConfig := StructuredConfig(configMapContent).Flatten() | ||
allConfigs = append(allConfigs, structuredConfig...) | ||
} | ||
case shared.ConfigResourceSelectorStructured: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused about the need for both "structured" and "literal".
The legacy resolveResourceRef
logic doesn't do any special handling for "literal", but we have an opportunity to be more flexible here.
In other words, it's odd to have two ways to express the same thing:
configRefs:
stack-config:
type: Literal
value: foo
--
configRefs:
stack-config:
type: Structured
value: foo
But this is net-new:
configRefs:
stack-config:
type: Structured
value:
my-config:
my-nested-field:
field: "value"
It would seem reasonable IMO to basically remove structured and just change how we handle literals (basically replacing this block with case ResourceSelectorLiteral
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this option first but the reason why I've preferred to introduce the "structured" type was don't change the "literal" behavior, because it's already used by EnvRefs
and SecretRefs
fields, and for these use cases the "literal" option looks good enough (specially for env var case)
For these use cases just passing a literal, simple string value makes sense, but a structured, complex value does not, I think...so I didn't want to change that, and introduced the new "structured" option (I used this word because Pulumi docs also use it to reference about complex configs. So the main reason was backward compatibility.
but you're right, it sounds a bit strange. maybe we could handle "literal" for configs as you suggested and also keep the legacy "literal" behavior for the other Refs
fields. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we don't want to change literal handling for EnvRefs
or SecretRefs
. (There's an argument that we should, but we don't need to think about that as part of this feature.)
but you're right, it sounds a bit strange. maybe we could handle "literal" for configs as you suggested and also keep the legacy "literal" behavior for the other Refs fields. WDYT?
Yeah, that's what I was thinking. Let EnvRefs
and SecretRefs
continue to use resolveResourceRef
, and you can use your structure handling for literals here.
thank you @blampe, don't worry, don't need to apologize, thank you for your support (i hope that I have not sounded like a jerk or something 😅 also), i will check the comments 👀 |
PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. |
hi friends 👋 , any news here? cc @blampe |
Hi @blampe, this feature is crucial for us, could you take a look when you get time, please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, have been OOO. I'm still a little uncertain about the type of ConfigRefs
, and I worry we're duplicating some of SecretRefs
. It would be nice to test merging behavior when keys collide.
Hi @blampe, this feature is crucial for us, could you take a look when you get time, please?
You should be able to use a custom image if this is blocking you.
allConfigs = append(allConfigs, configs...) | ||
} | ||
// Secret should be handled here as well because auto.ConfigValue should be marked as Secret:true | ||
case shared.ConfigResourceSelectorType(shared.ResourceSelectorSecret): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would I use this instead of the existing SecretRefs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original idea was to support the same options from ResourceRef
but I'm wondering about that...maybe should we support just literal/structured values and ConfigMap for configs?
sourceConfigMap := map[string]any{ | ||
"aws:assumeRole": map[string]any{ | ||
"roleArn": "my-role-arn", | ||
"sessionName": "my-session-name", | ||
}, | ||
"aws:defaultTags": map[string]any{ | ||
"tags": map[string]any{ | ||
"my-tag": "tag-value", | ||
}, | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what it's worth, I think if you add another key like "aws:assumeRole.roleArn": "other-role-arn"
this will fail non-deterministically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be but, sorry, is it a real problem? For a stack configured like this,
aws:assumeRole:
roleArn: "my-role-arn"
aws:assumeRole.roleArn: "other-role-arn"
I wouldn't expect any deterministic behavior here, it would look like a configuration mistake to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it may seem weird, but it's possible so we need to consider it. Hyrum's law.
It would be very helpful to see some more tests around what happens when multiple config maps are provided with overlapping keys like this.
for _, configRefMap := range sess.stack.ConfigRefs { | ||
for k, ref := range configRefMap { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear on why this is a []map[string]ConfigRef
instead of just a []ConfigRef
. The key only seems to be used for secret refs (which mirrors SecretRefs
?), and for the default case (do we need that?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a design decision, that I've mentioned here: #575 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've chosen the array of maps
option to be similar to the envRefs
and secretRefs
fields, where the key is the env/secret key name.
But in fact for structured/config map cases the key is not used 😕...not sure what could be a better option. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For literal values it's used as the config-key name:
configRefs:
- sample:
type: Literal
value: just-a-value
it will be resolved as a config like that:
sample: just-a-value
default: | ||
// try to resolve as a ResourceRef |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? resolveResourceRef
doesn't support structure so it seems odd to default to it. An error would be reasonable IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't, but resolveResourceRef
supports all other cases like env
, file
, etc. These options are supported in configRefs
too.
Maybe it does not make sense and configRefs
should support just structured/config map options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should keep the new API surface area as small as possible. Unless we have a good reason to include them now, we can omit env
, file
etc. and add them later if needed. We don't need a key k
for the literal case which leaves this default case as the only reason for it. Not needed IMO.
…nfigure Secrets through ConfigRefs
PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. |
hey @blampe , I've made some changes and updated the PR description; I've removed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can still simplify the API a bit. Let's try to scope this down to only the things we know we need, which it sounds like is config maps and literals?
Other things that will help get this over the line:
- Blocker: Resolve non-determinism by making ConfigRefs an array.
- Blocker: Tests around edge cases with merging multiple refs/literals.
- Optional: We could limit the change to an alpha API, to leave room for changes before we add it to v1.
If this is omitted, configuration is assumed to be checked in and | ||
taken from the source repository. If present, ConfigRefs values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean we over-write in-repo configs entirely? Why not overlay these configs on top of source?
It seems surprising to be able to commit some changes to config, deploy it, but then not see the new config take effect. Is this the existing behavior of the operator?
namespace: | ||
description: Namespace where the ConfigMap is stored. | ||
Deprecated; non-empty values will be considered invalid | ||
unless namespace isolation is disabled in the controller. | ||
type: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not add new things that are already deprecated.
namespace: | |
description: Namespace where the ConfigMap is stored. | |
Deprecated; non-empty values will be considered invalid | |
unless namespace isolation is disabled in the controller. | |
type: string |
description: Env selects an environment variable set on the | ||
operator process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this was copy-pasted, let's make sure the doc strings are accurate.
of selector. Must be one of: Env, FS, Secret, ConfigMap, | ||
Structured, Literal' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of selector. Must be one of: Env, FS, Secret, ConfigMap, | |
Structured, Literal' | |
of selector. Must be one of: Env, FS, ConfigMap, or Literal.' |
Or just ConfigMap/Literal if you scope it down to those two.
@@ -898,6 +975,83 @@ spec: | |||
which can be optionally specified inline. If this is omitted, configuration | |||
is assumed to be checked in and taken from the source repository. | |||
type: object | |||
configRefs: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was suggested that we keep this limited to an alpha version like this before adding it to v1. That way we aren't immediately locked in to the API.
@@ -1085,6 +1086,64 @@ func (sess *reconcileStackSession) SetEnvRefsForWorkspace(ctx context.Context, w | |||
return nil | |||
} | |||
|
|||
func (sess *reconcileStackSession) resolveConfigRefs(ctx context.Context) ([]ConfigKeyValue, error) { | |||
allConfigs := make([]ConfigKeyValue, 0) | |||
for _, configRefMap := range sess.stack.ConfigRefs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still non-deterministic, and the name is still unused. Please make this an array.
Value auto.ConfigValue | ||
} | ||
|
||
func NewStructuredConfigFromJSON(key string, rawValue apiextensionsv1.JSON) (*StructuredConfig, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want to accept YAML?
sourceConfigMap := map[string]any{ | ||
"aws:assumeRole": map[string]any{ | ||
"roleArn": "my-role-arn", | ||
"sessionName": "my-session-name", | ||
}, | ||
"aws:defaultTags": map[string]any{ | ||
"tags": map[string]any{ | ||
"my-tag": "tag-value", | ||
}, | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it may seem weird, but it's possible so we need to consider it. Hyrum's law.
It would be very helpful to see some more tests around what happens when multiple config maps are provided with overlapping keys like this.
literalRef := configRef.Literal | ||
if literalRef != nil { | ||
// ConfigLiteralRef handles both simple and structured values as json, flattening all keys to build a list of Pulumi key:value configs | ||
structuredConfig, err := NewStructuredConfigFromJSON(k, literalRef.Value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already mentioned above, but interpreting literal as an object eliminates the need for k
while still allowing structure.
literal:
value:
aws:region: us-west-2
foo:bar:
- baz
default: | ||
// try to resolve as a ResourceRef |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should keep the new API surface area as small as possible. Unless we have a good reason to include them now, we can omit env
, file
etc. and add them later if needed. We don't need a key k
for the literal case which leaves this default case as the only reason for it. Not needed IMO.
@ljtfreitas Are you still planning to move this PR forward and address the comments? |
Context
Currently structured configs are not supported properly by Pulumi k8s operator.
As Pulumi programs can be configured using YAML files, it looks fair to be able to configure a structured, complex config using the Stack CRD as well.
Proposed changes
The proposed approach is to introduce a new field called
configRefs
, rather than change the currentconfig
one.configRefs
has a format close to other fields likeenvRefs
andsecretsRefs
, supporting the sameenv
andfilesystem
options as well (secret
is not supported because just usesecretsRefs
looks better), and two new options:ConfigLiteralRef
ConfigLiteralRef refers to both a simple or possibly structured, nested value in YAML format, in the same way that people would put in regular
Pulumi.<stack>.yaml
files:The
value
content will be handled as a map of any type of value; so just simple key-pairs in YAML format are supported as well:ConfigMapRef
ConfigMapRef refers to a Kubernetes ConfigMap. The implementation assumes that the content from the ConfigMap's key is a structured, nested content in YAML format. The idea is to enable people to just put their configs from
Pulumi.<stack>.yaml
files in a ConfigMap, and the operator will be able to handle it.So given a ConfigMap called
my-stack-config
, like that:It could be consumed as a config to a Pulumi stack like that:
Configs merge strategy
All configs resolved from
configRefs
fields will be merged with the ones from theconfig
field.configRefs
resolved key-values will overrideconfig
ones with the same key.Configs
set
strategyAll configs will be set using the
path
flag. So theconfig
field will start to support nested values as well.It's especially useful for some complex fields from providers. An example:
Passing the value above with the
--path
flag, it will be configured like that in the result stack config:And simple key-value pairs keep working also.
Related issues (optional)
#258
#516