Skip to content
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

ROX-19820: Address feedback from gitops meeting #1294

Merged
merged 9 commits into from
Sep 28, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions dev/env/manifests/fleet-manager/04-gitops-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,31 @@ metadata:
data:
config.yaml: |
rhacsOperators:
crd:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per feedback that raw urls were preferrable

baseURL: https://raw.githubusercontent.com/stackrox/stackrox/{{ .GitRef }}/operator/bundle/manifests/
gitRef: 4.1.1

crdUrls:
- https://raw.githubusercontent.com/stackrox/stackrox/4.1.2/operator/bundle/manifests/platform.stackrox.io_securedclusters.yaml
- https://raw.githubusercontent.com/stackrox/stackrox/4.1.2/operator/bundle/manifests/platform.stackrox.io_centrals.yaml

operators:
- gitRef: 4.1.1
image: "quay.io/rhacs-eng/stackrox-operator:4.1.1"
- gitRef: 4.1.0

- deploymentName: "rhacs-operator-4-2-0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

waaat?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

- deploymentName: "rhacs-operator-4-1-0"
image: "quay.io/rhacs-eng/stackrox-operator:4.1.0"
centralLabelSelector: "rhacs.redhat.com/version-selector=4.1.0"
disableSecuredClusterReconciler: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


centrals:
overrides:
# Set label for all centrals to 4.1.0
- instanceIds: ["*"]
patch: |
metadata:
labels:
rhacs.redhat.com/version-selector: "4.1.0"
rhacs.redhat.com/version-selector: "4.2.0"
- instanceIds:
- "*"
patch: |
Expand Down
180 changes: 98 additions & 82 deletions e2e/e2e_canary_upgrade_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,17 @@ const (
namespace = "acsms"
)

func operatorConfigForVersion(version string) operator.OperatorConfig {
return operator.OperatorConfig{
"name": fmt.Sprintf("rhacs-operator-%s", version),
ludydoo marked this conversation as resolved.
Show resolved Hide resolved
"image": fmt.Sprintf("quay.io/rhacs-eng/stackrox-operator:%s", version),
"centralLabelSelector": fmt.Sprintf("rhacs.redhat.com/version-selector=%s", version),
}
}

var _ = Describe("Fleetshard-sync Targeted Upgrade", func() {

BeforeEach(func() {

if !features.TargetedOperatorUpgrades.Enabled() || !features.StandaloneMode.Enabled() {
Skip("Skipping canary upgrade test")
}
Expand All @@ -44,85 +51,78 @@ var _ = Describe("Fleetshard-sync Targeted Upgrade", func() {
client, err := fleetmanager.NewClient(fleetManagerEndpoint, auth)
Expect(err).ToNot(HaveOccurred())

operatorConfig1 := operatorConfigForVersion("4.1.1")
operatorConfig2 := operatorConfigForVersion("4.1.2")

Describe("should run ACS operators", func() {

It("should deploy single operator", func() {
ctx := context.Background()
oneOperatorVersionConfig := []operator.OperatorConfig{{
GitRef: "4.1.1",
Image: "quay.io/rhacs-eng/stackrox-operator:4.1.1",
}}
err := updateOperatorConfig(ctx, oneOperatorVersionConfig)
operatorConfigs := []operator.OperatorConfig{operatorConfig1}
err := updateOperatorConfig(ctx, operatorConfigs)
Expect(err).To(BeNil())

It("should contain one operator deployment", func() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an assertion to check the presence of 1 operator deployment

Eventually(getOperatorDeployments(ctx)).
WithTimeout(waitTimeout).
WithPolling(defaultPolling).
Should(HaveLen(1))
})
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")).
Eventually(operatorMatchesConfig(ctx, operatorConfig1)).
WithTimeout(waitTimeout).
WithPolling(defaultPolling).
Should(Succeed())
})

})

It("should deploy two operators with different versions", func() {
ctx := context.Background()
twoOperatorVersionConfig := []operator.OperatorConfig{{
GitRef: "4.1.1",
Image: "quay.io/rhacs-eng/stackrox-operator:4.1.1",
},
{
GitRef: "4.1.2",
Image: "quay.io/rhacs-eng/stackrox-operator:4.1.2",
},
}
operatorConfigs := []operator.OperatorConfig{operatorConfig1, operatorConfig2}

err := updateOperatorConfig(ctx, twoOperatorVersionConfig)
err := updateOperatorConfig(ctx, operatorConfigs)
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() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved checking that 2 operators were deployed before asserting their properties

Eventually(getOperatorDeployments(ctx)).
WithTimeout(waitTimeout).
WithPolling(defaultPolling).
Should(Succeed())
Should(HaveLen(2))
})
It("should deploy operator with label selector 4.1.2", func() {
Eventually(validateOperatorDeployment(ctx, "4.1.2", "quay.io/rhacs-eng/stackrox-operator:4.1.2")).
It("should deploy operator with label selector 4.1.1", func() {
Eventually(operatorMatchesConfig(ctx, operatorConfig1)).
WithTimeout(waitTimeout).
WithPolling(defaultPolling).
Should(Succeed())
})
It("should contain only two operator deployments", func() {
Eventually(getOperatorDeployments(ctx)).
It("should deploy operator with label selector 4.1.2", func() {
Eventually(operatorMatchesConfig(ctx, operatorConfig2)).
WithTimeout(waitTimeout).
WithPolling(defaultPolling).
Should(HaveLen(2))
Should(Succeed())
})
})

It("should delete the removed operator", func() {
ctx := context.Background()
operatorConfig := []operator.OperatorConfig{{
GitRef: "4.1.2",
Image: "quay.io/rhacs-eng/stackrox-operator:4.1.2",
}}
err := updateOperatorConfig(ctx, operatorConfig)
operatorConfigs := []operator.OperatorConfig{operatorConfig1}
err := updateOperatorConfig(ctx, operatorConfigs)
Expect(err).To(BeNil())

It("should deploy operator with label selector 4.1.2", func() {
Eventually(validateOperatorDeployment(ctx, "4.1.2", "quay.io/rhacs-eng/stackrox-operator:4.1.2")).
WithTimeout(waitTimeout).
WithPolling(defaultPolling).
Should(Succeed())
})
It("should contain only one operator deployments", func() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Eventually(getOperatorDeployments(ctx)).
WithTimeout(waitTimeout).
WithPolling(defaultPolling).
Should(HaveLen(1))
})

It("should deploy operator with label selector 4.1.2", func() {
Eventually(operatorMatchesConfig(ctx, operatorConfig1)).
WithTimeout(waitTimeout).
WithPolling(defaultPolling).
Should(Succeed())
})
})

})

Describe("should upgrade the central", func() {
Expand All @@ -147,10 +147,8 @@ var _ = Describe("Fleetshard-sync Targeted Upgrade", func() {
centralNamespace, err = services.FormatNamespace(createdCentral.Id)

ctx := context.Background()
oneOperatorVersionConfig := []operator.OperatorConfig{{
GitRef: "4.1.1",
Image: "quay.io/rhacs-eng/stackrox-operator:4.1.1",
}}
operatorConfig1 := operatorConfigForVersion("4.1.1")
oneOperatorVersionConfig := []operator.OperatorConfig{operatorConfig1}
err = updateOperatorConfig(ctx, oneOperatorVersionConfig)
Expect(err).To(BeNil())

Expand All @@ -165,7 +163,7 @@ var _ = Describe("Fleetshard-sync Targeted Upgrade", func() {
return nil
}).WithTimeout(waitTimeout).WithPolling(defaultPolling).Should(Succeed())
Eventually(func() error {
centralDeployment, err := getCentralDeployment(ctx, createdCentral.Name, centralNamespace)
centralDeployment, err := getDeployment(ctx, centralNamespace, createdCentral.Name)
if err != nil {
return fmt.Errorf("failed finding central deployment: %v", err)
}
Expand All @@ -178,15 +176,14 @@ var _ = Describe("Fleetshard-sync Targeted Upgrade", func() {

It("upgrade central", func() {
ctx := context.Background()
oneOperatorVersionConfig := []operator.OperatorConfig{{
GitRef: "4.1.2",
Image: "quay.io/rhacs-eng/stackrox-operator:4.1.2",
}}
oneOperatorVersionConfig := []operator.OperatorConfig{
operatorConfigForVersion("4.1.2"),
}
err = updateOperatorConfig(ctx, oneOperatorVersionConfig)
Expect(err).To(BeNil())

Eventually(func() error {
centralDeployment, err := getCentralDeployment(ctx, createdCentral.Name, centralNamespace)
centralDeployment, err := getDeployment(ctx, centralNamespace, createdCentral.Name)
if err != nil {
return fmt.Errorf("failed finding central deployment: %v", err)
}
Expand Down Expand Up @@ -249,51 +246,70 @@ func getCentralCR(ctx context.Context, name string, namespace string) (*v1alpha1
return central, nil
}

func getCentralDeployment(ctx context.Context, name string, namespace string) (*appsv1.Deployment, error) {
func getDeployment(ctx context.Context, namespace string, name string) (*appsv1.Deployment, error) {
deployment := &appsv1.Deployment{}
centralKey := ctrlClient.ObjectKey{Namespace: namespace, Name: name}
err := k8sClient.Get(ctx, centralKey, deployment)
err := k8sClient.Get(ctx, ctrlClient.ObjectKey{Namespace: namespace, Name: name}, deployment)
return deployment, err
}

func validateOperatorDeployment(ctx context.Context, gitRef string, image string) error {
deploymentName := "rhacs-operator-" + gitRef
labelSelectorEnv := "rhacs.redhat.com/version-selector=" + gitRef
deployment := &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: deploymentName,
Namespace: operator.ACSOperatorNamespace,
},
func getContainer(name string, containers []v1.Container) (*v1.Container, error) {
for _, container := range containers {
if container.Name == name {
return &container, nil
}
}
return nil, fmt.Errorf("container %s not found", name)
}

err := k8sClient.Get(ctx, ctrlClient.ObjectKey{Namespace: operator.ACSOperatorNamespace, Name: deploymentName}, deployment)
if err != nil {
return err
type operatorAssertion func(deploy *appsv1.Deployment) error

var operatorHasImage = func(image string) operatorAssertion {
return func(deploy *appsv1.Deployment) error {
container, err := getContainer("manager", deploy.Spec.Template.Spec.Containers)
if err != nil {
return err
}
if container.Image != image {
return fmt.Errorf("incorrect image %s", container.Image)
}
return nil
}
foundManager := false
foundLabelSelector := false
containers := deployment.Spec.Template.Spec.Containers
for _, container := range containers {
if container.Name == "manager" {
foundManager = true
if container.Image != image {
return fmt.Errorf("incorrect operator image %s", container.Image)
}
for _, envVar := range container.Env {
if envVar.Name == "CENTRAL_LABEL_SELECTOR" {
foundLabelSelector = true
if envVar.Value != labelSelectorEnv {
return fmt.Errorf("incorrect CENTRAL_LABEL_SELECTOR %s", envVar.Value)
}
}

var operatorHasCentralLabelSelector = func(labelSelector string) operatorAssertion {
return func(deploy *appsv1.Deployment) error {
container, err := getContainer("manager", deploy.Spec.Template.Spec.Containers)
if err != nil {
return err
}
for _, envVar := range container.Env {
if envVar.Name == "CENTRAL_LABEL_SELECTOR" {
if envVar.Value != labelSelector {
return fmt.Errorf("incorrect value %s", envVar.Value)
}
}
if !foundLabelSelector {
return fmt.Errorf("environment variable CENTRAL_LABEL_SELECTOR not found")
}
}
return nil
}
if !foundManager {
return fmt.Errorf("manager container not found")
}

func validateOperatorDeployment(deployment *appsv1.Deployment, assertions ...operatorAssertion) error {
for _, assertion := range assertions {
err := assertion(deployment)
if err != nil {
return err
}
}
return nil
}

func operatorMatchesConfig(ctx context.Context, config operator.OperatorConfig) error {
deploy, err := getDeployment(ctx, operator.ACSOperatorNamespace, config.GetDeploymentName())
if err != nil {
return err
}
return validateOperatorDeployment(deploy,
operatorHasImage(config.GetImage()),
operatorHasCentralLabelSelector(config.GetCentralLabelSelector()),
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,21 @@ spec:
containerName: manager
resource: limits.memory
divisor: '0'
{{- if .labelSelector }}
{{- if .centralLabelSelector }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there will be both a centralLabelSelector and securedClusterLabelSelector hence the renaming

- name: CENTRAL_LABEL_SELECTOR
value: "rhacs.redhat.com/version-selector={{ .labelSelector }}"
value: "{{ .centralLabelSelector }}"
{{- end }}
{{- if .securedClusterLabelSelector }}
- name: SECURED_CLUSTER_LABEL_SELECTOR
value: "{{ .securedClusterLabelSelector }}"
{{- end }}
{{- if .disableCentralReconciler }}
- name: CENTRAL_RECONCILER_DISABLED
ludydoo marked this conversation as resolved.
Show resolved Hide resolved
value: "true"
{{- end }}
{{- if .disableSecuredClusterReconciler }}
- name: SECURE_CLUSTER_RECONCILER_DISABLED
value: "true"
{{- end }}
image: "{{ .image }}"
imagePullPolicy: IfNotPresent
Expand Down
4 changes: 2 additions & 2 deletions fleetshard/pkg/central/charts/data/rhacs-operator/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ operator:
# images:
# - deploymentName: rhacs-operator-manager-4.0.0
# image: quay.io/rhacs-eng/stackrox-operator:4.0.0
# labelSelector: 4.0.0
# centralLabelSelector: 4.0.0
# - deploymentName: rhacs-operator-manager-4.0.1
# image: quay.io/rhacs-eng/stackrox-operator:4.0.1
# labelSelector: 4.0.1
# centralLabelSelector: 4.0.1
ludydoo marked this conversation as resolved.
Show resolved Hide resolved
images: []
# TODO: add values for resource limits and requests for both proxy and manager container
# TODO: add value for kube-rbac-proxy image
Loading