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.

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

Jump to bottom

chore: Allow return the client without performing a matchRepository #18053

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+409 −38
Open

chore: Allow return the client without performing a matchRepository #18053

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
18ab91b
return the client without performing a matchRepository operation when…
May 2, 2024
be9f111
Merge remote-tracking branch 'upstream/master' into js/enhance-plugin…
Jun 12, 2024
f0ded6c
add CheckPluginConfiguration pre-flight operation
Jun 13, 2024
2d1bc11
fix lint
Jun 13, 2024
0602051
Missing comment
Jun 13, 2024
4f9df08
Merge branch 'master' into js/enhance-plugin-evaluation
jsolana Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 6 additions & 5 deletions util/app/discovery/discovery.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,12 @@ func cmpSupports(ctx context.Context, pluginSockFilePath, appPath, repoPath, fil
return nil, nil, false
}

isSupported, isDiscoveryEnabled, err := matchRepositoryCMP(ctx, appPath, repoPath, cmpClient, env, tarExcludedGlobs)
// if plugin name is specified, lets return the client directly
if namedPlugin {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we still only want to do this if discovery is not enabled, since the CMP author might be using discovery as a sort of safety or even security mechanism.

We probably need a pre-flight request to get isDiscoveryEnabled without sending the whole repo. That's a somewhat more involved change, since it'll introduce a new API endpoint.

Copy link
Author

Choose a reason for hiding this comment

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

Make a lot of sense, let me update the MR
Thanks a lot for your feedback!

Copy link
Author

@jsolana jsolana Jun 12, 2024 •
edited

Choose a reason for hiding this comment

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

What do you think @crenshaw-dev ?

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
}

Wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the rough idea makes sense. I'm not sure it would need to be a streaming request. If I understand correctly, we wouldn't need to send any streaming contents to the CMP.

Copy link
Author

@jsolana jsolana Jun 12, 2024 •
edited

Choose a reason for hiding this comment

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

Yep I was thinking the same, I ve mimic other operations but no strong opinion about stream vs unary op in this moment 😅
Gonna update it to use unary call
Thanks mate

Copy link
Author

@jsolana jsolana Jun 13, 2024 •
edited

Choose a reason for hiding this comment

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

Done! Let me know wdyt! (also title and description of the PR was updated)
Thanks for all your support @crenshaw-dev :)

return conn, cmpClient, true
}

isSupported, _, err := matchRepositoryCMP(ctx, appPath, repoPath, cmpClient, env, tarExcludedGlobs)
if err != nil {
log.WithFields(log.Fields{
common.SecurityField: common.SecurityMedium,
Expand All @@ -176,10 +181,6 @@ func cmpSupports(ctx context.Context, pluginSockFilePath, appPath, repoPath, fil
}

if !isSupported {
// if discovery is not set and the plugin name is specified, let app use the plugin
if !isDiscoveryEnabled && namedPlugin {
return conn, cmpClient, true
}
log.WithFields(log.Fields{
common.SecurityField: common.SecurityLow,
common.SecurityCWEField: common.SecurityCWEMissingReleaseOfFileDescriptor,
Expand Down