-
Notifications
You must be signed in to change notification settings - Fork 42
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
Cloud data provider #1144
Cloud data provider #1144
Conversation
This pull request does not have a backport label. Could you fix it @uri-weisman? 🙏
|
📊 Allure Report - 💔 Some tests failed or were broken.
|
@@ -60,25 +72,43 @@ func (a DataProvider) FetchData(_ string, id string) (types.Data, error) { | |||
} | |||
|
|||
func (a DataProvider) EnrichEvent(event *beat.Event, resMetadata fetching.ResourceMetadata) error { | |||
_, err := event.Fields.Put(cloudAccountIdField, strings.FirstNonEmpty(resMetadata.AwsAccountId, a.accountId)) | |||
err := insertIfNotEmpty(cloudAccountIdField, strings.FirstNonEmpty(resMetadata.AwsAccountId, a.accountId), event) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocker: make sure this doesn't break aws orgs account fields (LGTM but let's double check)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orestisfl how this can be validated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, manually since we don't have E2E tests yet.
However, the unit tests cover the case already so maybe it's overkill.
type ProviderGetter interface { | ||
GetIdentity(ctx context.Context, cfg config.GcpConfig) (*cloud.Identity, error) | ||
} | ||
|
||
type Provider struct { | ||
service ResourceManager | ||
logger *logp.Logger | ||
} | ||
|
||
// CloudResourceManagerService is a wrapper around the GCP resource manager service to make it easier to mock | ||
type CloudResourceManagerService struct { | ||
service *cloudresourcemanager.Service | ||
} | ||
|
||
type ResourceManager interface { | ||
projectsGet(context.Context, string) (*cloudresourcemanager.Project, error) | ||
} | ||
|
||
func NewProvider(ctx context.Context, cfg *config.Config, logger *logp.Logger) *Provider { | ||
gcpClientOpt, err := auth.GetGcpClientConfig(cfg, logger) | ||
if err != nil { | ||
logger.Errorf("failed to get GCP client config: %v", err) | ||
return nil | ||
} | ||
gcpClientOpt = append(gcpClientOpt, option.WithScopes(cloudresourcemanager.CloudPlatformReadOnlyScope)) | ||
crmService, err := cloudresourcemanager.NewService(ctx, gcpClientOpt...) | ||
if err != nil { | ||
logger.Errorf("failed to create GCP resource manager service: %v", err) | ||
return nil | ||
} | ||
|
||
return &Provider{ | ||
service: &CloudResourceManagerService{service: crmService}, | ||
logger: logger, | ||
} | ||
} | ||
|
||
// GetIdentity returns GCP identity information | ||
func (p *Provider) GetIdentity(ctx context.Context, cfg config.GcpConfig) (*cloud.Identity, error) { | ||
proj, err := p.service.projectsGet(ctx, "projects/"+cfg.ProjectId) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
return &cloud.Identity{ | ||
Provider: provider, | ||
ProjectId: proj.ProjectId, | ||
ProjectName: proj.DisplayName, | ||
}, nil | ||
} | ||
|
||
func (p *CloudResourceManagerService) projectsGet(ctx context.Context, id string) (*cloudresourcemanager.Project, error) { | ||
project, err := p.service.Projects.Get(id).Context(ctx).Do() | ||
if err != nil { | ||
return nil, fmt.Errorf("unable to get project with id '%v': %v", id, err) | ||
} | ||
return project, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning nil
from NewProvider
can introduce segfault risks. GetIdentity
is only called once in the lifetime of the app so we can move all initialization code in the body of the function like we do for other Dependencies
in the Benchmark
type ProviderGetter interface { | |
GetIdentity(ctx context.Context, cfg config.GcpConfig) (*cloud.Identity, error) | |
} | |
type Provider struct { | |
service ResourceManager | |
logger *logp.Logger | |
} | |
// CloudResourceManagerService is a wrapper around the GCP resource manager service to make it easier to mock | |
type CloudResourceManagerService struct { | |
service *cloudresourcemanager.Service | |
} | |
type ResourceManager interface { | |
projectsGet(context.Context, string) (*cloudresourcemanager.Project, error) | |
} | |
func NewProvider(ctx context.Context, cfg *config.Config, logger *logp.Logger) *Provider { | |
gcpClientOpt, err := auth.GetGcpClientConfig(cfg, logger) | |
if err != nil { | |
logger.Errorf("failed to get GCP client config: %v", err) | |
return nil | |
} | |
gcpClientOpt = append(gcpClientOpt, option.WithScopes(cloudresourcemanager.CloudPlatformReadOnlyScope)) | |
crmService, err := cloudresourcemanager.NewService(ctx, gcpClientOpt...) | |
if err != nil { | |
logger.Errorf("failed to create GCP resource manager service: %v", err) | |
return nil | |
} | |
return &Provider{ | |
service: &CloudResourceManagerService{service: crmService}, | |
logger: logger, | |
} | |
} | |
// GetIdentity returns GCP identity information | |
func (p *Provider) GetIdentity(ctx context.Context, cfg config.GcpConfig) (*cloud.Identity, error) { | |
proj, err := p.service.projectsGet(ctx, "projects/"+cfg.ProjectId) | |
if err != nil { | |
return nil, err | |
} | |
return &cloud.Identity{ | |
Provider: provider, | |
ProjectId: proj.ProjectId, | |
ProjectName: proj.DisplayName, | |
}, nil | |
} | |
func (p *CloudResourceManagerService) projectsGet(ctx context.Context, id string) (*cloudresourcemanager.Project, error) { | |
project, err := p.service.Projects.Get(id).Context(ctx).Do() | |
if err != nil { | |
return nil, fmt.Errorf("unable to get project with id '%v': %v", id, err) | |
} | |
return project, nil | |
} | |
type ProviderGetter interface { | |
GetIdentity(ctx context.Context, cfg config.GcpConfig) (*cloud.Identity, error) | |
} | |
type Provider struct { | |
} | |
// CloudResourceManagerService is a wrapper around the GCP resource manager service to make it easier to mock | |
type CloudResourceManagerService struct { | |
service *cloudresourcemanager.Service | |
} | |
type ResourceManager interface { | |
projectsGet(context.Context, string) (*cloudresourcemanager.Project, error) | |
} | |
// GetIdentity returns GCP identity information | |
func (p *Provider) GetIdentity(ctx context.Context, logger *logp.Logger, cfg config.GcpConfig) (*cloud.Identity, error) { | |
gcpClientOpt, err := auth.GetGcpClientConfig(cfg, logger) | |
if err != nil { | |
return nil, err | |
} | |
gcpClientOpt = append(gcpClientOpt, option.WithScopes(cloudresourcemanager.CloudPlatformReadOnlyScope)) | |
crmService, err := cloudresourcemanager.NewService(ctx, gcpClientOpt...) | |
if err != nil { | |
logger.Errorf("failed to create GCP resource manager service: %v", err) | |
return nil, err | |
} | |
proj, err := CloudResourceManagerService{service: crmService}.projectsGet(ctx, "projects/"+cfg.ProjectId) | |
if err != nil { | |
return nil, err | |
} | |
return &cloud.Identity{ | |
Provider: provider, | |
ProjectId: proj.ProjectId, | |
ProjectName: proj.DisplayName, | |
}, nil | |
} | |
func (p CloudResourceManagerService) projectsGet(ctx context.Context, id string) (*cloudresourcemanager.Project, error) { | |
project, err := p.service.Projects.Get(id).Context(ctx).Do() | |
if err != nil { | |
return nil, fmt.Errorf("unable to get project with id '%v': %v", id, err) | |
} | |
return project, nil | |
} |
which also requires changing GetGcpClientConfig
:
func GetGcpClientConfig(gcpCred config.GcpConfig, log *logp.Logger) ([]option.ClientOption, error) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orestisfl - I get your point but in your implementation, I won't be able to mock the cloudresourcemanager
service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we return also an error in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
// GetIdentity returns GCP identity information
func (p *Provider) GetIdentity(ctx context.Context, cfg config.GcpConfig, logger *logp.Logger) (*cloud.Identity, error) {
if err := p.initialize(ctx, cfg, logger); err != nil {
return nil, err
}
proj, err := p.service.projectsGet(ctx, "projects/"+cfg.ProjectId)
if err != nil {
return nil, err
}
return &cloud.Identity{
Provider: provider,
ProjectId: proj.ProjectId,
ProjectName: proj.DisplayName,
}, nil
}
func (p *Provider) initialize(ctx context.Context, cfg config.GcpConfig, logger *logp.Logger) error {
if p.service != nil {
return nil
}
gcpClientOpt, err := auth.GetGcpClientConfig(cfg, logger)
if err != nil {
return err
}
gcpClientOpt = append(gcpClientOpt, option.WithScopes(cloudresourcemanager.CloudPlatformReadOnlyScope))
crmService, err := cloudresourcemanager.NewService(ctx, gcpClientOpt...)
if err != nil {
return err
}
p.service = &CloudResourceManagerService{service: crmService}
return nil
}
This pull request is now in conflicts. Could you fix it? 🙏
|
@@ -71,11 +71,11 @@ func validateJSONFromFile(filePath string) error { | |||
|
|||
b, err := os.ReadFile(filePath) | |||
if err != nil { | |||
return fmt.Errorf("The file %q cannot be read", filePath) | |||
return fmt.Errorf("the file %q cannot be read", filePath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return fmt.Errorf("the file %q cannot be read", filePath) | |
return fmt.Errorf("file %q cannot be read", filePath) |
} | ||
|
||
if !json.Valid(b) { | ||
return fmt.Errorf("The file %q does not contain valid JSON", filePath) | ||
return fmt.Errorf("the file %q does not contain valid JSON", filePath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return fmt.Errorf("the file %q does not contain valid JSON", filePath) | |
return fmt.Errorf("file %q does not contain valid JSON", filePath) |
As discussed in elastic#1144 (comment)
As discussed in elastic#1144 (comment)
As discussed in #1144 (comment)
As discussed in #1144 (comment)
Summary of your changes
Screenshot/Data
GCP
AWS
Related Issues
Checklist