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.

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

Implements s3 exporter #372

Closed
wants to merge 1 commit into from
Closed

Conversation

jpinsonneau
Copy link
Contributor

@jpinsonneau jpinsonneau commented Jun 12, 2023

This PR implements S3 exporter using the already existing s3 encode stage.

Here is a sample of generated files in the bucket:
sample.txt

Rely on netobserv/flowlogs-pipeline#442

@openshift-ci
Copy link

openshift-ci bot commented Jun 12, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jpinsonneau. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Comment on lines +267 to +273
//+kubebuilder:default:=""
// username to connect to server
AccessKeyID string `json:"accessKeyId"`

//+kubebuilder:default:=""
// password to connect to server
SecretAccessKey string `json:"secretAccessKey"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be better to load a secret here but it's useless for now as FLP doesn't.
In result, the credentials would be in clear text in the FLP configmap.

Copy link
Member

@jotak jotak Jun 15, 2023

Choose a reason for hiding this comment

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

Well, I think we should read it from a secret/CM anyway: not because it makes it more hidden (you're right that it will be visible in FLP anyway - which is ok as long as FLP is only accessible for cluster admins) , but in a gitops perspective users would like to decouple a config such as FlowCollector CR, and secrets management, for instance by using stuff like Vault: so that the FlowCollector CR can be safely stored in config repos (e.g. in git) without compromising its secrets, and secrets are generated/injected at runtime.

I have a commit for Kafka / SASL that I haven't pushed as a PR yet (I should do that) where there's a similar problem to solve: jotak@17a2ae9 - maybe I should push it and we can align our 2 PRs & reuse some structs / functions

Copy link
Member

Choose a reason for hiding this comment

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

(edit: rebased my sasl branch, new one is jotak@3a4f105 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was considering replacing these creds by a secret and mounting the secret inside FLP.
Do you feel it's not necessary here ?

Copy link
Member

@jotak jotak Jun 15, 2023

Choose a reason for hiding this comment

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

yes it can be done that way but I guess then you need to update FLP to read from file?
That's also what I did for sasl:
jotak@3a4f105#diff-c761638f65b982f9ecf3717616920a5a3c0bb1670fd8ee7e9ab51afc4f166581R578-R593

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I'm fine updating FLP to add this capability if we are aligned

)

// FlowCollectorExporter defines an additional exporter to send enriched flows to.
type FlowCollectorExporter struct {
// type selects the type of exporters. The available options are "KAFKA" and "IPFIX". "IPFIX" is <i>unsupported (*)</i>.
// type selects the type of exporters. The available options are "KAFKA", "IPFIX" and "S3". "IPFIX" is <i>unsupported (*)</i>.
Copy link
Member

Choose a reason for hiding this comment

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

I guess S3 would be unsupported too, or is there a request to support it?

@@ -733,6 +765,10 @@ type FlowCollectorExporter struct {
// IPFIX configuration, such as the IP address and port to send enriched IPFIX flows to. <i>Unsupported (*)</i>.
// +optional
IPFIX FlowCollectorIPFIXReceiver `json:"ipfix,omitempty"`

// S3 configuration, such as the endpoint, credentials and bucket name
Copy link
Member

Choose a reason for hiding this comment

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

Same here: should be said as "unsupported" ?

@jpinsonneau
Copy link
Contributor Author

/hold

@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jpinsonneau
Copy link
Contributor Author

Closing this as it's quite outdated

@jpinsonneau jpinsonneau closed this Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants