Plugins: Modify interface for plugin validations to allow taking PDC into account #96089
Conversation
pkg/api/plugin_resource.go
Outdated
| } | ||
|
|
||
| err = hs.PluginRequestValidator.Validate(dsURL, c.Req) | ||
| err = hs.PluginRequestValidator.Validate(ds, c.Req) |
There was a problem hiding this comment.
papagian
added
no-backport
papagian
requested review from
juanicabanas,
evictorero,
wbrowne,
andresmgot and
xnyo
and removed request for
a team
papagian
changed the title
pkg/services/query/query.go
Outdated
| @@ -244,7 +244,7 @@ func (s *ServiceImpl) handleExpressions(ctx context.Context, user identity.Reque | |||
| func (s *ServiceImpl) handleQuerySingleDatasource(ctx context.Context, user identity.Requester, parsedReq *parsedRequest) (*backend.QueryDataResponse, error) { | |||
| queries := parsedReq.getFlattenedQueries() | |||
| ds := queries[0].datasource | |||
| if err := s.pluginRequestValidator.Validate(ds.URL, nil); err != nil { | |||
| if err := s.dataSourceRequestValidator.Validate(ds, setting.SecureSocksDSProxySettings{}, nil); err != nil { | |||
There was a problem hiding this comment.
Don't we want to pass cfg.ProxySettings here?
| @@ -27,7 +29,7 @@ func RedirectLimitMiddleware(reqValidator validations.PluginRequestValidator) sd | |||
| return nil, locationErr | |||
| } | |||
|
|
|||
| if validationErr := reqValidator.Validate(location.String(), nil); validationErr != nil { | |||
| if validationErr := reqValidator.Validate(&datasources.DataSource{URL: location.String()}, setting.SecureSocksDSProxySettings{}, nil); validationErr != nil { | |||
There was a problem hiding this comment.
It seems like this is the only place that is problematic 😞 The only escape hatch we have here is that we should have the following labels available via opts sdkhttpclient.Options:
- Labels["datasource_name"]
- Labels["datasource_uid"]
- Labels["datasource_type"]
They should always be populated, but it would probably be a good idea to actually add types/funcs for these on the SDK struct itself (could be done in a follow up). That means we'd have to read the datasource from the DB in this case though. Maybe that's okay since this is only when status code is 3xx.
Aside: I also think we could get away with not having to pass setting.SecureSocksDSProxySettings actually since *cfg.Setting could just become a field over here - since that is the only implementation where it's actually used.
There was a problem hiding this comment.
Regarding the 1st point I wonder if it's ever possible the res.Location() (which is what we currently check) to be different from the datasource URL? I think that we intentionally check the location header.
Regarding the 2nd point I agree: that would simplify it a lot
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions! |
it additionally renames
validations.PluginRequestValidatortovalidations.DataSourceRequestValidator