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

Enhance CMP Plugin Evaluation for Applications with Name-based Configuration #17948

Closed
jsolana opened this issue Apr 24, 2024 · 2 comments · Fixed by #18053
Closed

Enhance CMP Plugin Evaluation for Applications with Name-based Configuration #17948

jsolana opened this issue Apr 24, 2024 · 2 comments · Fixed by #18053
Labels
enhancement New feature or request

Comments

@jsolana
Copy link
Contributor

jsolana commented Apr 24, 2024

Summary

Modify the behavior during cmp plugin evaluation (DetectConfigManagementPlugin) to avoid transferring the entire repository to cmp-server when the application configures the plugin by name.

Motivation

In scenarios involving monorepos and/or multiple clusters, transferring the entire repository multiple times solely for verifying the correct configuration of the plugin can lead to disk usage bottlenecks impacting performance and resource utilization:

image

Proposal

Instead of transferring the entire repository during plugin evaluation, consider adjusting the implementation to return the client (cmpClient) without performing a matchRepository operation when the application'plugin be configured by its.

utils/app/discovery/discovery.go

func cmpSupports(ctx context.Context, pluginSockFilePath, appPath, repoPath, fileName string, env []string, tarExcludedGlobs []string, namedPlugin bool) (io.Closer, pluginclient.ConfigManagementPluginServiceClient, bool) {
	absPluginSockFilePath, err := filepath.Abs(pluginSockFilePath)
	if err != nil {
		log.Errorf("error getting absolute path for plugin socket dir %v, %v", pluginSockFilePath, err)
		return nil, nil, false
	}
	address := filepath.Join(absPluginSockFilePath, fileName)
	if !files.Inbound(address, absPluginSockFilePath) {
		log.Errorf("invalid socket file path, %v is outside plugin socket dir %v", fileName, pluginSockFilePath)
		return nil, nil, false
	}

	cmpclientset := pluginclient.NewConfigManagementPluginClientSet(address)

	conn, cmpClient, err := cmpclientset.NewConfigManagementPluginClient()
	if err != nil {
		log.WithFields(log.Fields{
			common.SecurityField:    common.SecurityMedium,
			common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor,
		}).Errorf("error dialing to cmp-server for plugin %s, %v", fileName, err)
		return nil, nil, false
	}
	
        // if plugin name is specified, lets return the client directly
	if namedPlugin {
		return conn, cmpClient, true
	}

	isSupported, _, err := matchRepositoryCMP(ctx, appPath, repoPath, cmpClient, env, tarExcludedGlobs)
	if err != nil {
		log.WithFields(log.Fields{
			common.SecurityField:    common.SecurityMedium,
			common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor,
		}).Errorf("repository %s is not the match because %v", repoPath, err)
		io.Close(conn)
		return nil, nil, false
	}

	if !isSupported {
		log.WithFields(log.Fields{
			common.SecurityField:    common.SecurityLow,
			common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor,
		}).Debugf("Reponse from socket file %s does not support %v", fileName, repoPath)
		io.Close(conn)
		return nil, nil, false
	}
	return conn, cmpClient, true
}
@jsolana jsolana added the enhancement New feature or request label Apr 24, 2024
@jsolana jsolana changed the title Avoid matchRepository if plugin Enhance CMP Plugin Evaluation for Applications with Name-based Configuration Apr 24, 2024
@jsolana
Copy link
Contributor Author

jsolana commented Apr 24, 2024

If it is ok, I can create the MR :)

@jsolana
Copy link
Contributor Author

jsolana commented Jun 20, 2024

Moving the discussion and current conclusions from #18053 for visibility

Makes more sense provide a new pre-flight operation to check the plugin configuration (and check if discovery is enabled or not)

plugin.proto

message CheckPluginConfigurationResponse {
    bool isDiscoveryConfigured = 1;
}


...

// CheckPluginConfiguration is a pre-flight request  to check the plugin configuration
    // without sending the whole repo.
    rpc CheckPluginConfiguration(stream AppStreamRequest) returns (CheckPluginConfigurationResponse) {
    }

In that case isDiscoveryConfigured will be true if there is any discovery.FileName or discovery.Find info (in this case we won't propagate repo info, then, no pattern matching will be executed)

plugin.go

type CheckPluginConfigurationStream interface {
	Stream
	SendAndClose(response *apiclient.CheckPluginConfigurationResponse) error
}

func (s *Service) CheckPluginConfiguration(stream apiclient.ConfigManagementPluginService_CheckPluginConfigurationServer) error {
	return s.checkPluginConfigurationGeneric(stream)
}

func (s *Service) checkPluginConfigurationGeneric(stream CheckPluginConfigurationStream) error {
	isDiscoveryConfigured := s.isDiscoveryConfigured()
	repoResponse := &apiclient.CheckPluginConfigurationResponse{IsDiscoveryConfigured: isDiscoveryConfigured}

	err := stream.SendAndClose(repoResponse)
	if err != nil {
		return fmt.Errorf("error sending check plugin configuration response: %w", err)
	}
	return nil
}

func (s *Service) isDiscoveryConfigured() (IsDiscoveryConfigured bool) {
	config := s.initConstants.PluginConfig
	return config.Spec.Discover.FileName != "" || config.Spec.Discover.Find.Glob != "" || len(config.Spec.Discover.Find.Command.Command) > 0
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant