feat: Allow return the client without performing a matchRepository

[jsolana]

Closes #17948

Modify the behavior during DetectConfigManagementPlugin to avoid transferring the entire repository to cmp-server here
Use a new pre-flight operation to check if discovery is enabled or not.
If discover is not configured, returns the client without without further checks

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Toolchain Guide
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.
• Optional. My organization is added to USERS.md.
• Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

[crenshaw-dev]

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.

[jsolana]

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

[jsolana]

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?

[crenshaw-dev]

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.

[jsolana]

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

[jsolana]

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

[codecov]

Codecov Report

Attention: Patch coverage is 43.75000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 45.08%. Comparing base (6f76e71) to head (4f9df08).

Files	Patch %	Lines
util/app/discovery/discovery.go	0.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #18053      +/-   ##
==========================================
- Coverage   45.08%   45.08%   -0.01%     
==========================================
  Files         354      354              
  Lines       48006    48022      +16     
==========================================
+ Hits        21644    21651       +7     
- Misses      23547    23556       +9     
  Partials     2815     2815              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[crenshaw-dev]

Can we drop these fields? I don't think they're needed. I'll put up another PR to remove them from the unrelated lines.

[jsolana]

do you mean

cfg, err := cmpClient.CheckPluginConfiguration(ctx, &empty.Empty{})
	if err != nil {
		log.Errorf("error checking plugin configuration %s, %v", fileName, err)
		return nil, nil, false
        }

correct?
Why?