Skip to content

ROX-13943: Hide "sourced" autogenerated image integrations behind a feature flag #4160

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.

Sign up for GitHub

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

Merged
RTann merged 9 commits into from
Dec 15, 2022
Merged

ROX-13943: Hide "sourced" autogenerated image integrations behind a feature flag #4160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c461758
update
RTann Dec 15, 2022
402359c
updates
RTann Dec 15, 2022
1d67722
flip
RTann Dec 15, 2022
53652ca
style
RTann Dec 15, 2022
3f4fdd9
Merge branch 'master' into ROX-13943
RTann Dec 15, 2022
403135f
PR updates
RTann Dec 15, 2022
45ec03d
forgot .0
RTann Dec 15, 2022
754801c
add another log
RTann Dec 15, 2022
30c8e64
Update central/imageintegration/datastore/singleton.go
RTann Dec 15, 2022
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
15 changes: 15 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,21 @@ Please avoid adding duplicate information across this changelog and JIRA/doc inp

### Technical Changes

## [3.73.1]

### Added Features

### Removed Features

### Deprecated Features

### Technical Changes
3.73.0 introduced a change to ACS autogenerated image integration workflows.
However, this change in workflow caused Central to take too long on startup (details [here](https://access.redhat.com/node/6990153)).
To fix the issue introduced in 3.73.0, 3.73.1 will reinstate the old workflow.
Therefore, autogenerated integrations may not work successfully in environments with various credentials
used for multiple repos within a global registry.

## [3.73.0]
### Removed Features
- ROX-12839: we will stop shipping the docs embedded in the product, starting with the release following this one (docs will still be available online)
Expand Down
5 changes: 5 additions & 0 deletions central/enrichment/singleton.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
v1 "github.com/stackrox/rox/generated/api/v1"
"github.com/stackrox/rox/pkg/env"
"github.com/stackrox/rox/pkg/expiringcache"
"github.com/stackrox/rox/pkg/features"
imageEnricher "github.com/stackrox/rox/pkg/images/enricher"
"github.com/stackrox/rox/pkg/metrics"
nodeEnricher "github.com/stackrox/rox/pkg/nodes/enricher"
Expand Down Expand Up @@ -70,6 +71,10 @@ func initializeManager() {
return
}
for _, ii := range integrations {
// Only upsert autogenerated integrations with a source if the feature is enabled.
if !features.SourcedAutogeneratedIntegrations.Enabled() && ii.GetAutogenerated() && ii.GetSource() != nil {
continue
}
Comment on lines +74 to +77
Copy link
Copy Markdown
Member

@rukletsov rukletsov Dec 15, 2022

Choose a reason for hiding this comment

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

My mental boolean compiler claims the code and the comment do not quite match. Applying De Morgan's rule:

		if features.SourcedAutogeneratedIntegrations.Enabled() || !ii.GetAutogenerated() || ii.GetSource() == nil {
			//... upsert
		}

This reads to me as "Upsert all if feature is enabled, otherwise skip autogenerated and those with non-nil source". The autogenerated with non-nil source indeed will be inserted only when feature is enabled but what about non-autogenerated with non-nil source?

Copy link
Copy Markdown
Contributor Author

@RTann RTann Dec 15, 2022

Choose a reason for hiding this comment

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

The Source field was added into 73 and its addition has caused some issues. The goal is to ensure when the feature flag is turned off, we revert back to the old implementation, which inserted all autogenerated registries. The Source field should not be populated when the flag is off, so this check is meant to ensure that, if it did somehow pass through, only registries without Source are upserted

if err := manager.Upsert(ii); err != nil {
log.Errorf("unable to use previous integration %s: %v", ii.GetName(), err)
}
Expand Down
17 changes: 8 additions & 9 deletions central/image/service/service_impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/stackrox/rox/pkg/env"
"github.com/stackrox/rox/pkg/errox"
"github.com/stackrox/rox/pkg/expiringcache"
"github.com/stackrox/rox/pkg/features"
"github.com/stackrox/rox/pkg/grpc/authz"
"github.com/stackrox/rox/pkg/grpc/authz/idcheck"
"github.com/stackrox/rox/pkg/grpc/authz/or"
Expand Down Expand Up @@ -267,21 +268,19 @@ func (s *serviceImpl) ScanImageInternal(ctx context.Context, request *v1.ScanIma
// image data (i.e. scan results, signature data etc.).
func (s *serviceImpl) enrichImage(ctx context.Context, img *storage.Image, fetchOpt enricher.FetchOption,
requestSource *v1.ScanImageInternalRequest_Source) error {
var source *enricher.RequestSource
if requestSource != nil {
source = &enricher.RequestSource{
enrichmentContext := enricher.EnrichmentContext{
FetchOpt: fetchOpt,
Internal: true,
}

if features.SourcedAutogeneratedIntegrations.Enabled() && requestSource != nil {
enrichmentContext.Source = &enricher.RequestSource{
ClusterID: requestSource.GetClusterId(),
Namespace: requestSource.GetNamespace(),
ImagePullSecrets: set.NewStringSet(requestSource.GetImagePullSecrets()...),
}
}

enrichmentContext := enricher.EnrichmentContext{
FetchOpt: fetchOpt,
Internal: true,
Source: source,
}

if _, err := s.enricher.EnrichImage(ctx, enrichmentContext, img); err != nil {
log.Errorf("error enriching image %q: %v", img.GetName().GetFullName(), err)
// Purposefully, don't return here because we still need to save it into the DB so there is a reference
Expand Down
3 changes: 2 additions & 1 deletion central/imageintegration/datastore/datastore_impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
v1 "github.com/stackrox/rox/generated/api/v1"
"github.com/stackrox/rox/generated/storage"
"github.com/stackrox/rox/pkg/env"
"github.com/stackrox/rox/pkg/features"
"github.com/stackrox/rox/pkg/sac"
searchPkg "github.com/stackrox/rox/pkg/search"
"github.com/stackrox/rox/pkg/uuid"
Expand Down Expand Up @@ -78,7 +79,7 @@ func (ds *datastoreImpl) AddImageIntegration(ctx context.Context, integration *s
return "", sac.ErrResourceAccessDenied
}

if integration.GetId() == "" {
if !features.SourcedAutogeneratedIntegrations.Enabled() || integration.GetId() == "" {
integration.Id = uuid.NewV4().String()
}
err := ds.storage.Upsert(ctx, integration)
Expand Down
29 changes: 27 additions & 2 deletions central/imageintegration/datastore/singleton.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/stackrox/rox/central/imageintegration/store/bolt"
"github.com/stackrox/rox/central/imageintegration/store/postgres"
"github.com/stackrox/rox/pkg/env"
"github.com/stackrox/rox/pkg/features"
"github.com/stackrox/rox/pkg/sac"
"github.com/stackrox/rox/pkg/sync"
"github.com/stackrox/rox/pkg/utils"
Expand All @@ -22,16 +23,40 @@ var (
dataStore DataStore
)

func initializeDefaultIntegrations(storage store.Store) {
func initializeIntegrations(storage store.Store) {
ctx := sac.WithGlobalAccessScopeChecker(context.Background(), sac.AllowAllAccessScopeChecker())
iis, err := storage.GetAll(ctx)
utils.CrashOnError(err)
// If we are starting from scratch in online-mode, add the default image integrations.
if !env.OfflineModeEnv.BooleanSetting() && len(iis) == 0 {
// Add default integrations
for _, ii := range store.DefaultImageIntegrations {
utils.Must(storage.Upsert(ctx, ii))
}
}

Comment thread
RTann marked this conversation as resolved.
// If the feature flag is disabled, remove all "sourced" autogenerated registries from the datastore.
if !features.SourcedAutogeneratedIntegrations.Enabled() {
if len(iis) > 0 {
log.Infof("[STARTUP] Starting deletion of 'sourced' image integrations")
}

var attempted, deleted int
for _, ii := range iis {
if ii.GetAutogenerated() && ii.GetSource() != nil {
Copy link
Copy Markdown
Contributor

@dhaus67 dhaus67 Dec 15, 2022

Choose a reason for hiding this comment

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

When there's customer who migrate from 3.72.0 -> 3.73.0, they will have the mix of autogenerated integrations with Source != nil and Source == nil.

In case of a "clean" installation of 3.73.0, all autogenerated image integrations will have Source != nil set.

So, we will delete all autogenerated image integrations then.

In that case, how will the recovery look like for this? Will it be eventual consistent such that the autogenerated registries we expect to have (i.e. for internal OpenShift registry) will show up after some time, or does that require bumps of sensors to send the autogenerated registries once more?

(I honestly don't think that the OpenShift internal ones are that important from an autogenerated image integration perspective, unless central can reach them, which won't be the case for most of them)

Copy link
Copy Markdown
Contributor Author

@RTann RTann Dec 15, 2022

Choose a reason for hiding this comment

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

In the case of a clean install, I'd expect us to eventually find all the autogenerated registries. When it comes to OpenShift internal registry, we do not use the stuff sent to Central for that

attempted++
// Use Should so release versions do not panic.
if err := utils.ShouldErr(storage.Delete(ctx, ii.GetId())); err != nil {
deleted++
}
}
}
Comment thread
RTann marked this conversation as resolved.
if attempted > 0 {
log.Infof("Successfully deleted %d out of %d image integration(s)", deleted, attempted)
}

log.Info("Completed deletion of 'sourced' image integrations")
}
}

func initialize() {
Expand All @@ -46,7 +71,7 @@ func initialize() {
storage = bolt.New(globaldb.GetGlobalDB())
indexer = index.New(globalindex.GetGlobalTmpIndex())
}
initializeDefaultIntegrations(storage)
initializeIntegrations(storage)
searcher := search.New(storage, indexer)
dataStore = New(storage, indexer, searcher)
}
Expand Down
11 changes: 9 additions & 2 deletions central/sensor/service/pipeline/imageintegrations/pipeline.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/stackrox/rox/pkg/centralsensor"
"github.com/stackrox/rox/pkg/env"
"github.com/stackrox/rox/pkg/errox"
"github.com/stackrox/rox/pkg/features"
"github.com/stackrox/rox/pkg/logging"
"github.com/stackrox/rox/pkg/metrics"
"github.com/stackrox/rox/pkg/set"
Expand Down Expand Up @@ -65,6 +66,10 @@ type pipelineImpl struct {
}

func (s *pipelineImpl) Reconcile(ctx context.Context, clusterID string, storeMap *reconciliation.StoreMap) error {
if !features.SourcedAutogeneratedIntegrations.Enabled() {
return nil
}

existingIDs := set.NewStringSet()

conn := connection.FromContext(ctx)
Expand Down Expand Up @@ -247,7 +252,8 @@ func (s *pipelineImpl) Run(ctx context.Context, clusterID string, msg *central.M
}
defer countMetrics.IncrementResourceProcessedCounter(pipeline.ActionToOperation(msg.GetEvent().GetAction()), metrics.ImageIntegration)

if msg.GetEvent().GetAction() == central.ResourceAction_REMOVE_RESOURCE {
if features.SourcedAutogeneratedIntegrations.Enabled() && msg.GetEvent().GetAction() == central.ResourceAction_REMOVE_RESOURCE {
// Remove is only supported if the feature flag is enabled.
return s.runRemove(ctx, msg.GetEvent().GetImageIntegration().GetId())
}

Expand All @@ -269,7 +275,8 @@ func (s *pipelineImpl) Run(ctx context.Context, clusterID string, msg *central.M
return errors.Wrapf(err, "setting up integration params for %q", imageIntegration.GetId())
}
source := imageIntegration.GetSource()
if source == nil {
if !features.SourcedAutogeneratedIntegrations.Enabled() || source == nil {
Copy link
Copy Markdown
Member

@rukletsov rukletsov Dec 15, 2022

Choose a reason for hiding this comment

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

I thought we don't speak about autogenerated when the flag is disabled?

Copy link
Copy Markdown
Contributor Author

@RTann RTann Dec 15, 2022

Choose a reason for hiding this comment

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

This flag is specifically for the new version of autogenerated registries introduced in 3.73.0: ones with this Source field. We have a separate environment setting (not feature flag) to ignore all autogenerated registry integrations: https://github.com/stackrox/stackrox/blob/master/pkg/env/autogenerated_registries.go

imageIntegration.Name = fmt.Sprintf("Autogenerated %s for cluster %s", description, clusterName)
return s.legacyRun(ctx, imageIntegration, matches)
}
imageIntegration.Name = fmt.Sprintf("Autogenerated %s for cluster %s from %s/%s",
Expand Down
3 changes: 3 additions & 0 deletions pkg/features/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,7 @@ var (

// RoxSyslogExtraFields enables user to add additional key value pairs in syslog alert notification in cef format
RoxSyslogExtraFields = registerFeature("Enable extra fields for syslog integration", "ROX_SYSLOG_EXTRA_FIELDS", false)

// SourcedAutogeneratedIntegrations enables adding a "source" to autogenerated integrations.
SourcedAutogeneratedIntegrations = registerFeature("Enable autogenerated integrations with cluster/namespace/secret source", "ROX_SOURCED_AUTOGENERATED_INTEGRATIONS", false)
)
21 changes: 18 additions & 3 deletions pkg/images/enricher/enricher_impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/stackrox/rox/pkg/errorhelpers"
"github.com/stackrox/rox/pkg/errox"
"github.com/stackrox/rox/pkg/expiringcache"
"github.com/stackrox/rox/pkg/features"
"github.com/stackrox/rox/pkg/images/integration"
"github.com/stackrox/rox/pkg/images/utils"
"github.com/stackrox/rox/pkg/integrationhealth"
Expand Down Expand Up @@ -296,9 +297,13 @@ func (e *enricherImpl) enrichWithMetadata(ctx context.Context, enrichmentContext
e.errorsPerRegistry[registry] = 0
})
}
id, name := registry.DataSource().GetId(), registry.DataSource().GetName()
if features.SourcedAutogeneratedIntegrations.Enabled() {
id, name = registry.Source().GetId(), registry.Source().GetName()
}
e.integrationHealthReporter.UpdateIntegrationHealthAsync(&storage.IntegrationHealth{
Id: registry.Source().GetId(),
Name: registry.Source().GetName(),
Id: id,
Name: name,
Type: storage.IntegrationHealth_IMAGE_INTEGRATION,
Status: storage.IntegrationHealth_HEALTHY,
LastTimestamp: timestamp.TimestampNow(),
Expand Down Expand Up @@ -343,7 +348,10 @@ func (e *enricherImpl) enrichImageWithRegistry(ctx context.Context, image *stora
if err != nil {
return false, errors.Wrapf(err, "getting metadata from registry: %q", registry.Name())
}
metadata.DataSource = imageIntegrationToDataSource(registry.Source())
metadata.DataSource = registry.DataSource()
if features.SourcedAutogeneratedIntegrations.Enabled() {
metadata.DataSource = imageIntegrationToDataSource(registry.Source())
}
metadata.Version = metadataVersion
image.Metadata = metadata

Expand Down Expand Up @@ -681,6 +689,9 @@ func isOpenshiftGlobalPullSecret(source *storage.ImageIntegration_Source) bool {
func (e *enricherImpl) getRegistriesForContext(ctx EnrichmentContext) ([]registryTypes.ImageRegistry, error) {
registries := e.integrations.RegistrySet().GetAll()
if ctx.Internal {
if !features.SourcedAutogeneratedIntegrations.Enabled() {
return registries, nil
}
if ctx.Source == nil {
return registries, nil
}
Expand All @@ -699,6 +710,10 @@ func (e *enricherImpl) getRegistriesForContext(ctx EnrichmentContext) ([]registr
// 2. If the integration's source matches with the EnrichmentContext.Source
// Note that this function WILL modify the input array.
func filterRegistriesBySource(requestSource *RequestSource, registries []registryTypes.ImageRegistry) {
if !features.SourcedAutogeneratedIntegrations.Enabled() {
return
}

filteredRegistries := registries[:0]
for _, registry := range registries {
integration := registry.Source()
Expand Down
13 changes: 11 additions & 2 deletions pkg/registries/factory_impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@ type factoryImpl struct {

type registryWithDataSource struct {
types.Registry
source *storage.ImageIntegration
datasource *storage.DataSource
source *storage.ImageIntegration
}

func (r *registryWithDataSource) DataSource() *storage.DataSource {
return r.datasource
}

func (r *registryWithDataSource) Source() *storage.ImageIntegration {
Expand All @@ -32,6 +37,10 @@ func (e *factoryImpl) CreateRegistry(source *storage.ImageIntegration) (types.Im

return &registryWithDataSource{
Registry: integration,
source: source,
datasource: &storage.DataSource{
Id: source.GetId(),
Name: source.GetName(),
},
source: source,
}, nil
}
4 changes: 4 additions & 0 deletions pkg/registries/set_impl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ func (f fakeRegistry) Name() string {
return f.name
}

func (f fakeRegistry) DataSource() *storage.DataSource {
return nil
}

func (f fakeRegistry) Source() *storage.ImageIntegration {
return nil
}
Expand Down
1 change: 1 addition & 0 deletions pkg/registries/types/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ type Registry interface {
// integration formed the interface
type ImageRegistry interface {
Registry
DataSource() *storage.DataSource
Source() *storage.ImageIntegration
}

Expand Down
6 changes: 4 additions & 2 deletions qa-tests-backend/src/test/groovy/ImageScanningTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,7 +621,8 @@ class ImageScanningTest extends BaseSpecification {
}

private static String source(String server) {
return "Autogenerated ${server} for cluster ${DEFAULT_CLUSTER_NAME} from.*"
// TODO: append " from .*" once SourcedAutogeneratedIntegrations is enabled.
return "Autogenerated ${server} for cluster ${DEFAULT_CLUSTER_NAME}"
}

@Unroll
Expand Down Expand Up @@ -802,8 +803,9 @@ class ImageScanningTest extends BaseSpecification {
private static String expectAutoGeneratedRegistry(Secret secret) {
ImageIntegrationOuterClass.ImageIntegration autoGenerated = null
withRetry(5, 2) {
// TODO: append " from ${secret.namespace}/${secret.name}" once SourcedAutogeneratedIntegrations is enabled.
autoGenerated = ImageIntegrationService.getImageIntegrationByName(
"Autogenerated ${secret.server} for cluster ${DEFAULT_CLUSTER_NAME} from ${secret.namespace}/${secret.name}"
"Autogenerated ${secret.server} for cluster ${DEFAULT_CLUSTER_NAME}"
)
assert autoGenerated
}
Expand Down
23 changes: 16 additions & 7 deletions sensor/common/detector/enricher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/stackrox/rox/pkg/booleanpolicy/augmentedobjs"
"github.com/stackrox/rox/pkg/concurrency"
"github.com/stackrox/rox/pkg/expiringcache"
"github.com/stackrox/rox/pkg/features"
"github.com/stackrox/rox/pkg/images/types"
"github.com/stackrox/rox/pkg/protoutils"
"github.com/stackrox/rox/pkg/set"
Expand Down Expand Up @@ -61,14 +62,18 @@ func scanImage(ctx context.Context, svc v1.ImageServiceClient, req *scanImageReq
ctx, cancel := context.WithTimeout(ctx, scanTimeout)
defer cancel()

return svc.ScanImageInternal(ctx, &v1.ScanImageInternalRequest{
internalReq := &v1.ScanImageInternalRequest{
Image: req.containerImage,
Source: &v1.ScanImageInternalRequest_Source{
}
if features.SourcedAutogeneratedIntegrations.Enabled() {
internalReq.Source = &v1.ScanImageInternalRequest_Source{
ClusterId: req.clusterID,
Namespace: req.namespace,
ImagePullSecrets: req.pullSecrets,
},
})
}
}

return svc.ScanImageInternal(ctx, internalReq)
}

func scanImageLocal(ctx context.Context, svc v1.ImageServiceClient, req *scanImageRequest) (*v1.ScanImageInternalResponse, error) {
Expand Down Expand Up @@ -218,15 +223,19 @@ func (e *enricher) runImageScanAsync(imageChan chan<- imageChanResult, req *scan
func (e *enricher) getImages(deployment *storage.Deployment) []*storage.Image {
imageChan := make(chan imageChanResult, len(deployment.GetContainers()))

pullSecretSet := set.NewStringSet(e.serviceAccountStore.GetImagePullSecrets(deployment.GetNamespace(), deployment.GetServiceAccount())...)
pullSecretSet.AddAll(deployment.GetImagePullSecrets()...)
var pullSecrets []string
if features.SourcedAutogeneratedIntegrations.Enabled() {
pullSecretsSet := set.NewStringSet(e.serviceAccountStore.GetImagePullSecrets(deployment.GetNamespace(), deployment.GetServiceAccount())...)
pullSecretsSet.AddAll(deployment.GetImagePullSecrets()...)
pullSecrets = pullSecretsSet.AsSlice()
}
for idx, container := range deployment.GetContainers() {
e.runImageScanAsync(imageChan, &scanImageRequest{
containerIdx: idx,
containerImage: container.GetImage(),
clusterID: clusterid.Get(),
namespace: deployment.GetNamespace(),
pullSecrets: pullSecretSet.AsSlice(),
pullSecrets: pullSecrets,
})
}
images := make([]*storage.Image, len(deployment.GetContainers()))
Expand Down
Loading