Support structured config #516

ljtfreitas · 2023-11-04T20:23:09Z

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

This PR proposes two approaches to support structured config:

makes the `config` field support a JSON as value

It could be useful to keep compatibility with older versions. Declaring the config field as a map[string]apiextensionsv1.JSON, existing support to inline fields keeps working (as aws:region: whatever) and expanding the config to a more complex object also works.

To make this work with Pulumi Automation API, in case the value is a json, the same rules used by Pulumi CLI to handle structured configs are applied. The implementation flattens all object keys to generate Pulumi-CLI compatible key names. For example:

config:
  my-key: my-value
  my-list:
    - a
    - b
    - c
  aws:region: us-east-1
  aws:assumeRole:
     roleArn: my-role-arn
     sessionName: my-assumed-session-name

it will generate these config keys/values:

my-key: my-value
my-list[0]: a
my-list[1]: b
my-list[2]: c
aws:region: us-east-1
aws:assumeRole.roleArn: my-role-arn
aws:assumeRole.sessionName: my-assumed-session-name

and all those keys are send through Automation API as Path: true

adds a new field `configRefs`

adds to CRD a new field called configRefs, that works in a similar way that EnvRefs or SecretRefs, adding two new possible values: configmap and structured

configmap adds support to read a K8s ConfigMap and get a specific key to use as a config value
structured is a JSON value that can be used to pass a structured configuration, following the same previous pattern from the config field.

integration tests

I was not able to make the tests pass in my local environment but right now it's just an initial implementation proposal. I can keep working on that if you people think is worth it.

Related issues (optional)

#258

github-actions · 2023-11-04T20:23:25Z

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

rquitales · 2023-11-08T18:47:22Z

/run-acceptance-tests

github-actions · 2023-11-08T18:47:45Z

Please view the PR build - https://github.com/pulumi/pulumi-kubernetes-operator/actions/runs/6802683612

Sergey-Kizimov · 2023-12-09T02:44:34Z

Any updates? it would help us if this feature will be implemented.

ljtfreitas · 2023-12-14T13:34:24Z

Hi friends, any news about it?

rquitales · 2023-12-20T16:06:50Z

pkg/controller/stack/stack_config.go

+		return nil, err
+	}
+
+	configValues := make([]ConfigKeyValue, 0)


nit: I since we're already using make(), let's provide a capacity, eg. make([]ConfigKeyValue, 0, len(flatten))

rquitales · 2023-12-20T16:12:18Z

test/stale_state_test.go

-			"password":   randString(),
-			"port":       rabbitPort,
-			"secretName": credsSecretName,
+		setupStack.Spec.Config = map[string]v1.JSON{


It would be nice to see some tests that exercise a structured config as well.

rquitales · 2023-12-20T16:28:59Z

pkg/apis/pulumi/shared/stack_types.go

@@ -45,7 +45,7 @@ type StackSpec struct {
 	Stack string `json:"stack"`
 	// (optional) Config is the configuration for this stack, which can be optionally specified inline. If this
 	// is omitted, configuration is assumed to be checked in and taken from the source repository.
-	Config map[string]string `json:"config,omitempty"`
+	Config map[string]apiextensionsv1.JSON `json:"config,omitempty"`


I'm somewhat apprehensive of switching this type even though existing strings should be compatible with this. Is it possible to maintain the map[string]string type, and do custom parsing/marshaling ourselves instead? I understand that this might make it slightly more difficult in terms of defining the structured config as it now has to be a scalar/string literal json representation rather than a yaml object I believe. WDYT?

rquitales · 2023-12-20T16:35:13Z

Hi friends, any news about it?

@ljtfreitas I apologize for the delay in reviewing this. Unfortunately, it fell off my radar and got lost in my GitHub notifications. I've provided some feedback, and overall, I believe this is a promising direction that we should push to completion soon to resolve a crucial user journey. My main concern lies with the type change and its potential downstream implications. As a user of this feature, what are your thoughts on keeping the type as a string? The structured config could still be represented as a string literal of the desired JSON configuration. While this may introduce some additional friction, I am inclined to maintain consistency with our CRD spec. WDYT?

ljtfreitas · 2023-12-21T14:25:54Z

Hi friends, any news about it?

@ljtfreitas I apologize for the delay in reviewing this. Unfortunately, it fell off my radar and got lost in my GitHub notifications. I've provided some feedback, and overall, I believe this is a promising direction that we should push to completion soon to resolve a crucial user journey. My main concern lies with the type change and its potential downstream implications. As a user of this feature, what are your thoughts on keeping the type as a string? The structured config could still be represented as a string literal of the desired JSON configuration. While this may introduce some additional friction, I am inclined to maintain consistency with our CRD spec. WDYT?

Hi @rquitales , don't worry.

About the question, yes, moving from string to map[string]apiextensionsv1.JSON is a breaking change, no doubt about it. I changed it as a potential idea but we need to think carefully about it. In other hand, a string field does not have a good ergonomic, I guess...we could put a json content, yes, but the program code would be required to handle it. Using the native config format from Pulumi just sounds more natural and ergonomic.

Maybe the approach using a new field, which I've suggested as configRefs, can be safer. config could stay untouched and more complex configurations could use the new field. An example:

apiVersion: pulumi.com/v1
kind: Stack
metadata:
  name: my-stack
spec:
    configRefs:
        aws:
            type: structured
            value:
              region: us-east-1
              assumeRole:
                roleArn: my-role-arn
                sessionName: my-assumed-session-name
        my-key:
          type: literal
          value: my-value
        my-list:
          type: structured
          value: [a, b, c]

The current implementation of this pull request would generate these config values:

my-key: my-value
my-list[0]: a
my-list[1]: b
my-list[2]: c
aws:region: us-east-1
aws:assumeRole.roleArn: my-role-arn
aws:assumeRole.sessionName: my-assumed-session-name

(actually I need to write a test to be sure about it 😅. but it's the idea).

Looking now the configRefs looks more complex than I've thought. The potential advantage here is be able to use env vars and secrets as config values (and I've introduced a new ConfigMap support).

WDYT? Maybe introducing a new field could be a better path?

EronWright · 2024-01-02T18:14:03Z

Regarding the API changes, I believe we need to ensure that we're following the rules of CRD schemas (ref). In particular, the list of configuration properties should be expressed as a list (with a merge key), not as a map.

ljtfreitas · 2024-01-05T20:31:19Z

Regarding the API changes, I believe we need to ensure that we're following the rules of CRD schemas (ref). In particular, the list of configuration properties should be expressed as a list (with a merge key), not as a map.

Sorry, not sure if I understood @EronWright . I don't get how passing config as a map would violate CRD rules.

Anyway, sounds better to create a new field for structured config in order to don't break the current API. I can keep working on that (my second suggestion is about it) and refining the implementation, if you folks agree.

github-actions · 2024-04-24T00:31:20Z

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions · 2024-04-24T00:34:23Z

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions · 2024-04-25T14:10:06Z

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions · 2024-04-25T14:30:29Z

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions · 2024-04-25T14:41:16Z

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

github-actions · 2024-04-25T14:44:31Z

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

ljtfreitas · 2024-04-25T17:19:28Z

Hi @rquitales @EronWright , sorry for the delay. I rethink some things about the concerns and opened a new pull request: #575

EronWright requested review from EronWright and rquitales November 6, 2023 17:28

rquitales reviewed Dec 20, 2023

View reviewed changes

ljtfreitas added 3 commits April 23, 2024 21:34

Initial implementation to support structured config

20eb3c7

Initial implementation to support ConfigRefs

d62e4b0

Reverting changes about Config; just ConfigRefs will be a complex config

d637e40

ljtfreitas force-pushed the support-structured-config branch from c78472f to d637e40 Compare April 24, 2024 00:34

ljtfreitas added 2 commits April 24, 2024 15:15

Trying to make tests pass...

8c8d103

Refactoring StructuredConfig + controller's support to ConfigRefs tests

79cadf2

ljtfreitas force-pushed the support-structured-config branch from a25d8d6 to 689af8d Compare April 25, 2024 14:40

Fixing some descriptions on CRD config fields

483bec5

ljtfreitas force-pushed the support-structured-config branch from 689af8d to 483bec5 Compare April 25, 2024 14:44

ljtfreitas closed this Apr 25, 2024

ljtfreitas mentioned this pull request Apr 25, 2024

Support structured config #575

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support structured config #516

Support structured config #516

ljtfreitas commented Nov 4, 2023 •

edited

Loading

github-actions bot commented Nov 4, 2023

rquitales commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

Sergey-Kizimov commented Dec 9, 2023

ljtfreitas commented Dec 14, 2023

rquitales Dec 20, 2023

rquitales Dec 20, 2023

rquitales Dec 20, 2023

rquitales commented Dec 20, 2023

ljtfreitas commented Dec 21, 2023 •

edited

Loading

EronWright commented Jan 2, 2024

ljtfreitas commented Jan 5, 2024

github-actions bot commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

github-actions bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

ljtfreitas commented Apr 25, 2024

Support structured config #516

Support structured config #516

Conversation

ljtfreitas commented Nov 4, 2023 • edited Loading

Context

Proposed changes

makes the config field support a JSON as value

adds a new field configRefs

integration tests

Related issues (optional)

github-actions bot commented Nov 4, 2023

rquitales commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

Sergey-Kizimov commented Dec 9, 2023

ljtfreitas commented Dec 14, 2023

rquitales Dec 20, 2023

Choose a reason for hiding this comment

rquitales Dec 20, 2023

Choose a reason for hiding this comment

rquitales Dec 20, 2023

Choose a reason for hiding this comment

rquitales commented Dec 20, 2023

ljtfreitas commented Dec 21, 2023 • edited Loading

EronWright commented Jan 2, 2024

ljtfreitas commented Jan 5, 2024

github-actions bot commented Apr 24, 2024

github-actions bot commented Apr 24, 2024

github-actions bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

ljtfreitas commented Apr 25, 2024

ljtfreitas commented Nov 4, 2023 •

edited

Loading

makes the `config` field support a JSON as value

adds a new field `configRefs`

ljtfreitas commented Dec 21, 2023 •

edited

Loading