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

feat: oci logging analytics output plugin implementation #7830

Merged
merged 12 commits into from
Sep 22, 2023

Conversation

adiforluls
Copy link
Member

@adiforluls adiforluls commented Aug 15, 2023

A new output plugin for OCI Logging Analytics.
https://docs.oracle.com/en-us/iaas/logging-analytics/index.html
REST API reference of the output destination: https://docs.oracle.com/en-us/iaas/api/#/en/logan-api-spec/20200601/Upload/UploadLogEventsFile


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • Example configuration file for the change
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • Run local packaging test showing all targets (including any new ones) build.
  • Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • Documentation required for this feature

fluent/fluent-bit-docs#1186

Backporting

  • Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

@adiforluls
Copy link
Member Author

[Output]
    Name oci_logan
    Match *
    Namespace <REDACTED>
    config_file_location <absolute-path-to-oci-config-file>
    profile_name ADMIN
    oci_la_log_group_id ocid1.loganalyticsloggroup.oc1<REDACTED>tkovmrkq
    oci_la_log_source_name Kubernetes Container Generic Logs
    tls On
    tls.verify Off
[2023/08/15 18:36:22] [ info] [output:oci_logan:oci_logan.0] payload={"logEvents":[{"logSourceName":"Kubernetes Container Generic Logs","logRecords":["I0801 00:19:20.282516      12 store.go:588] \"Secret was added and it is used in ingress annotations. Parsing\" secret=\"cattle-system/tls-rancher-ingress\"","I0801 00:19:20.283998      12 backend_ssl.go:67] \"Adding secret to local store\" name=\"cattle-system/tls-rancher-ingress\"","W0801 00:19:21.667885      12 controller.go:1152] Service \"cattle-system/rancher\" does not have any active Endpoint.","I0801 00:19:21.668014      12 controller.go:190] \"Configuration changes detected, backend reload required\"","I0801 00:19:21.718198      12 controller.go:207] \"Backend successfully reloaded\""]}]}
[2023/08/15 18:36:22] [ info] [output:oci_logan:oci_logan.0] lg_id=ocid1.loganalyticsloggroup.oc1.iad.amaaaaaalknnwbqaldbpnvd5mjs6tjyhprh7s43ftwk2efoo47ggtkovmrkq
[2023/08/15 18:36:22] [ info] [output:oci_logan:oci_logan.0] full_uri=/20200601/namespaces/odsbuilddev/actions/uploadLogEventsFile?logGroupId=ocid1.loganalyticsloggroup.oc1.iad.amaaaaaalknnwbqaldbpnvd5mjs6tjyhprh7s43ftwk2efoo47ggtkovmrkq&payloadType=JSON
[2023/08/15 18:36:23] [ info] [output:oci_logan:oci_logan.0] built client
[2023/08/15 18:36:23] [ info] [output:oci_logan:oci_logan.0] built request
[2023/08/15 18:36:23] [ info] [output:oci_logan:oci_logan.0] request header POST /20200601/namespaces/odsbuilddev/actions/uploadLogEventsFile?logGroupId=ocid1.loganalyticsloggroup.oc1.iad.amaaaaaalknnwbqaldbpnvd5mjs6tjyhprh7s43ftwk2efoo47ggtkovmrkq&payloadType=JSON HTTP/1.1

[2023/08/15 18:36:24] [ info] [output:oci_logan:oci_logan.0] placed request
[2023/08/15 18:36:24] [ info] [output:oci_logan:oci_logan.0] success

@adiforluls adiforluls force-pushed the aditya/oci-logan-plugin branch from 33f099a to afee9d9 Compare August 15, 2023 13:19
@adiforluls
Copy link
Member Author

To code-owners: PR description includes the link of REST API that is used to flush logs. The authorization is done using a configuration file, following is the documentation for that https://docs.oracle.com/en-us/iaas/Content/API/Concepts/sdkconfig.htm

@adiforluls
Copy link
Member Author

I'll fix the merge conflict and include appropriate copyrights tomorrow.

@adiforluls
Copy link
Member Author

cc: @edsiper @leonardo-albertovich

@patrick-stephens patrick-stephens added the ok-package-test Run PR packaging tests label Aug 15, 2023
@patrick-stephens
Copy link
Contributor

Useful plugin I think to have @adiforluls so appreciate the effort.

Before we can review I think we need to tackle the various failing target builds.
These may need code changes so no point looking at code if it might change.

Ideally this plugin is cross-platform but if it is not supported on Windows/macOS then defaults need to be set to disable it:

Please build locally for all Linux targets using the script linked above - this uses a container build to do each target so takes some time but should help you debug any missing dependencies or per-target issues.

If it is not supported please add a comment as well to explain why not so people can see in the future if they want to enable it.

It will need documentation too adding in fluent/fluent-bit-docs @adiforluls.

@adiforluls
Copy link
Member Author

adiforluls commented Aug 15, 2023

Sure @patrick-stephens, I've been able to build it on macOS (the log output is from a binary built and run on macOS).

Also I don't see that tests ran for this PR? All I see are a bunch of tests skipped

About documentation: Is that supposed to be contributed alongside this PR in the docs repo or after this PR is merged in fluent-bit? Will work on writing the documentation accordingly.

@patrick-stephens
Copy link
Contributor

Yeah Github status is not reporting any problems but it has not triggered the CI for package builds for some reason or any of the unit tests.

@patrick-stephens
Copy link
Contributor

I think the issue is that there is a block for first time contributors before we run any workflows - that should show an approve-and-run button here but does not. I think that's what is the issue, no UI to approve the workflows to run for you.

@patrick-stephens
Copy link
Contributor

Ah @adiforluls the reason is you have conflicts that need to be resolved - nothing can run until that is sorted.

@adiforluls adiforluls temporarily deployed to pr August 15, 2023 16:00 — with GitHub Actions Inactive
@adiforluls adiforluls temporarily deployed to pr August 15, 2023 16:00 — with GitHub Actions Inactive
@adiforluls adiforluls temporarily deployed to pr August 15, 2023 16:00 — with GitHub Actions Inactive
@adiforluls
Copy link
Member Author

Ack, it was a small conflict so I fixed it just now

@patrick-stephens
Copy link
Contributor

Yeah, you can see there are a few coding issues, e.g. on older targets this is not valid syntax:

#11 99.52 /src/plugins/out_oci_logan/oci_logan.c:1115:9: error: 'for' loop initial declarations are only allowed in C99 mode
#11 99.52          for(int i = 0; i < map_size; i++) {

Check the coding style but I think Fluent Bit requires everything pre-declared.

@adiforluls adiforluls temporarily deployed to pr August 15, 2023 16:32 — with GitHub Actions Inactive
@adiforluls adiforluls temporarily deployed to pr August 16, 2023 08:47 — with GitHub Actions Inactive
@adiforluls
Copy link
Member Author

Here's the documentation PR fluent/fluent-bit-docs#1186
@edsiper

@edsiper
Copy link
Member

edsiper commented Aug 31, 2023

@adiforluls is it the "oci_logan" name a recognizable term that describes Oracle Cloud Log Analytics platform ?, I just want to make sure people understand what it means, or if we need to rename it before merging it

CMakeLists.txt Outdated
@@ -255,6 +255,7 @@ option(FLB_OUT_PROMETHEUS_REMOTE_WRITE "Enable Prometheus remote write plugin"
option(FLB_OUT_S3 "Enable AWS S3 output plugin" Yes)
option(FLB_OUT_VIVO_EXPORTER "Enabel Vivo exporter output plugin" Yes)
option(FLB_OUT_WEBSOCKET "Enable Websocket output plugin" Yes)
option(FLB_OUT_OCI_LOGAN "Enable OCI Logging analytics plugin" Yes)
Copy link
Contributor

Choose a reason for hiding this comment

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

Per the docs PR, I would expand OCI to be Oracle Cloud Infrastructure as it is a fairly generic acronym and may be confused with open container initiative.

Copy link
Member

Choose a reason for hiding this comment

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

what about oracle_log_analytics ? azure has azure_logs ingestion

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah sounds good to me, making it clear it is Oracle I think is the key one - we have AWS/Google equivalents too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, I'll make these changes

Copy link
Member Author

Choose a reason for hiding this comment

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

Refactored @edsiper

Signed-off-by: adiforluls <[email protected]>
@adiforluls adiforluls temporarily deployed to pr September 1, 2023 06:35 — with GitHub Actions Inactive
@adiforluls adiforluls temporarily deployed to pr September 1, 2023 06:35 — with GitHub Actions Inactive
@adiforluls adiforluls temporarily deployed to pr September 1, 2023 06:35 — with GitHub Actions Inactive
@adiforluls adiforluls temporarily deployed to pr September 1, 2023 07:03 — with GitHub Actions Inactive
@adiforluls
Copy link
Member Author

Hi @edsiper @patrick-stephens , I see this was added to v2.1.9 milestone but wasn't merged to be included in 2.1.9. Let me know what's holding the merge such that this goes out in the next release.

@patrick-stephens
Copy link
Contributor

Assuming you have updated per the review comments it likely just needs a re-review to be merged. It will then go into whichever is the next release after that.

Unfortunately it takes time to work through all the things in the review queue but I'm sure @edsiper will have a look as soon as he can. Please just make sure all comments are addressed.

@adiforluls
Copy link
Member Author

Thanks @patrick-stephens for you response.
Below is a summary of comments/changes requested that I've addressed

  • The code changes that were requested have been made
  • Evaluation of leaks was done and leaks that were found were fixed
  • Documentation PR for the plugin was raised
  • Renamed the plugin to oracle_log_analytics

cc: @edsiper

Note: Once this is merged, I'll be raising another PR to add two other modes of authentication to this plugin, the size of that PR is roughly the same as this one. It'll be good if we can get this in by next week if the PR looks good such that we have enough time to review the follow up PR and get that in before the next release as well.

@edsiper
Copy link
Member

edsiper commented Sep 19, 2023

the plugin build option still has the old name: FLB_OUT_OCI_LOGAN , same as the registration structure, please get it fixed so we can merge it, thank you

@adiforluls adiforluls temporarily deployed to pr September 19, 2023 05:30 — with GitHub Actions Inactive
@adiforluls adiforluls temporarily deployed to pr September 19, 2023 05:30 — with GitHub Actions Inactive
@adiforluls adiforluls temporarily deployed to pr September 19, 2023 05:30 — with GitHub Actions Inactive
@adiforluls
Copy link
Member Author

Fixed the old name references @edsiper

@adiforluls adiforluls temporarily deployed to pr September 19, 2023 06:01 — with GitHub Actions Inactive
#define FLB_OCI_ERROR_CODE_TOO_MANY_REQUESTS "TooManyRequests"
#define FLB_OCI_ERROR_CODE_INTERNAL_SERVER_ERROR "InternalServerError"

#define METADATA_URL_BASE "http://169.254.169.254/opc/v2"
Copy link
Member

Choose a reason for hiding this comment

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

why these endpoints are hard coded and there is no host/DNS ? are these definitions in use ?

Copy link
Member Author

Choose a reason for hiding this comment

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

These are not in use currently but will be in use in the follow up PR to add two other modes of authentication to connect to Oracle Cloud Infrastructure endpoints. I can remove it from this PR if you want.

The reason for no host/DNS is probably because these endpoints were set this way. OCI has SDKs in various languages and they include these endpoints hardcoded just like this.

Copy link
Member Author

@adiforluls adiforluls Sep 21, 2023

Choose a reason for hiding this comment

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

Removed those endpoints from this PR since they are unused currently @edsiper

Signed-off-by: adiforluls <[email protected]>
@adiforluls adiforluls temporarily deployed to pr September 21, 2023 05:13 — with GitHub Actions Inactive
@adiforluls adiforluls temporarily deployed to pr September 21, 2023 05:13 — with GitHub Actions Inactive
@adiforluls adiforluls temporarily deployed to pr September 21, 2023 05:13 — with GitHub Actions Inactive
@adiforluls adiforluls temporarily deployed to pr September 21, 2023 05:43 — with GitHub Actions Inactive
@edsiper edsiper merged commit f16a742 into fluent:master Sep 22, 2023
71 of 73 checks passed
leonardo-albertovich pushed a commit that referenced this pull request Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-required ok-package-test Run PR packaging tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants