ROX-19820: Address feedback from gitops meeting #1294

ludydoo · 2023-09-21T14:53:29Z

Description

ROX-19820

Addressing issues and feedback from the gitops meeting

Do not use templating for specifying the CRD base url.Directly specify as such. Simpler than using a template.
Add flags disableCentralReconciler and disableSecuredClusterReconciler on the operator
Drop Git References. Necessary to accommodate the Dogfood instance. The gitRef abstraction is not sufficient/flexible enough.
align the gitops operator config with the helm values

Checklist (Definition of Done)

Unit and integration tests added
Added test description under Test manual
Documentation added if necessary (i.e. changes to dev setup, test execution, ...)
CI and all relevant tests are passing
Add the ticket number to the PR title if available, i.e. ROX-12345: ...
Discussed security and business related topics privately. Will move any security and business related topics that arise to private communication channel.
Add secret to app-interface Vault or Secrets Manager if necessary
RDS changes were e2e tested manually
Check AWS limits are reasonable for changes provisioning new resources

Test manual

TODO: Add manual testing efforts

# To run tests locally run:
make db/teardown db/setup db/migrate
make ocm/setup OCM_OFFLINE_TOKEN=<ocm-offline-token> OCM_ENV=development
make verify lint binary test test/integration

ludydoo · 2023-09-21T14:55:46Z

e2e/e2e_canary_upgrade_test.go

 			Expect(err).To(BeNil())

-			It("should deploy operator with label selector 4.1.1", func() {
-				Eventually(validateOperatorDeployment(ctx, "4.1.1", "quay.io/rhacs-eng/stackrox-operator:4.1.1")).
+			It("should contain only two operator deployments", func() {


moved checking that 2 operators were deployed before asserting their properties

ludydoo · 2023-09-21T14:56:04Z

e2e/e2e_canary_upgrade_test.go

-					WithTimeout(waitTimeout).
-					WithPolling(defaultPolling).
-					Should(Succeed())
-			})
 			It("should contain only one operator deployments", func() {


moved checking that only 1 operator deployment is present before asserting its properties

ludydoo · 2023-09-21T14:56:20Z

e2e/e2e_canary_upgrade_test.go

 			Expect(err).To(BeNil())

+			It("should contain one operator deployment", func() {


Added an assertion to check the presence of 1 operator deployment

ludydoo · 2023-09-21T14:57:48Z

fleetshard/pkg/central/charts/data/rhacs-operator/templates/rhacs-operator-deployment.yaml

@@ -82,9 +82,21 @@ spec:
                  containerName: manager
                  resource: limits.memory
                  divisor: '0'
-            {{- if .labelSelector }}
+            {{- if .centralLabelSelector }}


there will be both a centralLabelSelector and securedClusterLabelSelector hence the renaming

ludydoo · 2023-09-21T14:58:10Z

fleetshard/pkg/central/operator/config.go

@@ -16,82 +24,82 @@ func parseConfig(content []byte) (OperatorConfigs, error) {
 	return out, nil
 }

-// Validate validates the operator configuration and can be used in different life-cycle stages like runtime and deploy time.


Moved to validation.go with all validation methods

ludydoo · 2023-09-21T14:58:55Z

fleetshard/pkg/central/operator/config.go

-	Image      string `json:"image"`
-	GitRef     string `json:"gitRef"`
-	HelmValues string `json:"helmValues,omitempty"`
+type OperatorConfig map[string]interface{}


Changed to map[string]interface{} so that it is compatible with helm values

ludydoo · 2023-09-21T14:59:20Z

fleetshard/pkg/central/operator/config.go

+type OperatorConfig map[string]interface{}
+
+// GetDeploymentName returns the deployment name of the operator.
+func (o OperatorConfig) GetDeploymentName() string {


Added getters to simplify accessing properties of the map[string]interface{} with known keys

ludydoo · 2023-09-21T15:00:00Z

dev/env/manifests/fleet-manager/04-gitops-config.yaml

@@ -6,22 +6,31 @@ metadata:
 data:
  config.yaml: |
    rhacsOperators:
-      crd:


Per feedback that raw urls were preferrable

ludydoo · 2023-09-21T15:00:45Z

fleetshard/pkg/central/operator/config.go

-
-// CRDConfig represents the crd to be installed in the data-plane cluster. The CRD is downloaded automatically
-// from the base URL. It takes a GitRef to resolve a GitHub link to the CRD definition.
-type CRDConfig struct {


Removed as this is now a []string

ludydoo · 2023-09-21T15:02:44Z

fleetshard/pkg/central/operator/config_test.go

-	assert.Equal(t, "quay.io/rhacs-eng/stackrox-operator:4.1.1", conf.Configs[0].Image)
-}
-
-func TestGetOperatorConfigFailsValidation(t *testing.T) {


Moved to validation_test.go

ludydoo · 2023-09-21T15:03:08Z

fleetshard/pkg/central/operator/upgrade.go

 )

-func parseOperatorConfigs(operators OperatorConfigs) ([]chartutil.Values, error) {


Moved validation logic to validation.go

kurlov

Looks nice!

kurlov · 2023-09-21T15:02:38Z

dev/env/manifests/fleet-manager/04-gitops-config.yaml

-          image: "quay.io/rhacs-eng/stackrox-operator:4.1.1"
-        - gitRef: 4.1.0
+
+        - deploymentName: "rhacs-operator-4-2-0"


nit: dots are allowed in deployment name if they are not in the end

I meant that rhacs-operator-4.2.0 is a valid deployment name but rhacs-operator-4.2.0. is not

e2e/e2e_canary_upgrade_test.go

fleetshard/pkg/central/charts/data/rhacs-operator/templates/rhacs-operator-deployment.yaml

fleetshard/pkg/central/charts/data/rhacs-operator/values.yaml

kurlov · 2023-09-21T15:11:22Z

fleetshard/pkg/central/operator/config.go

 // OperatorConfigs represents all operators and the CRD which should be installed in a data-plane cluster.
 type OperatorConfigs struct {
-	CRD     CRDConfig        `json:"crd"`
+	CRDURLs []string         `json:"crdUrls"`


Just a thought: Should we replace list of URLs with two values? CentralCRDURL and SecuredClusterCRDURL

I don't mind either way

Though, this will get passed to helm directly. Its function is not to install CRDs per se, but it can install any yaml resource.

Rename to additionalResources, and allow any yaml to be specified there.

Either keep CrdUrls or split into CentralCRDUrl or SecuredClusterCRDUrl, and validate that these files actually contain only 1 valid kind: CustomResourceDefinition

SimonBaeumer

Great job here @ludydoo and @kurlov 👊

SimonBaeumer · 2023-09-22T09:34:03Z

dev/env/manifests/fleet-manager/04-gitops-config.yaml

          image: "quay.io/rhacs-eng/stackrox-operator:4.1.0"
+          centralLabelSelector: "rhacs.redhat.com/version-selector=4.1.0"
+          disableSecuredClusterReconciler: true


nit: instead of disableSecuredClusterReconciler it should be securedClusterReconcilerEnabled: false. Double-negations are often done wrong.
Fred and me had it yesterday where we wanted to enable delegated scanning but disabled it:
ROX_DELEGATED_SCANNING_DISABLED=true.

openshift-ci · 2023-09-22T15:37:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kurlov, ludydoo, SimonBaeumer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [SimonBaeumer,kurlov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Address feedback from gitops meeting

01b9bbc

ludydoo requested a review from kurlov September 21, 2023 14:53

ludydoo temporarily deployed to development September 21, 2023 14:53 — with GitHub Actions Inactive

ludydoo requested a review from SimonBaeumer September 21, 2023 14:53

ludydoo temporarily deployed to development September 21, 2023 14:53 — with GitHub Actions Inactive

ludydoo commented Sep 21, 2023

View reviewed changes

Remove unused struct

94b1ba0

ludydoo commented Sep 21, 2023

View reviewed changes

ludydoo temporarily deployed to development September 21, 2023 14:59 — with GitHub Actions Inactive

ludydoo commented Sep 21, 2023

View reviewed changes

kurlov reviewed Sep 21, 2023

View reviewed changes

Fix tests

42fe6cc

ludydoo temporarily deployed to development September 21, 2023 15:25 — with GitHub Actions Inactive

PR Comments

84adada

ludydoo temporarily deployed to development September 21, 2023 15:30 — with GitHub Actions Inactive

ludydoo temporarily deployed to development September 21, 2023 15:33 — with GitHub Actions Inactive

Fix runtime

0b93283

ludydoo temporarily deployed to development September 21, 2023 16:10 — with GitHub Actions Inactive

ludydoo temporarily deployed to development September 21, 2023 16:11 — with GitHub Actions Inactive

ludydoo requested a review from kurlov September 21, 2023 17:05

SimonBaeumer approved these changes Sep 22, 2023

View reviewed changes

openshift-ci bot assigned SimonBaeumer Sep 22, 2023

openshift-ci bot added lgtm approved labels Sep 22, 2023

PR Comments

329d29e

openshift-ci bot removed the lgtm label Sep 22, 2023

ludydoo temporarily deployed to development September 22, 2023 11:54 — with GitHub Actions Inactive

tests

19af8eb

ludydoo temporarily deployed to development September 22, 2023 12:05 — with GitHub Actions Inactive

ludydoo temporarily deployed to development September 22, 2023 12:06 — with GitHub Actions Inactive

tests

e0d2664

ludydoo temporarily deployed to development September 22, 2023 13:14 — with GitHub Actions Inactive

kurlov approved these changes Sep 22, 2023

View reviewed changes

openshift-ci bot assigned kurlov Sep 22, 2023

openshift-ci bot added the lgtm label Sep 22, 2023

ludydoo merged commit 479d887 into main Sep 28, 2023

ludydoo deleted the ROX-19820-gitops-feedback branch September 28, 2023 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ROX-19820: Address feedback from gitops meeting #1294

ROX-19820: Address feedback from gitops meeting #1294

ludydoo commented Sep 21, 2023 •

edited

Loading

ludydoo Sep 21, 2023

ludydoo Sep 21, 2023

ludydoo Sep 21, 2023

ludydoo Sep 21, 2023

ludydoo Sep 21, 2023

ludydoo Sep 21, 2023

ludydoo Sep 21, 2023

ludydoo Sep 21, 2023

ludydoo Sep 21, 2023

ludydoo Sep 21, 2023

ludydoo Sep 21, 2023

kurlov left a comment

kurlov Sep 21, 2023

ludydoo Sep 21, 2023

kurlov Sep 21, 2023

kurlov Sep 21, 2023

ludydoo Sep 21, 2023

ludydoo Sep 22, 2023 •

edited

Loading

SimonBaeumer left a comment

SimonBaeumer Sep 22, 2023

openshift-ci bot commented Sep 22, 2023

		Expect(err).To(BeNil())

		It("should contain one operator deployment", func() {

		)

		func parseOperatorConfigs(operators OperatorConfigs) ([]chartutil.Values, error) {

ROX-19820: Address feedback from gitops meeting #1294

ROX-19820: Address feedback from gitops meeting #1294

Conversation

ludydoo commented Sep 21, 2023 • edited Loading

Description

Checklist (Definition of Done)

Test manual

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

Choose a reason for hiding this comment

kurlov 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

ludydoo Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

SimonBaeumer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 22, 2023

ludydoo commented Sep 21, 2023 •

edited

Loading

ludydoo Sep 22, 2023 •

edited

Loading