[#2756] Apply config through the airy controller #3176

ljupcovangelski · 2022-05-18T12:25:34Z

addresses #2756

A draft PR for proof of concept and also to get some feedback 🙏 .

The CLI returns this:

$ airy config apply
Configured component  Enabled
security              true
sources-facebook      true
sources-google        true
integration-webhook   true

The endpoint accepts json

{
  "security": {
    "allowedOrigins": "*",
    "jwtSecret": "secret",
    "systemToken": "test"
  },
  "components": {
    "sources": {
      "facebook": {
        "token1": "token",
        "token2": "token"
      }
    }
  }
}

and also the legacy yaml format of the airy.yaml file.

ljupcovangelski · 2022-05-18T12:26:43Z

lib/go/payloads/BUILD

@@ -1,15 +1,18 @@
 load("@com_github_airyhq_bazel_tools//lint:buildifier.bzl", "check_pkg")
 load("@io_bazel_rules_go//go:def.bzl", "go_library")
-# gazelle:prefix github.com/airyhq/airy/lib/go/httpclient/payloads
+# gazelle:prefix github.com/airyhq/airy/lib/go/payloads


Moved the payloads outside of the httpclient package, because they are also used by the controller.

ljupcovangelski · 2022-05-18T12:29:04Z

lib/go/config/airy_yaml.go

+type ConfigComponent struct {
+	Removed   bool   `json:"removed"`
+	Component string `json:"component"`
+}


This is currently only used by the ConfigApplyResponsePayload, so not sure if it should be here or in payloads. Also Removed is just to be in line with the current concept (when a configuration is applied without a component - that component is removed).

I'd be happy to remove this and add a separate endpoint for removing a component.

chrismatix · 2022-05-18T12:37:10Z

infrastructure/controller/pkg/endpoints/cluster_apply.go

+	namespace := "default"
+	responseConfigComponents := payloads.ConfigApplyResponsePayload{}
+
+	err = json.Unmarshal(body, &conf)


As mentioned I would suggest that we nest the config inside a data field. This will make the API more maintainable as we can add more options to the request body later when needed.

Maybe having everything inside components. Also if we add more options specially mandatory options. That will create a breaking change anyway. Not sure having a nested value will be necessary

chrismatix · 2022-05-18T12:39:30Z

infrastructure/controller/pkg/endpoints/cluster_apply.go

+	if len(secData) != 0 {
+		applyErr := k8s.ApplyConfigMap("security", namespace, secData, map[string]string{}, s.clientSet)
+		if applyErr != nil {
+			klog.Error("Unable to apply configuration for \"security\"\nError:\n" + err.Error())


In this case, the request should fail and terminate early (same below)

In this case, the request should fail and terminate early (same below)

I am not sure about this. The components are independent from each-other, so an error in deploying one component/configMap should not impact the ones that follow after.

We can return perhaps a list of failed components

It's true that we could do that but for clients, a completely failed state is easier to recover from than a state that is half failed.

What do you return for a request that only did 50% of what you asked it to? 200 aka "success"? In that case, clients may not be aware that anything failed at all

Yes, I get it 👍 We can return the failed-components in a separate list, but I think as it is very unlikely that only one configMap fails - I will set it to return a 500 - InternalServerError.
From the UI I assume it will mostly be used for applying a single component at a time anyway.

infrastructure/controller/pkg/endpoints/cluster_apply.go

chrismatix · 2022-05-21T17:19:07Z

docs/docs/api/endpoints/components.md

+
+```json
+{
+  "Security": {


Payload needs to follow our snake_case convention

Payload needs to follow our snake_case convention

Thanks 👍. I changed it for security and data. For the values which were in the airy.yaml (allowedOrigins, systemToken, jwtSecret, ...) I suggest we do it in a subsequent PR, as it has lots of changes across almost all components.

chrismatix · 2022-05-23T08:16:32Z

docs/docs/api/endpoints/components.md

+```json5
+{
+  "security": {
+    "systemToken": "token",


We want to make this API the new source of truth for interacting with Airy. Therefore I'd argue that as with all other endpoints we also convert these keys into snake_case so that the interface is uniform.

chrismatix · 2022-05-23T08:18:17Z

docs/docs/api/endpoints/components.md

+The list of the deleted components is returned.
+
+```json5
+{


Delete outer {} since this isn't valid json

chrismatix · 2022-05-23T08:20:04Z

infrastructure/controller/pkg/endpoints/components_update.go

+	}
+
+	var conf payloads.ComponentsUpdateRequestPayload
+	namespace := os.Getenv("NAMESPACE")


I think we can use the variable that's already stored in s.namespace here

chrismatix · 2022-05-23T08:23:03Z

docs/docs/api/endpoints/components.md

+  {
+    "name": "security",
+    "enabled": true,
+    "data": null


Can data ever be non-null? If no, then I'd remove it.

Also: security isn't a component

Do we need another endpoint for applying the configuration forsecurity?

juan-sebastian · 2022-05-23T08:55:06Z

I changed the cli part to use airy-controller endpoint. #3130

lib/go/httpclient/components.go

chrismatix · 2022-05-24T13:11:08Z

lib/go/httpclient/components.go

+	}
+
+	res := payloads.ComponentsUpdateResponsePayload{}
+	e := c.post("components.update", payload, &res)


I will fix this in my PR #3221

…iryhq/airy into feature/2756-config-through-controller

ljupcovangelski · 2022-05-27T13:28:36Z

Added another endpoint for updating the general configuration of Airy Core - cluster.update. Currently only security can be configured, but we can use that endpoint to also update other generic configuration in the future.
airy config apply applies both components through components.update and the cluster config through cluster.update.
Components are deleted only by explicitly calling components.delete.
Added conversion for the security attributes and the components attributes to snake_case when posting to the API. The controller converts them back to lower_camel_case before writing the configuration, so this is compatible with the current airy.yaml file and the data that the components expect in the configMaps.
When a configMap is not applied, the endpoint returns 500, even though some of the configMaps might have applied successfully before that. We return a list of which configMaps were applied and which one failed, just in case this is needed on the client side.
Updated the docs

juan-sebastian

Look good. Juste some small things to adresse and it will be good to merge

infrastructure/controller/pkg/endpoints/cluster_update.go

juan-sebastian

I did some minor changes it looks good now to me

docs/docs/api/endpoints/cluster.md

chrismatix · 2022-05-30T07:25:50Z

infrastructure/controller/pkg/endpoints/components_get.go

@@ -17,7 +17,7 @@ type ClusterGet struct {
 }

 func (s *ClusterGet) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	components, err := configmaps.GetComponentsConfigMaps(r.Context(), s.namespace, s.clientSet, maskSecrets)
+	components, err := k8s.GetComponentsConfigMaps(r.Context(), s.namespace, s.clientSet, maskSecrets)


Does this actually work yet? 🤔 When I run the image in my local cluster this endpoint returns:

{"components":{}}

Or perhaps there is something more than add the image that I have to do :)

Does this actually work yet? thinking When I run the image in my local cluster this endpoint returns:

{"components":{}}

Or perhaps there is something more than add the image that I have to do :)

You would also need to add the ingress rules.

This request is directly from within the cluster. If it was a networking issue the response would be a 404. Will do more testing later

Co-authored-by: Christoph Proeschel <chris@airy.co>

juan-sebastian

Perfect. great work!! Thanks for adding the label everywhere

github-actions bot added the feature label May 18, 2022

ljupcovangelski commented May 18, 2022

View reviewed changes

chrismatix reviewed May 18, 2022

View reviewed changes

ljupcovangelski changed the title ~~Feature/2756 config through controller~~ [#2756] Apply config through the airy controller May 18, 2022

ljupcovangelski force-pushed the feature/2756-config-through-controller branch 2 times, most recently from db4db7d to 537a769 Compare May 20, 2022 14:45

ljupcovangelski added 2 commits May 20, 2022 16:53

[#2756] Apply config through the airy controller

135499a

Rebase and fix stuff

0c62e5c

ljupcovangelski force-pushed the feature/2756-config-through-controller branch from 537a769 to 0c62e5c Compare May 20, 2022 15:11

chrismatix reviewed May 21, 2022

View reviewed changes

Update docs

50a247d

ljupcovangelski marked this pull request as ready for review May 23, 2022 07:54

chrismatix reviewed May 23, 2022

View reviewed changes

cli use airy-controller endpoint

bdc2f15

typo

5a6c53f

chrismatix reviewed May 23, 2022

View reviewed changes

lib/go/httpclient/components.go Outdated Show resolved Hide resolved

chrismatix reviewed May 24, 2022

View reviewed changes

ljupcovangelski added 5 commits May 25, 2022 09:48

cluster.update endpoint and modifying payloads

92f05d9

Merge branch 'feature/2756-config-through-controller' of github.com:a…

1bccd97

…iryhq/airy into feature/2756-config-through-controller

small cleanup

16d1dc1

Converting camel to snake_case and vise-versa.

c45b837

Add ingress

f576be1

juan-sebastian reviewed May 27, 2022

View reviewed changes

infrastructure/controller/pkg/endpoints/cluster_update.go Outdated Show resolved Hide resolved

minor changes

ac68018

juan-sebastian previously approved these changes May 27, 2022

View reviewed changes

chrismatix reviewed May 30, 2022

View reviewed changes

docs/docs/api/endpoints/cluster.md Outdated Show resolved Hide resolved

chrismatix reviewed May 30, 2022

View reviewed changes

docs/docs/api/endpoints/cluster.md Outdated Show resolved Hide resolved

chrismatix reviewed May 30, 2022

View reviewed changes

ljupcovangelski dismissed juan-sebastian’s stale review via 6be0e82 May 30, 2022 09:01

ljupcovangelski and others added 2 commits May 30, 2022 11:01

Apply suggestions from code review

6be0e82

Co-authored-by: Christoph Proeschel <chris@airy.co>

Add configMaps for every component

1fb3282

juan-sebastian approved these changes May 31, 2022

View reviewed changes

ljupcovangelski merged commit c7db4fe into develop May 31, 2022

ljupcovangelski deleted the feature/2756-config-through-controller branch May 31, 2022 11:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#2756] Apply config through the airy controller #3176

[#2756] Apply config through the airy controller #3176

ljupcovangelski commented May 18, 2022 •

edited

Loading

ljupcovangelski May 18, 2022

ljupcovangelski May 18, 2022

chrismatix May 18, 2022

juan-sebastian May 18, 2022 •

edited

Loading

chrismatix May 18, 2022

ljupcovangelski May 18, 2022 •

edited

Loading

chrismatix May 19, 2022

ljupcovangelski May 20, 2022

chrismatix May 21, 2022

ljupcovangelski May 23, 2022

chrismatix May 23, 2022

chrismatix May 23, 2022

chrismatix May 23, 2022

chrismatix May 23, 2022

ljupcovangelski May 23, 2022

juan-sebastian commented May 23, 2022

chrismatix May 24, 2022

juan-sebastian May 27, 2022

ljupcovangelski commented May 27, 2022 •

edited

Loading

juan-sebastian left a comment

juan-sebastian left a comment

chrismatix May 30, 2022

ljupcovangelski May 30, 2022

chrismatix May 30, 2022

juan-sebastian left a comment

[#2756] Apply config through the airy controller #3176

[#2756] Apply config through the airy controller #3176

Conversation

ljupcovangelski commented May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juan-sebastian May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljupcovangelski May 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juan-sebastian commented May 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljupcovangelski commented May 27, 2022 • edited Loading

juan-sebastian left a comment

Choose a reason for hiding this comment

juan-sebastian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juan-sebastian left a comment

Choose a reason for hiding this comment

ljupcovangelski commented May 18, 2022 •

edited

Loading

juan-sebastian May 18, 2022 •

edited

Loading

ljupcovangelski May 18, 2022 •

edited

Loading

ljupcovangelski commented May 27, 2022 •

edited

Loading