ROX-15088: Use integration health report in declarative config manager #5105

dhaus67 · 2023-03-06T03:14:57Z

Description

This PR is a follow up for pull/5048, in which the API of the /v1/integrationhealth service was extended to cover declarative configurations.

Within this PR, the integrationhealth.Reporter is now used within the declarativeconfig.Manager to report the health status of resources created from declarative configuration as well as the overall health of the config maps.

For now, the following is registered and updated:

For each watch handler (i.e. each config map), an integration health is created. In case any issues occurred during either unmarshalling the config file or transforming the contents, the health status is set to unhealthy and errors are surfaced.
For each transformed resource (i.e. proto message), an integration health is created. In case any issues occurred during upserting the resource, the health status is set to unhealthy and errors are surfaced.

Notes:

What's currently not covered, but already possible, is the deletion of the health status once a resource is deleted. This will be done within ROX-14694.

Caveats within the PR:

the handleMissingTypeUpdater called stopSignal.Done(), while it should have called stopSignal.Signal() (currently, not stop signal would be sent).
the release tests for ^^ didn't fail because the signals (both stopSignal & shortCircuitSignal) were not initialized via concurrency.NewSignal(), hence stopSignal.IsDone() always yielded true

Both things were fixed within this PR.

Checklist

Investigated and inspected CI test results
Unit test and regression tests added
Evaluated and added CHANGELOG entry if required
Determined and documented upgrade steps
Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

see added unit tests.

Manual tests:

Create the central-services chart

roxctl helm output central-services --image-defaults=opensource

Install the central services (a.k.a. central) within the stackrox namespace. Set the ROX_DECLARATIVE_CONFIGURATION feature flag to true, add declarative config mounts, and optionally set loglevel to debug

helm install stackrox-central-services -n stackrox -f - <<EOF
central:
  declarativeConfiguration:
    mounts:
      configMaps:
        - permission-set
        - access-scope
        - auth-provider
        - role
customize:
  envVars:
    ROX_DECLARATIVE_CONFIGURATION: true
    LOGLEVEL: Debug
EOF ./stackrox-central-services-chart

Once initialized, each configuration mount should be included within the response of the integration health service. They all should be UNINITIALIZED.

roxcurl /v1/integrationhealth/declarativeconfigs | jq
{
  "integrationHealth": [
    {
      "id": "/run/stackrox.io/declarative-configuration/access-scope",
      "name": "Config Map access-scope",
      "type": "DECLARATIVE_CONFIG",
      "status": "UNINITIALIZED",
      "errorMessage": "",
      "lastTimestamp": "2023-03-06T23:38:05.541191670Z"
    },
    {
      "id": "/run/stackrox.io/declarative-configuration/auth-provider",
      "name": "Config Map auth-provider",
      "type": "DECLARATIVE_CONFIG",
      "status": "UNINITIALIZED",
      "errorMessage": "",
      "lastTimestamp": "2023-03-06T23:38:05.593600136Z"
    },
    {
      "id": "/run/stackrox.io/declarative-configuration/permission-set",
      "name": "Config Map permission-set",
      "type": "DECLARATIVE_CONFIG",
      "status": "UNINITIALIZED",
      "errorMessage": "",
      "lastTimestamp": "2023-03-06T23:38:05.646174401Z"
    },
    {
      "id": "/run/stackrox.io/declarative-configuration/role",
      "name": "Config Map role",
      "type": "DECLARATIVE_CONFIG",
      "status": "UNINITIALIZED",
      "errorMessage": "",
      "lastTimestamp": "2023-03-06T23:38:05.698478865Z"
    }
  ]
}

Create a valid declarative config file containing a permission set

kubectl apply -f - <<EOF
apiVersion: v1
kind: ConfigMap
metadata:
  name: permission-set
  namespace: stackrox
data:
  permission-set: |
    name: hello-world
    description: the first declaratively created permission set
    resources:
    - resource: Integration
      access: READ_ACCESS
    - resource: Administration
      access: READ_ACCESS
    - resource: Access
      access: READ_ACCESS
EOF

The integration health list should now include the permission set, referencing the config map name, and the config map should be marked as healthy

roxcurl /v1/integrationhealth/declarativeconfigs | jq
{
  "integrationHealth": [
...
    {
      "id": "/run/stackrox.io/declarative-configuration/permission-set",
      "name": "Config Map permission-set",
      "type": "DECLARATIVE_CONFIG",
      "status": "HEALTHY",
      "errorMessage": "",
      "lastTimestamp": "2023-03-06T23:44:45.699022645Z"
    },
    {
      "id": "89e43e8c-a50b-5429-b53d-13c26b9d5080",
      "name": "hello-world in config map permission-set",
      "type": "DECLARATIVE_CONFIG",
      "status": "HEALTHY",
      "errorMessage": "",
      "lastTimestamp": "2023-03-06T23:46:05.723449047Z"
    }
  ]
}

Create a declarative configuration which contains invalid format

kubectl apply -f - <<EOF
apiVersion: v1
kind: ConfigMap
metadata:
  name: auth-provider
  namespace: stackrox
data:
  auth-provider: |
    {
      "some": "funny",
      "key": "value",
      "pairs": "here"
    }
EOF

The associated config map auth-provider should be marked unhealthy, including the error

roxcurl /v1/integrationhealth/declarativeconfigs | jq
{
  "integrationHealth": [
...
    {
      "id": "/run/stackrox.io/declarative-configuration/auth-provider",
      "name": "Config Map auth-provider",
      "type": "DECLARATIVE_CONFIG",
      "status": "UNHEALTHY",
      "errorMessage": "unmarshalling raw configuration: raw configuration {\n  \"some\": \"funny\",\n  \"key\": \"value\",\n  \"pairs\": \"here\"\n}\n didn't match any of the given configurations: 4 errors occurred:\n\t* yaml: unmarshal errors:\n  line 2: field some not found in type declarativeconfig.AuthProvider\n  line 3: field key not found in type declarativeconfig.AuthProvider\n  line 4: field pairs not found in type declarativeconfig.AuthProvider\n\t* yaml: unmarshal errors:\n  line 2: field some not found in type declarativeconfig.AccessScope\n  line 3: field key not found in type declarativeconfig.AccessScope\n  line 4: field pairs not found in type declarativeconfig.AccessScope\n\t* yaml: unmarshal errors:\n  line 2: field some not found in type declarativeconfig.PermissionSet\n  line 3: field key not found in type declarativeconfig.PermissionSet\n  line 4: field pairs not found in type declarativeconfig.PermissionSet\n\t* yaml: unmarshal errors:\n  line 2: field some not found in type declarativeconfig.Role\n  line 3: field key not found in type declarativeconfig.Role\n  line 4: field pairs not found in type declarativeconfig.Role\n\n",
      "lastTimestamp": "2023-03-06T23:49:30.645077233Z"
    }
  ]
}

Create a declarative configuration which contains an invalid reference (i.e. a role referencing a non-existing access scope)

kubectl apply -f - <<EOF
apiVersion: v1
kind: ConfigMap
metadata:
  name: role
  namespace: stackrox
data:
  role: |
    name: hello-world
    description: the first declaratively created role
    permissionSet: hello-world
    accessScope: hello-world
EOF

The associated config map should be marked healthy, as configurations could be unmarshalled from it, but the created resources should be marked unhealthy (note: this shouldn't be the case immediately, but only after the error occurred 5 times)

roxcurl /v1/integrationhealth/declarativeconfigs | jq
{
  "integrationHealth": [
...
    {
      "id": "hello-world",
      "name": "hello-world in config map role",
      "type": "DECLARATIVE_CONFIG",
      "status": "UNHEALTHY",
      "errorMessage": "referenced access scope df5d408f-e083-5920-b35a-774684481925 does not exist: invalid arguments",
      "lastTimestamp": "2023-03-06T23:57:05.725916471Z"
    }
  ]
}

dhaus67 · 2023-03-06T03:15:10Z

Current dependencies on/for this PR:

master
- PR ROX-15088: Use integration health report in declarative config manager #5105 👈

This comment was auto-generated by Graphite.

ghost · 2023-03-06T03:43:03Z

Images are ready for the commit at 485047c.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.74.x-338-g485047cbe4.

central/declarativeconfig/manager_impl.go

dhaus67 · 2023-03-07T11:39:04Z

/retest

central/declarativeconfig/manager_impl.go

ivan-degtiarenko · 2023-03-07T14:38:03Z

central/declarativeconfig/manager_impl.go

-		for _, message := range messages {
-			if err := typeUpdater.Upsert(m.reconciliationCtx, message); err != nil {
-				m.reconciliationErrorReporter.ProcessError(message, err)
+		for handler, protoMessagesByType := range transformedMessagesByHandler {


I'm eager to keep global order of proto types to avoid extra errors and displaying unhealthy configs when it's not needed. How about we use a structure like map[protoType]map[handlerID][]proto.Message?

P.S. Another benefit of this structure is that it will allow to easily flag all handlers which contain unsupported type as unhealthy

But, the global order is kept right now, no?

We go through each proto type, and for each proto type we consolidate all handlers, retrieving only the proto messages for this specific type. Once done, we continue with the next proto type. The global order is kept (also, ensured within the tests via gomock.InOrder that the order is actually kept).

Right, sorry - didn't notice the outer loop. No questions to this part

ivan-degtiarenko · 2023-03-07T14:51:23Z

central/declarativeconfig/types/extractors.go

+}
+
+func extractNameFromProtoMessage(message proto.Message) string {
+	messageWithName, ok := message.(interface {


One alternative for groups that comes to mind is to create a fake name based on group properties - something like provider_id:key:value:roleName - WDYT?

I guess we are back to good old serialized key for groups 🥴

I'm not sure how much it's better, given that the auth provider ID will be non-telling for customers to identify the auth provider (imagine you have two groups sharing the same attributes / role name, but for different auth providers). Guess the same applies right now with the group ID, so I changed it to be the tuple of these values

I guess we are back to good old serialized key for groups 🥴

“The more you know about the past, the better prepared you are for the future.” —Theodore Roosevelt

given that the auth provider ID will be non-telling for customers to identify the auth provider

I guess you're right, they should know what they are looking at. What do you think about terrifying
auth_provider_id:<id>:key:<key>:value:<value>:role_name:<roleName> format? A more pleasant alternative would be group <key>:<value>:<roleName> for auth provider ID <authProviderID>

Yeah I'm fine doing it either way, since we can be flexible with the name.
The root issue we have is that the group has no name and the auth provider is in ID-format instead of name, so looking at the declarative config it may be non-trivial identifying the groups.

Updated the names to the group <key>:<value>:<roleName> for auth provider ID <authProviderID> format.

central/declarativeconfig/manager_impl_test.go

ivan-degtiarenko · 2023-03-07T15:03:44Z

central/declarativeconfig/manager_impl_test.go

+		Name:         "Config Map my-cool-config-map",
+		Type:         storage.IntegrationHealth_DECLARATIVE_CONFIG,
+		Status:       storage.IntegrationHealth_UNHEALTHY,
+		ErrorMessage: "unmarshalling raw configuration: raw configuration \n{\"cool\": \"key\", \"value\": \"pairs\"}\n didn't match any of the given configurations: 4 errors occurred:\n\t* yaml: unmarshal errors:\n  line 2: field cool not found in type declarativeconfig.AuthProvider\n  line 2: field value not found in type declarativeconfig.AuthProvider\n\t* yaml: unmarshal errors:\n  line 2: field cool not found in type declarativeconfig.AccessScope\n  line 2: field value not found in type declarativeconfig.AccessScope\n\t* yaml: unmarshal errors:\n  line 2: field cool not found in type declarativeconfig.PermissionSet\n  line 2: field value not found in type declarativeconfig.PermissionSet\n\t* yaml: unmarshal errors:\n  line 2: field cool not found in type declarativeconfig.Role\n  line 2: field value not found in type declarativeconfig.Role\n\n",


Perhaps we can specify matcher to check not exact error message here, but rather that it contains an unmarshalling error

I guess we could, but I don't want to make the gomock.Matcher more complex than it needs to be. I'd just keep it for now, maybe later we can change it if another long-ish error exists.

ivan-degtiarenko

Let's resolve conversation around group name and merge this PR 👍

ivan-degtiarenko · 2023-03-09T19:11:48Z

central/declarativeconfig/manager_impl.go

+		errorsPerDeclarativeConfig:     map[string]int32{},
+		idExtractor:                    idExtractor,
+		nameExtractor:                  nameExtractor,
+		shortCircuitSignal:             concurrency.NewSignal(),


nit: is it necessary to initialize signals? Don't think we did that before

See the description:

the release tests for ^^ didn't fail because the signals (both stopSignal & shortCircuitSignal) were not initialized via concurrency.NewSignal(), hence stopSignal.IsDone() always yielded true

Currently, the reconciliation didn't work because we always stopped the reconciliation loop.

ivan-degtiarenko · 2023-03-09T19:14:39Z

central/declarativeconfig/manager_impl.go

-		for _, message := range messages {
-			if err := typeUpdater.Upsert(m.reconciliationCtx, message); err != nil {
-				m.reconciliationErrorReporter.ProcessError(message, err)
+		for handler, protoMessagesByType := range transformedMessagesByHandler {


Right, sorry - didn't notice the outer loop. No questions to this part

ivan-degtiarenko · 2023-03-09T19:15:34Z

central/declarativeconfig/manager_impl.go

+		}
+		log.Debugf("Error within reconciliation for %+v: %v", message, err)
+	} else {
+		currentDeclarativeConfigErrors := m.errorsPerDeclarativeConfig[messageID]


These 3 lines can be simplified to

m.errorsPerDeclarativeConfig[messageID] = 0

ivan-degtiarenko · 2023-03-09T19:25:35Z

central/declarativeconfig/types/extractors.go

+}
+
+func extractNameFromProtoMessage(message proto.Message) string {
+	messageWithName, ok := message.(interface {


I guess we are back to good old serialized key for groups 🥴

“The more you know about the past, the better prepared you are for the future.” —Theodore Roosevelt

given that the auth provider ID will be non-telling for customers to identify the auth provider

I guess you're right, they should know what they are looking at. What do you think about terrifying
auth_provider_id:<id>:key:<key>:value:<value>:role_name:<roleName> format? A more pleasant alternative would be group <key>:<value>:<roleName> for auth provider ID <authProviderID>

ivan-degtiarenko

Couple of small fixes, otherwise LGTM

ivan-degtiarenko · 2023-03-09T22:34:23Z

central/declarativeconfig/types/extractors.go

+// IDExtractor extracts the ID from proto messages.
+type IDExtractor func(m proto.Message) string
+
+// NameExtractor extracts the ID from proto messages.


Suggested change

// NameExtractor extracts the ID from proto messages.

// NameExtractor extracts the name from proto messages.

#5105)

dhaus67 requested a review from ivan-degtiarenko March 6, 2023 03:15

github-actions bot added the area/central label Mar 6, 2023

ivan-degtiarenko reviewed Mar 6, 2023

View reviewed changes

dhaus67 force-pushed the master-dh/rox-15088-report-errors-during-creation branch from 80656e3 to 1abf2f1 Compare March 6, 2023 21:48

dhaus67 requested a review from ivan-degtiarenko March 6, 2023 21:48

dhaus67 force-pushed the master-dh/rox-15088-report-errors-during-creation branch from 1abf2f1 to 67c340c Compare March 6, 2023 22:36

ivan-degtiarenko reviewed Mar 7, 2023

View reviewed changes

dhaus67 requested a review from ivan-degtiarenko March 7, 2023 21:54

dhaus67 force-pushed the master-dh/rox-15088-report-errors-during-creation branch 3 times, most recently from aaa15f1 to d975301 Compare March 8, 2023 12:54

ivan-degtiarenko reviewed Mar 9, 2023

View reviewed changes

dhaus67 added 7 commits March 9, 2023 20:32

rox-15088-report-errors-during-creation

50f4176

Address review comments.

64180f6

Fix initializing stop signal; refactor tests.

8b1d9c7

Final touches.

b91e177

Address review comments.

6126c8a

Fix.

88d9f4b

Address final review comments.

6a4a3ad

dhaus67 requested a review from ivan-degtiarenko March 9, 2023 19:38

dhaus67 force-pushed the master-dh/rox-15088-report-errors-during-creation branch from d975301 to 6a4a3ad Compare March 9, 2023 19:38

ivan-degtiarenko approved these changes Mar 9, 2023

View reviewed changes

Address final review comments.

485047c

dhaus67 enabled auto-merge (squash) March 10, 2023 00:20

dhaus67 merged commit a14a7f3 into master Mar 10, 2023

dhaus67 deleted the master-dh/rox-15088-report-errors-during-creation branch March 10, 2023 01:02

vikin91 pushed a commit that referenced this pull request Mar 13, 2023

ROX-15088: Use integration health report in declarative config manager (

d4efd24

#5105)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROX-15088: Use integration health report in declarative config manager #5105

ROX-15088: Use integration health report in declarative config manager #5105

dhaus67 commented Mar 6, 2023 •

edited

Loading

dhaus67 commented Mar 6, 2023

ghost commented Mar 6, 2023 •

edited by ghost

Loading

dhaus67 commented Mar 7, 2023

ivan-degtiarenko Mar 7, 2023

dhaus67 Mar 7, 2023

ivan-degtiarenko Mar 9, 2023

ivan-degtiarenko Mar 7, 2023

dhaus67 Mar 7, 2023

ivan-degtiarenko Mar 9, 2023

dhaus67 Mar 9, 2023

ivan-degtiarenko Mar 7, 2023

dhaus67 Mar 7, 2023

ivan-degtiarenko left a comment

ivan-degtiarenko Mar 9, 2023

dhaus67 Mar 9, 2023

ivan-degtiarenko Mar 9, 2023

ivan-degtiarenko Mar 9, 2023

ivan-degtiarenko Mar 9, 2023

ivan-degtiarenko left a comment

ivan-degtiarenko Mar 9, 2023

	// NameExtractor extracts the ID from proto messages.
	// NameExtractor extracts the name from proto messages.

ROX-15088: Use integration health report in declarative config manager #5105

ROX-15088: Use integration health report in declarative config manager #5105

Conversation

dhaus67 commented Mar 6, 2023 • edited Loading

Description

Checklist

Testing Performed

dhaus67 commented Mar 6, 2023

ghost commented Mar 6, 2023 • edited by ghost Loading

dhaus67 commented Mar 7, 2023

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

ivan-degtiarenko 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivan-degtiarenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaus67 commented Mar 6, 2023 •

edited

Loading

ghost commented Mar 6, 2023 •

edited by ghost

Loading