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.

Sign up for GitHub

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

Jump to bottom

add Google Cloud Run output type #243

Merged
merged 1 commit into from
May 31, 2021
Merged

add Google Cloud Run output type #243

merged 1 commit into from
May 31, 2021

Conversation

developer-guy
Copy link
Contributor

Signed-off-by: Batuhan Apaydın batuhan.apaydin@trendyol.com

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area config

/area outputs

What this PR does / why we need it:
This PR enables to use of Google Cloud Run as an output.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

@developer-guy
Copy link
Contributor Author

/kind feature

@poiana poiana added kind/feature New feature or request and removed needs-kind labels May 22, 2021
@Issif Issif added this to In progress in 2.x via automation May 22, 2021
@Issif Issif added this to the 2.23.0 milestone May 22, 2021
@developer-guy developer-guy self-assigned this May 23, 2021
developer-guy
developer-guy commented May 23, 2021
View reviewed changes
outputs/client.go Outdated
@@ -178,6 +178,12 @@ func (c *Client) Post(payload interface{}) error {
req.Header.Add("event-namespace", c.Config.Kubeless.Namespace)
}

if c.OutputType == "GCPCloudRun" {
if c.Config.GCP.CloudRun.JWT != "" {
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 made that control for users who might want to use the workload identity feature instead of JWT based authentication. So, JWT based authentication should not be required for us, but if the users want JWT based authentication to enable, we should add it to the Authorization header.

Issif
Issif reviewed May 24, 2021
View reviewed changes
outputs/client.go Outdated
Comment on lines 181 to 182
if c.OutputType == "GCPCloudRun" {
if c.Config.GCP.CloudRun.JWT != "" {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if c.OutputType == "GCPCloudRun" {
if c.Config.GCP.CloudRun.JWT != "" {
if c.OutputType == "GCPCloudRun" && c.Config.GCP.CloudRun.JWT != "" {

Nested if is useless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you 👍

Issif
Issif reviewed May 24, 2021
View reviewed changes
main.go Outdated Show resolved Hide resolved
Issif
Issif requested changes May 24, 2021
View reviewed changes
Copy link
Member

@Issif Issif left a comment

Choose a reason for hiding this comment

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

Are missing the updates of:

  • README.md
  • stats.go
  • config_example.yaml

@developer-guy
Copy link
Contributor Author

@cartyc friend, would you mind to look at this issue, I couldn't get it to work with "Workload Identity", seems it's not working without providing a valid JWT. 🤔

Issif
Issif requested changes May 25, 2021
View reviewed changes
Copy link
Member

@Issif Issif left a comment

Choose a reason for hiding this comment

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

Update of env var in README too please 😉

Issif
Issif previously approved these changes May 25, 2021
View reviewed changes
Copy link
Member

@Issif Issif left a comment

Choose a reason for hiding this comment

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

can you add it in the list of outputs

config_example.yaml Show resolved Hide resolved
@poiana poiana added the lgtm label May 25, 2021
@poiana
Copy link

poiana commented May 25, 2021

LGTM label has been added.

Git tree hash: dfdf490cd1ff7079c942859dac707428b5f9069e

@developer-guy developer-guy force-pushed the feature/cloudrun-support branch 2 times, most recently from fcf744f to c3c7d47 Compare May 30, 2021 12:46
@developer-guy developer-guy changed the title wip: add Google Cloud Run output type add Google Cloud Run output type May 30, 2021
@developer-guy
Copy link
Contributor Author

@Issif @leogr folks, I think we are ready to merge this PR because it should be okay now. I removed the "WIP:" prefix from the PR description.

@developer-guy developer-guy requested a review from Issif May 30, 2021 12:47
Issif
Issif reviewed May 30, 2021
View reviewed changes
outputs/gcpcloudrun.go Outdated Show resolved Hide resolved
Issif
Issif reviewed May 30, 2021
View reviewed changes
outputs/gcpcloudrun.go Outdated Show resolved Hide resolved
Issif
Issif requested changes May 30, 2021
View reviewed changes
outputs/gcpcloudrun.go Outdated Show resolved Hide resolved
@developer-guy
add Google Cloud Run output type
ea94ae0 
Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>
@developer-guy developer-guy requested a review from Issif May 31, 2021 06:31
@poiana poiana added the lgtm label May 31, 2021
@poiana
Copy link

poiana commented May 31, 2021

LGTM label has been added.

Git tree hash: d6c3407e6273e9e46db5bd71e651801b95ec6bc9

@poiana
Copy link

poiana commented May 31, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Issif

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

The pull request process is described 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

@poiana poiana merged commit 9606e47 into falcosecurity:master May 31, 2021
2.x automation moved this from In progress to Done May 31, 2021
@developer-guy developer-guy mentioned this pull request Jun 2, 2021
Closed
@radhikapc radhikapc mentioned this pull request Jun 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Issif Issif Issif approved these changes

@KeisukeYamashita KeisukeYamashita Awaiting requested review from KeisukeYamashita

@leogr leogr Awaiting requested review from leogr

Assignees

@Issif Issif

@developer-guy developer-guy

Labels
approved dco-signoff: yes kind/feature New feature or request lgtm size/M
Projects
Falcosidekick 2.x
Status: Done
2.x
Done
Milestone
2.23.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@developer-guy @poiana @Issif