INTLY-5492 Include stateful sets #32

sergioifg94 · 2020-05-15T14:11:54Z

Description

Link to JIRA: https://issues.redhat.com/browse/INTLY-5492

Create a new controller that watches StatefulSet objects and inspects their images.
Update CLI to generate reports for stateful sets as well.

Factor common logic between controllers into a generic reconciler pkg/controller/generic
that performs the reconcilliation logic delegating the specific logic for each resource
into an injected HeimdallObjectInterface implementation.

Verification steps

You'll need a cluster with RHMI 2.x installation. The Keycloak operator uses stateful sets to deploy the Keycloak pods. The following steps use keycloak to verify this

CLI

Build the CLI as specified in the README
Generate a report for the RHSSO namespace:

./cli -namespaces=redhat-rhmi-rhsso

Verify that the keycloak docker image is shown in the resulting report redhat-sso-7/sso73-openshift

Cluster

Manually deploy the Heimdall operator

oc login...

# prepare cluster
make cluster/prepare

# switch to project
oc project heimdall

# deploy cluster role and cluster role binding
oc create -f deploy/cluster_role.yaml
oc create -f deploy/cluster_role_binding.yaml

# create pull secret - ping for pull secret if required

# update operator deployment to use quay.io/sfrancog/heimdall-operator:master image and pull secret created in previous step
vi ./deploy/operator.yaml

# deploy operator
oc create -f ./deploy/operator.yaml

Once the operator is running, find the Keycloak StatefulSet and add the label heimdall.monitored: 'true' to indicate Heimdall to monitor the StatefulSet
Verify that the StatefulSet is annotated with the heimdall.* annotations
Find the keycloak stateful set pods and verify that heimdall.* labels have been added to them

ImageMonitor verification

Delete the heimdall.monitored label from the StatefulSet and verify that the annotations are removed.
In the same namespace, create an ImageMonitor CR with empty spec (it'll monitor all resources in the namespace)
Verify that the heimdall.monitored label is added to the StatefulSet
Delete the ImageMonitor
Verify that the heimdall.monitored label is deleted automatically

damienomurchu · 2020-05-20T14:19:42Z

Verified cli against a 2.2.0 cluster

CLI built from master:

~/integr8ly/heimdall/cmd/cli ‹master› 
➜ ./cli -namespaces=redhat-rhmi-rhsso                                                                                                                               130 ↵
+-----------+-------+------------+--------------+-----+--------------------+----------------------+------------------+--------------+--------------------+-----------------------------+---------------+----------------+---------------+
| COMPONENT | IMAGE | IMAGE HASH | IMAGE STREAM | TAG | UPTO DATE WITH TAG | PERSISTENT IMAGE TAG | LATEST PATCH TAG | FLOATING TAG | USING FLOATING TAG | UPTO DATE WITH FLOATING TAG | CRITICAL CVES | IMPORTANT CVES | MODERATE CVES |
+-----------+-------+------------+--------------+-----+--------------------+----------------------+------------------+--------------+--------------------+-----------------------------+---------------+----------------+---------------+
+-----------+-------+------------+--------------+-----+--------------------+----------------------+------------------+--------------+--------------------+-----------------------------+---------------+----------------+---------------+

CLI built from PR:

~/integr8ly/heimdall/cmd/cli ‹INTLY-5492› 
➜ ./cli -namespaces=redhat-rhmi-rhsso    
checking image : registry.access.redhat.com/redhat-sso-7/sso73-openshift:1.0-31
+-----------+------------------------------+------------------------------------------------------------------+--------------+--------+--------------------+----------------------+------------------+--------------+--------------------+-----------------------------+---------------+----------------+---------------+
| COMPONENT | IMAGE                        | IMAGE HASH                                                       | IMAGE STREAM | TAG    | UPTO DATE WITH TAG | PERSISTENT IMAGE TAG | LATEST PATCH TAG | FLOATING TAG | USING FLOATING TAG | UPTO DATE WITH FLOATING TAG | CRITICAL CVES | IMPORTANT CVES | MODERATE CVES |
+-----------+------------------------------+------------------------------------------------------------------+--------------+--------+--------------------+----------------------+------------------+--------------+--------------------+-----------------------------+---------------+----------------+---------------+
| keycloak  | redhat-sso-7/sso73-openshift | a3960b56f9fbd2c4da8f5945185517785cffdaa65a3960e6ca27fc16c1ba6b9e | false        | 1.0-31 | true               | 1.0-31               | 1.0-34           | 1.0          | false              | false                       |             0 |             10 |             0 |
+-----------+------------------------------+------------------------------------------------------------------+--------------+--------+--------------------+----------------------+------------------+--------------+--------------------+-----------------------------+---------------+----------------+---------------+

davidffrench

@sergioifg94 Changes look good to me. One inline question on the inclusion of logr.

Are there plans to have the existing controllers use the generic_controller as well? It may not be worth the effort and testing here since Heimdall will likely be deprecated in the future.

davidffrench · 2020-05-25T10:35:58Z

go.mod

@@ -5,18 +5,28 @@ go 1.13
 require (
 	cloud.google.com/go v0.37.4 // indirect
 	github.com/coreos/prometheus-operator v0.34.0
+	github.com/go-logr/logr v0.1.0


This package addition is bringing in additional indirect dependencies and a lot of new packages under the vendor folder. Is there a reason to use this over "sigs.k8s.io/controller-runtime/pkg/runtime/log"?

https://github.com/integr8ly/heimdall/blob/master/pkg/controller/deployments/controller.go#L25

The only reason this package was added was to declare the logr.Logger type as a member of the Reconciler struct: https://github.com/integr8ly/heimdall/pull/32/files#diff-1acebcd3c7162bca673972c513212b1dR52

Variables of this type are being used already but the type was never explicitely declared so I guess that's why it wasn't added to the go.mod until now. Not sure if there's any way around it

Thanks @sergioifg94 . I am not seeing where the logr package was being used directly already. I think that is what the controller-runtime log package might use under the hood. My preference would be to stay consistent with the existing controllers here and use the controller-runtime log package the same as https://github.com/integr8ly/heimdall/blob/master/pkg/controller/deployments/controller.go#L25

@davidffrench That's the function where I got the logr.Logger type. It returns an instance of this type but, because so far it has been used with type inference, the type was never explicitely declared. A workaround for this is to let the generic controller create the logger and receive the name so we can avoid importing the logr package and keep using it

I think it is fine to allow the type inference. You can continue using the controller-runtime abstraction on logr as is used in other controllers. The package is still included in our vendor folder as it is a dependency of controller-runtime log package. https://github.com/integr8ly/heimdall/tree/master/vendor/github.com/go-logr

@davidffrench Just updated the PR. Had to create an interface with the same methods and fortunately go's type system is smart enough to validate that logr.Logger is assignable to that interface even though they have a different name, so I don't have to explicitly import the package

sergioifg94 · 2020-05-25T11:04:36Z

About the generic controller. I initially thought of refactoring the existing controllers as part of the ticket, but it was a lot of extra work so though of creating a follow up ticket when this is merged. But as you say, if Heimdall is being deprecated there is no point

damienomurchu · 2020-05-25T14:01:43Z

/lgtm

Successfully verified on a cluster - stateful set annotated, and stateful set pods labelled

KevFan · 2020-05-25T14:09:48Z

I had a quick look at this but I think you might have missed some change for the ImageMonitor controller to label the statefulsets also once an ImageMonitor CR is created in that particular namespace

heimdall/pkg/controller/imagemonitor/controller.go

Line 107 in 70ec4f6

    
           if err := r.objectLabeler.LabelAllDeploymentsAndDeploymentConfigs(ctx, map[string]string{domain.HeimdallMonitored: "true"}, imageMon.Spec.ExcludePattern, imageMon.Namespace); err != nil {

Currently, I think it would just label the deployment / deployment configs -> pods from that particular namespace 🤔

heimdall/pkg/cluster/deployments_deployment_configs.go

Line 24 in 70ec4f6

    
           func (ol *ObjectsLabeler) LabelAllDeploymentsAndDeploymentConfigs(ctx context.Context, labels map[string]string, excludePattern string, ns string) error {

sergioifg94 · 2020-05-25T14:16:51Z

Good spot @KevFan , I wasn't aware of that part. I'll have a look at including stateful sets

Create a new controller that watches StatefulSet objects and inspects their images. Update CLI to generate reports for stateful sets as well. Update ImageMonitor controller to label stateful sets as well. Factor common logic between controllers into a generic reconciler `pkg/controller/generic` that performs the reconcilliation logic delegating the specific logic for each resource into an injected `HeimdallObjectInterface` implementation.

davidffrench · 2020-05-25T15:40:41Z

/lgtm

sergioifg94 · 2020-05-25T15:42:32Z

Just pushed the changes mentioned by @KevFan. Verified it myself on my own cluster but added the verification steps here if anyone wants to double check

davidffrench · 2020-05-26T08:09:31Z

/approve

davidffrench · 2020-05-26T09:47:56Z

@sergioifg94 This needs approval from one of the Heimdall approvers - https://github.com/integr8ly/heimdall/blob/master/OWNERS

sergioifg94 · 2020-05-26T09:50:23Z

@davidffrench Thanks for the heads up, will ping them 👍

mikenairn · 2020-05-26T10:13:05Z

Changes look fine to me. Just double checking before i approve that we are happy this will work on OpenShift 3 (It's installed as part of the current 1.x RHMI clusters https://github.com/integr8ly/installation/blob/master/inventories/group_vars/all/manifest.yaml#L137), i can't see why it wouldn't, but it doesn't look like we tested it.

sergioifg94 · 2020-05-26T10:34:38Z

@mikenairn Good point, I'll do some testing on a 1.x cluster. Thanks!

sergioifg94 · 2020-05-27T09:55:07Z

@mikenairn Took me a bit but just verified on a 1.x installation that Heimdall still works. Edited the URL where Heimdall resources are downloaded to point to a branch in my fork that uses my image. The operator starts successfully and monitors the resources as expected.

mikenairn · 2020-05-27T10:39:01Z

/approve

openshift-ci-robot · 2020-05-27T10:39:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidffrench, mikenairn

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~~ [mikenairn]

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

davidffrench reviewed May 25, 2020

View reviewed changes

sergioifg94 force-pushed the INTLY-5492 branch from dd691bb to f81c496 Compare May 25, 2020 13:55

openshift-ci-robot assigned damienomurchu May 25, 2020

openshift-ci-robot added the lgtm label May 25, 2020

sergioifg94 force-pushed the INTLY-5492 branch from f81c496 to f54ca78 Compare May 25, 2020 15:38

openshift-ci-robot removed the lgtm label May 25, 2020

openshift-ci-robot assigned davidffrench May 25, 2020

openshift-ci-robot added the lgtm label May 25, 2020

openshift-ci-robot added the approved label May 27, 2020

openshift-merge-robot merged commit bce0728 into integr8ly:master May 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INTLY-5492 Include stateful sets #32

INTLY-5492 Include stateful sets #32

sergioifg94 commented May 15, 2020 •

edited

damienomurchu commented May 20, 2020 •

edited

davidffrench left a comment

davidffrench May 25, 2020

sergioifg94 May 25, 2020

davidffrench May 25, 2020

sergioifg94 May 25, 2020

davidffrench May 25, 2020

sergioifg94 May 25, 2020

sergioifg94 commented May 25, 2020

damienomurchu commented May 25, 2020

KevFan commented May 25, 2020

sergioifg94 commented May 25, 2020

davidffrench commented May 25, 2020

sergioifg94 commented May 25, 2020

davidffrench commented May 26, 2020

davidffrench commented May 26, 2020

sergioifg94 commented May 26, 2020

mikenairn commented May 26, 2020 •

edited

sergioifg94 commented May 26, 2020

sergioifg94 commented May 27, 2020

mikenairn commented May 27, 2020

openshift-ci-robot commented May 27, 2020

INTLY-5492 Include stateful sets #32

INTLY-5492 Include stateful sets #32

Conversation

sergioifg94 commented May 15, 2020 • edited

Description

Verification steps

CLI

Cluster

ImageMonitor verification

damienomurchu commented May 20, 2020 • edited

davidffrench left a comment

Choose a reason for hiding this comment

davidffrench May 25, 2020

Choose a reason for hiding this comment

sergioifg94 May 25, 2020

Choose a reason for hiding this comment

davidffrench May 25, 2020

Choose a reason for hiding this comment

sergioifg94 May 25, 2020

Choose a reason for hiding this comment

davidffrench May 25, 2020

Choose a reason for hiding this comment

sergioifg94 May 25, 2020

Choose a reason for hiding this comment

sergioifg94 commented May 25, 2020

damienomurchu commented May 25, 2020

KevFan commented May 25, 2020

sergioifg94 commented May 25, 2020

davidffrench commented May 25, 2020

sergioifg94 commented May 25, 2020

davidffrench commented May 26, 2020

davidffrench commented May 26, 2020

sergioifg94 commented May 26, 2020

mikenairn commented May 26, 2020 • edited

sergioifg94 commented May 26, 2020

sergioifg94 commented May 27, 2020

mikenairn commented May 27, 2020

openshift-ci-robot commented May 27, 2020

sergioifg94 commented May 15, 2020 •

edited

damienomurchu commented May 20, 2020 •

edited

mikenairn commented May 26, 2020 •

edited