Page MenuHomePhabricator

[terraform] [1/n] adding Opensearch service
ClosedPublic

Authored by will on Nov 13 2023, 11:22 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 11, 7:44 AM
Unknown Object (File)
Mon, Nov 11, 4:24 AM
Unknown Object (File)
Mon, Oct 28, 12:25 AM
Unknown Object (File)
Thu, Oct 24, 5:19 AM
Unknown Object (File)
Fri, Oct 18, 6:38 AM
Unknown Object (File)
Fri, Oct 18, 6:38 AM
Unknown Object (File)
Fri, Oct 18, 6:38 AM
Unknown Object (File)
Fri, Oct 18, 6:38 AM

Details

Summary

Adding the Opensearch service as a shared module

Test Plan

Tested on staging. Properly deployed and interactive

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 13 2023, 11:39 PM
Harbormaster failed remote builds in B24095: Diff 33194!
will retitled this revision from [terraform] adding Opensearch service to [terraform] [1/n] adding Opensearch service.Nov 13 2023, 11:44 PM
will retitled this revision from [terraform] [1/n] adding Opensearch service to [terraform] [2/n] adding Opensearch service.Nov 20 2023, 6:02 PM
will retitled this revision from [terraform] [2/n] adding Opensearch service to [terraform] [1/n] adding Opensearch service.

squashing opensearch terraform changes

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 20 2023, 8:46 PM
Harbormaster failed remote builds in B24295: Diff 33447!
Harbormaster returned this revision to the author for changes because remote builds failed.Nov 20 2023, 11:05 PM
Harbormaster failed remote builds in B24297: Diff 33449!

Enable Opensearch on local dev terraform environment

Remove unnecessary iam arn for opensearch

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 20 2023, 11:32 PM
Harbormaster failed remote builds in B24299: Diff 33451!

Rebase changes to local and remote

will requested review of this revision.Nov 21 2023, 12:41 AM

This diff overlaps with D9757 - are you going to abandon that one?

services/terraform/remote/aws_iam.tf
199โ€“211 โ†—(On Diff #33479)

My comment https://phab.comm.dev/D9757?id=32929#inline-61182 hasn't been addressed.

If you're not planning doing it here, can you create a follow-up task as a subtask of ENG-4540?

This diff overlaps with D9757 - are you going to abandon that one?

Yeah. There were some issues I ran into while rebasing and I had to abandon D9757.

It's generally best to update existing diffs instead of putting up new ones. If you do want to abandon an existing diff, you should use the abandon option from the menu at the bottom (at this point you should do this)

varun added 1 blocking reviewer(s): bartek.

looks fine to me but setting @bartek as blocking since he's more well-versed in terraform

bartek requested changes to this revision.Dec 6 2023, 2:40 AM

You addressed my feedback from this diff in D9878 which is currently a draft. That one looks good to me ๐Ÿ‘
But changes from that diff overlap with this diff (you created opensearch.tf in both diffs) and that makes me confused which one I should review. I guess you again had some trouble when rebasing

Requesting changes now but feel free to click "Request Review" when you're ready ๐Ÿ˜‰

This revision now requires changes to proceed.Dec 6 2023, 2:40 AM

@wyilio, it's critical that you figure out how to handle diff stacks โ€“ right now it looks like you have a pattern of repeatedly opening conflicting diffs.

I'm not exactly sure as to your workflow. I'd suggest that in your next 1:1 with another engineer (eg. @varun, @bartek, or me), you talk through your workflow and get some feedback.

Some high-level advice: if you have multiple diffs touching the same code, it's critical that those diffs correspond to commits within the same local branch.

will marked an inline comment as done.

Restrict domain search policy

Change name to opensearch-service

Add principal and remove role specific policy configuration

I was having trouble getting the HTTP requests to work with the opensearch domain when I included opensearch_domain_access under managed_policy_arns. After placing opensearch_domain_access in its own policy, there didn't seem to be any more 403 Forbidden errors when accessing the opensearch domain with the search index lambda.

In this diff, I lay out the policy for future usage. In [4/n], I set the principals identifier to the lambda arn. Hopefully this addresses the earlier comment on limiting access scope.

Whitespace changes for formatting

This comment was removed by will.
will removed a reviewer: bartek.
varun added a subscriber: bartek.

accepting to unblock. it seems like @bartek's feedback has been addressed

This revision is now accepted and ready to land.Jan 3 2024, 11:29 PM

Thanks for accepting on my behalf! Looks good to me

This revision was automatically updated to reflect the committed changes.