Add aws support #640

rabunkosar-dd · 2023-11-19T06:27:26Z

A proposed skeleton PR to support AWS resource expansion

rabunkosar-dd · 2023-12-19T17:51:53Z

@manugarg can you take a look at this if you have time? that would help replace some of our custom code we have internally to supplement cloudprober. Thank you.

manugarg · 2023-12-20T09:19:44Z

@manugarg can you take a look at this if you have time? that would help replace some of our custom code we have internally to supplement cloudprober. Thank you.

Hello Rabun,

Sorry for the long delay. I've started reviewing. I'll try to finish in a couple of days.

Thanks for submitting this.

manugarg

Thanks once again for sending this change and your patience. I've not finished reviewing, but sending in the comments so far. Any chance you can split this change it into:

1 PR to add AWS targets support.
1 PR to add elasticache and RDS.

cmd/cloudprober.go

internal/rds/aws/elasticache.go

rabunkosar-dd · 2024-01-04T19:23:00Z

Thanks once again for sending this change and your patience. I've not finished reviewing, but sending in the comments so far. Any chance you can split this change it into:

1 PR to add AWS targets support.

1 PR to add elasticache and RDS.

I have addressed some of your feedback including the splits for AWS APIs. I'll work on splitting the PR into multiple pieces for easy reviews. Thanks for the reviews so far

manugarg

Sorry for delay again. Please see my review inline.

internal/rds/client/client.go

targets/endpoint/endpoint.go

internal/rds/client/client.go

manugarg · 2024-01-12T02:59:26Z

cmd/cloudprober.go

I don't understand why this file is included in this change?

it's imports tool, placing the internal package in the top section, seems like a good change but I can remove if you want

I see, actually flag is in the middle for a reason. It's required for Google's importing tools to work properly. Please drop this file.

internal/rds/aws/testdata/targets.json

internal/rds/aws/testdata/targets2.textpb

internal/rds/aws/testdata/targets1.textpb

manugarg · 2024-01-16T08:54:51Z

internal/rds/aws/proto/config.proto

+  // How often resources should be refreshed.
+  optional int32 re_eval_sec = 98 [default = 600];  // default 10 mins


I am assuming you'll eventually need names and filters for all these resources too?

yes that's correct

manugarg · 2024-01-16T08:55:33Z

internal/rds/aws/proto/config.proto

+  // Amazon Resource Name (ARN) of the load balancer
+  // if specified, only the corresponding load balancer information is returned.
+  repeated string name = 1;


As name can be slightly confusing, should we standardize on ARN (arn) for AWS resources?

that was a wrong comment on my end, this was for the name of the LB not ARN, we can ARN later if needed. I plan to add support for LBs as a separate PR and then multiple options (ARN and name etc) can be added as filters if needed

manugarg · 2024-02-18T21:23:40Z

Hi @rabunkosar-dd, do you want to continue working on it.

Apologies for the initials delays in review, but I think you're pretty close.

(P.S. Bigger PRs require blocking more time but if you break it up, we can get through them more quickly)

rabunkosar-dd · 2024-02-20T17:49:20Z

Hi @rabunkosar-dd, do you want to continue working on it.

Apologies for the initials delays in review, but I think you're pretty close.

(P.S. Bigger PRs require blocking more time but if you break it up, we can get through them more quickly)

Hey, I'm planning to resume working on it, got blocked on other projects. Will update the PR soon

manugarg

I think it's getting close. Added some comments about dropping irrelevant file:

cmd/cloudprober.go
targets/endpoint/endpoint.go
rds/client/client.go

Also, it will be great if you can trim down the proto file for now.

Please see my comments inline inline as well.

manugarg · 2024-02-21T00:23:27Z

cmd/cloudprober.go

I see, actually flag is in the middle for a reason. It's required for Google's importing tools to work properly. Please drop this file.

manugarg · 2024-02-21T00:25:16Z

targets/endpoint/endpoint.go

To make this change more focussed, I'd leave this file too. Typo fix is not relevant to this PR.

manugarg · 2024-02-21T00:33:57Z

internal/rds/client/client.go

Can you please leave out this file too. Fixing of insecure package is an independent thing. Thanks!

manugarg · 2024-02-21T07:08:40Z

internal/rds/aws/aws_test.go

+// Copyright 2019 The Cloudprober Authors.
+//


I'd put the latest year in the copyright headers of new *.go files.

manugarg · 2024-02-21T07:20:29Z

internal/rds/aws/proto/config.proto

Can you please add resource types to protobuf as you actually add their logic. It will be easier to review them together like that. With that you'll have to change the Go code also.

So I'd prefer if this file contains only the following for now:

// AWS provider config. message ProviderConfig { // Profile for the session. optional string profile_name = 1; // AWS region optional string region = 2; // TODO: Add resource types in further PRs. }

conallob · 2024-11-11T21:22:28Z

This PR is almost 12 months old. I'm prepared to clone the development branch and wrap up this PR unless anyone objects.

If you prefer I do not, please say so by 2024-11-28 aka Thanksgiving, when I will clone the development branch and ensure rabunkosar-dd@ is recognised as co-author

fromz · 2025-05-06T06:08:58Z

@conallob do you still have a plan to complete this work?

conallob · 2025-05-07T23:52:05Z

@fromz Apologies for the delay. Yes, I ended up getting busier than I expected, so I gave @rabunkosar-dd more time.

#1075 is now a WIP, as I work through all the merge conflicts between my fork of https://github.com/rabunkosar-dd/cloudprober/tree/add_aws_support and rebase it against https://github.com/cloudprober/cloudprober/tree/main

prompt: ``` Rebase this branch against https://github.com/rabunkosar-dd/cloudprober/tree/add_aws_support , then rebase the branch against upstream, before integrating the feedback provided in cloudprober#640 (review) 17277625 ```

rabunkosar-dd and others added 5 commits November 18, 2023 22:18

add AWS support for some services

f98c266

init maps

87c7f26

unit tests for ec2

dbceb92

add elasticache test and bug fix

525c8f7

Merge branch 'master' into add_aws_support

e45c79a

manugarg self-requested a review December 20, 2023 09:19

manugarg added this to the v0.13.3 milestone Dec 21, 2023

manugarg added the enhancement New feature or request label Dec 21, 2023

manugarg reviewed Dec 23, 2023

View reviewed changes

rabunkosar-dd added 2 commits January 3, 2024 13:31

split rds and ec API calls

2e116af

remove test files

48b6e87

rabunkosar-dd added 2 commits January 4, 2024 11:36

update comments

2a06fbe

remove AWS resource discovery for a separate PR

02cad60

rabunkosar-dd requested a review from manugarg January 5, 2024 18:26

manugarg reviewed Jan 16, 2024

View reviewed changes

address comments

6785310

rabunkosar-dd requested a review from manugarg February 20, 2024 22:26

update the comment

46069f8

manugarg reviewed Feb 21, 2024

View reviewed changes

manugarg removed this from the v0.13.3 milestone Mar 21, 2024

manugarg added the waiting on proposer label May 14, 2024

manugarg added the stale label Jul 12, 2024

manugarg mentioned this pull request Sep 30, 2024

Support Consul Target Discovery #864

Open

conallob mentioned this pull request May 6, 2025

Add aws support #1075

Closed

conallob mentioned this pull request Oct 3, 2025

[targets.aws] Add aws support, by rebasing PR #640 against main using Claude Code #1153

Open

		// How often resources should be refreshed.
		optional int32 re_eval_sec = 98 [default = 600]; // default 10 mins

Add aws support #640

Are you sure you want to change the base?

Add aws support #640

Uh oh!

Conversation

rabunkosar-dd commented Nov 19, 2023

Uh oh!

rabunkosar-dd commented Dec 19, 2023

Uh oh!

manugarg commented Dec 20, 2023

Uh oh!

manugarg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rabunkosar-dd commented Jan 4, 2024

Uh oh!

manugarg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manugarg commented Feb 18, 2024

Uh oh!

rabunkosar-dd commented Feb 20, 2024

Uh oh!

manugarg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manugarg Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

conallob commented Nov 11, 2024

Uh oh!

fromz commented May 6, 2025

Uh oh!

conallob commented May 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

manugarg Feb 21, 2024 •

edited

Loading