Read the Pi hostname from config instead of rule_type #1313

jhrozek · 2023-10-27T15:45:47Z

This PR adds a policy engine configuration struct that can be set in the config
file or through an environment variable. The only use now is to override the Pi
hostname so that our staging instance can default to staging Pi and production
can default to production Pi. In Kube, the easiest way to set the Pi hostname is
probably through enviornment variables, you can test that with:

MEDIATOR_ENGINE_EVAL_PACKAGE_INTELLIGENCE_ENDPOINT=http://my.little.pi.server go run cmd/server/main.go serve

Fixes: #1276

JAORMX · 2023-10-27T18:17:12Z

I was thinking more along the lines of having pi be handled via a provider, and configuring it from there instead.

jhrozek · 2023-10-27T19:18:00Z

I was thinking more along the lines of having pi be handled via a provider, and configuring it from there instead.

interesting idea, let me simmer on that.

jhrozek · 2023-10-27T19:37:43Z

I was thinking more along the lines of having pi be handled via a provider, and configuring it from there instead.

interesting idea, let me simmer on that.

First thought - currently for the Pi evaluator we'd need two providers - GitHub to reply to PRs and then the Pi provider. We'd need to extend the Context message in the protobuf to hold an array of providers, then. We'd also need to extend the EntityContext to keep the providers private and instead of accessing the single Provider by name, we'd need to have a GetProviderByName/Type.

It really seems like something we want, but I'm unsure if it's something we can deliver in the remaining week without regressions.

JAORMX · 2023-10-30T07:19:47Z

I was thinking more along the lines of having pi be handled via a provider, and configuring it from there instead.

interesting idea, let me simmer on that.

First thought - currently for the Pi evaluator we'd need two providers - GitHub to reply to PRs and then the Pi provider. We'd need to extend the Context message in the protobuf to hold an array of providers, then. We'd also need to extend the EntityContext to keep the providers private and instead of accessing the single Provider by name, we'd need to have a GetProviderByName/Type.

It really seems like something we want, but I'm unsure if it's something we can deliver in the remaining week without regressions.

To get the provider, the only thing we really need is the project that's already available in the context and the provider name. Why do we need a list of providers?

jhrozek · 2023-10-30T14:05:43Z

I was thinking more along the lines of having pi be handled via a provider, and configuring it from there instead.

interesting idea, let me simmer on that.

First thought - currently for the Pi evaluator we'd need two providers - GitHub to reply to PRs and then the Pi provider. We'd need to extend the Context message in the protobuf to hold an array of providers, then. We'd also need to extend the EntityContext to keep the providers private and instead of accessing the single Provider by name, we'd need to have a GetProviderByName/Type.
It really seems like something we want, but I'm unsure if it's something we can deliver in the remaining week without regressions.

To get the provider, the only thing we really need is the project that's already available in the context and the provider name. Why do we need a list of providers?

To sum up the discussion we had earlier - we have technical debt in the Pi evaluator and the OSV evaluator in the sense that they both analyse the PR but also reply to it. This is because the evaluators predate the remediations. We need to split them into an evaluator and a remediator, then each would be using a single provider and we can do what @JAORMX suggested.

In the meantime, to not add even more technical debt, I added just a simple env variable.

jhrozek · 2023-10-30T14:06:10Z

examples/github/rule-types/pr_package_intelligence_check.yaml

unrelated, but should we rename the evaluator to trusty?

Probably, yes. You can do it now or in the big rename.

Since we're squashing PRs I would prefer a separate PR - otherwise this PR that adds a single env variable would /also/ rename a bunch of identifiers and it would be too hard to search git history.

and I was asking explicitly because I think this is too easy to miss in the big rename - I assume that whoever is doing the big rename would be looking for mediator, but not necessarily for package_intelligence.

evankanderson · 2023-10-30T16:10:59Z

examples/github/rule-types/pr_package_intelligence_check.yaml

Probably, yes. You can do it now or in the big rename.

evankanderson · 2023-10-30T16:13:50Z

internal/engine/eval/eval.go

 			return nil, fmt.Errorf("rule type engine missing package_intelligence configuration")
 		}
+		if pie.GetEndpoint() == "" {


I'm confused on how this is getting used -- it looks like we're getting pie from rt.Def.Eval, modifying the returned result (maybe it's a pointer?), and then passing e's package intelligence.

Should this be using e.GetPackageIntelligence()?

ah, I should have passed in pie. Thanks for noticing that. But in this specific case, it doesn't matter, because rt.Def.Eval is a pointer to a protobuf representation of that evaluator configuration. This protobuf represtatation is instantiated every time we evaluate a rule, so setting its attribute doesn't affect any other rules that might be using the same rule_type.
(Hopefully this answers the question)

I figured there was some aliasing going on, but I played dumb because I suspected the using of aliases was accidental.

…efaults Fixes: #1276

… an environment variable

evankanderson · 2023-10-30T16:16:33Z

internal/engine/eval/eval.go

 			return nil, fmt.Errorf("rule type engine missing package_intelligence configuration")
 		}
+		if pie.GetEndpoint() == "" {
+			pie.Endpoint = os.Getenv("MEDIATOR_UNSTABLE_PACKAGE_INTELLIGENCE_ENDPOINT")


I think an env var is okay for now, but it would be nice eventually to have a set of options in Config from internal/config rather than reading from the environment here. But we can defer that refactor for now.

The idea is actually to move this to be a provider. But yeah, it'll be refactored.

the config was what this PR started with but we had a call today with @JAORMX and decided against it in favour of addressing some of the technical debt in the evaluators first which will allow us to use just provider config instead.

evankanderson · 2023-10-30T17:10:42Z

internal/engine/eval/eval.go

 			return nil, fmt.Errorf("rule type engine missing package_intelligence configuration")
 		}
+		if pie.GetEndpoint() == "" {


I figured there was some aliasing going on, but I played dumb because I suspected the using of aliases was accidental.

rdimitrov

Approving 👍

I think we can add that to our ideas doc, right?

jhrozek · 2023-10-30T20:39:00Z

Approving 👍

I think we can add that to our ideas doc, right?

even better, we have an issue that tracks the refactor #1203

… a constant instead (#1473) When we first developed the Trusty evaluator, we had a configurable Trusty endpoint which we used to both talk to the API endpoint and to construct the HTTP URL to link to a package alternative. Nowadays, we use the name/namespace of the k8s service to contact trusty by default (See PR #1313 and #1401), but that means our PR replies were pointing to `http://pi.pi:8000` as well. Instead, let's use the new prod/staging constants to add a HTTP URL of a Trusty instance and point to that. Since Trusty is a hosted service only, let's just use a constant.

jhrozek force-pushed the config_mediator branch from 4275dd4 to c2a7aa7 Compare October 30, 2023 14:04

jhrozek commented Oct 30, 2023

View reviewed changes

evankanderson reviewed Oct 30, 2023

View reviewed changes

jhrozek added 2 commits October 30, 2023 17:23

Don't specify the Pi server explicitly in rule_type, rely on config d…

50471f8

…efaults Fixes: #1276

If the pr_package_intelligence_check doesn't specify an endpoint, use…

086879a

… an environment variable

jhrozek force-pushed the config_mediator branch from c2a7aa7 to 086879a Compare October 30, 2023 16:34

evankanderson approved these changes Oct 30, 2023

View reviewed changes

rdimitrov approved these changes Oct 30, 2023

View reviewed changes

JAORMX approved these changes Oct 30, 2023

View reviewed changes

jhrozek merged commit fe4a950 into mindersec:main Oct 30, 2023
12 checks passed

evankanderson mentioned this pull request Nov 2, 2023

Remove old token key secrets since we're using Keycloak #1411

Merged

jhrozek mentioned this pull request Nov 4, 2023

Don't reuse the configured Trusty http endpoint for the PR reply, use a constant instead #1473

Merged

evankanderson mentioned this pull request Nov 6, 2023

login docs.md #1492

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read the Pi hostname from config instead of rule_type #1313

Read the Pi hostname from config instead of rule_type #1313

jhrozek commented Oct 27, 2023

JAORMX commented Oct 27, 2023

jhrozek commented Oct 27, 2023

jhrozek commented Oct 27, 2023

JAORMX commented Oct 30, 2023

jhrozek commented Oct 30, 2023

jhrozek Oct 30, 2023

evankanderson Oct 30, 2023

jhrozek Oct 30, 2023

jhrozek Oct 30, 2023

evankanderson Oct 30, 2023

evankanderson Oct 30, 2023

jhrozek Oct 30, 2023

evankanderson Oct 30, 2023

evankanderson Oct 30, 2023

JAORMX Oct 30, 2023

jhrozek Oct 30, 2023

evankanderson Oct 30, 2023

rdimitrov left a comment

jhrozek commented Oct 30, 2023

Read the Pi hostname from config instead of rule_type #1313

Read the Pi hostname from config instead of rule_type #1313

Conversation

jhrozek commented Oct 27, 2023

JAORMX commented Oct 27, 2023

jhrozek commented Oct 27, 2023

jhrozek commented Oct 27, 2023

JAORMX commented Oct 30, 2023

jhrozek commented Oct 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdimitrov left a comment

Choose a reason for hiding this comment

jhrozek commented Oct 30, 2023