GHA: Build and Publish to Public ECR and Build Nix #49

adamtajti · 2023-08-23T14:48:01Z

We're moving our Docker image hosting from Docker Hub to AWS Public ECR Repositories.

Additionally this PR also attempts to fix failures in the tests that were undetected in the last merge as we just added these workflows.

jgdef-tulip · 2023-08-31T20:35:32Z

Although the ECR repository may have public-ecr in its path, it's still a private registry:

@adamtajti , is the intention for the public to be able to pull these images? In which case, I think we should look into AWS public gallery? cc @aranair

aranair · 2023-08-31T20:37:15Z

Although the ECR repository may have public-ecr in its path, it's still a private registry:

@adamtajti , is the intention for the public to be able to pull these images? In which case, I think we should look into AWS public gallery? cc @aranair

https://gallery.ecr.aws/u4f7y3k8/oplogtoredis I think this is the relevant public gallery entry? 🤔

adamtajti · 2023-08-31T20:43:25Z

I created that public repository as I initially wanted to place the images there, but there were a couple of permission issues that I faced while deploying from the garden and from my own machine as well, so I decided to stick with the private repository until we think that is important enough. Updated the README. I think it's alright that the workflow does something private for us at the moment, cause the forks won't be whitelisted by our IAM OIDC identity provider (@aranair @jgdef-tulip )

jgdef-tulip · 2023-08-31T20:59:55Z

.github/workflows/pull-request.yml

-          export PATH=$PATH:$(go env GOPATH)/bin
-          golangci-lint --version
+      - name: golangci-lint
+        uses: golangci/golangci-lint-action@v3


Should this be anchored with a hash?

I think it's fine.

But don't we use anchors for all non-GitHub actions as a security issue?

Hmm, you are right, although I think that later versions of the action should provide fixes to both discovered security issues as well improve the performance of the existing functionalities. I'm ok with adding one though since it's no biggie, I'll hop back to this branch in a few hours.

It seems like I forgot about this. Pinned down the versions.

jgdef-tulip · 2023-08-31T21:00:25Z

default.nix

-  nativeBuildInputs = [installShellFiles];
-  doCheck           = false;
-  doInstallCheck    = false;
+  nativeBuildInputs = [ installShellFiles ];


Is there a nix linter?

Not that I know of.

https://discourse.nixos.org/t/list-of-nix-linters/19279

jgdef-tulip · 2023-08-31T21:01:34Z

integration-tests/fault-injection/harness/inserter.go

-			log.Printf("Warning: mongo insert failed: %s", err)
+			log.Printf("Warning: mongo insert failed for doc %s: %s", id, err)
+			if insertResult != nil && insertResult.InsertedID != nil {
+				log.Printf("Warning: although the previous insert faced this error, the InsertedID wasn't nil, so we'll conclude it was a success (InterruptedDueToReplStateChange). InsertedID: %s", insertResult.InsertedID)


🤔 is there a way to check the type of err (e.g., for InterruptedDueToReplStateChange)?

I guess, this worked though, so lets go with this one. There could be many different errors, this was just what I picked up. The result is apparently simply not reliable in case of insert when using replicated mongo.

adamtajti · 2023-09-27T13:07:55Z

Ready for review again. Rebased, repushed, we are using 1:1 IAM roles for these repositories to push the images from their respective workflows.

adamtajti · 2023-10-06T06:08:14Z

(fyi The tulip/tulip parts got merged)

verifies that the vendorHash has been properly updated.

adamtajti · 2023-10-11T07:36:50Z

I've included a Nix build in this pull request, which ensures that the vendorHash stays up-to-date, especially for developers who may not be as familiar with Nix, just like myself. This should also help the developer get reminded that the semantic version number should be incremented with the change as well. I've added documentation for these changes in the README file. Thanks

jgdef-tulip · 2023-10-11T11:29:36Z

.github/workflows/build-and-push.yml

+  cancel-in-progress: ${{ github.ref != 'refs/heads/master' }}
+
+permissions:
+  id-token: write # for the creds itself


Remind me again, the meaning behind the comment for the creds itself ?

Honestly, I had no idea whatever this was required or not and what the comment meant. It looks like it was written by Ho Man, or someone before him and it was copied around (like over here). I remember failing the login without setting these permissions there, so I guess I assumed that it's needed over here as well, but it looks like it's not. Never looked into it deep enough. Removed the comment and removed the write perms.

id-token is required for oidc role assumption

jgdef-tulip · 2023-10-11T11:30:25Z

.github/workflows/build-and-push.yml

+        id: generate-tag
+        run: |
+          # Extract the current version from `default.nix`. This must pass.
+          version=$(nix flake show . --quiet --all-systems --json | jq -r '.defaultPackage."aarch64-darwin".name' | cut -d'-' -f2-)


it's fine to hard-code aarch64-darwin ?

I think it is, cause all the ${system} dicts will have the same .name attribute. Although I agree that it's ugly.

jgdef-tulip · 2023-10-11T11:31:56Z

.github/workflows/build-and-push.yml

+        uses: docker/build-push-action@3b5e8027fcad23fda98b2e3ac259d8d67585f671 # 4.0.0
+        with:
+          tags: ${{ steps.setup-ecr-buildx.outputs.ecr_registry }}/tulip/oplogtoredis:${{ steps.generate-tag.outputs.TAG }}
+          provenance: false


It's outside of this PR but eventually I like the idea of attaching metadata to instances via the provenance/SLSA attestations

If it's just a switch to true flag, then lets change it. I'm not familiar with this feature, but it looks like that's all that we would need to do at a first glance, it writes the metadata into the image index.

jgdef-tulip · 2023-10-11T11:32:41Z

.github/workflows/pull-request.yml

@@ -3,19 +3,31 @@ name: Pull Request
 on: [ pull_request ]

 jobs:
+  nix_build:
+    runs-on: ubuntu-20.04


any reason why there's an explicit ubuntu version here for PRs, but the build & push workflow uses ubuntu-latest?

Hmm, none, I just followed the pattern in this file. I just changed it to latest. For the other steps here we can't make a bump yet though, it'll need some additional dependencies to be installed. I want to do that in another round.

jgdef-tulip · 2023-10-11T11:34:13Z

README.md

+# TODO: default.nix: Change the `vendorHash` property to an empty string and run `nix build`.
+nix build
+# ...
+# error: hash mismatch in fixed-output derivation '/nix/store/by8bc99ywfw6j1i6zjxcwdmc5b3mmgzv-oplogtoredis-3.0.0-go-modules.drv':


Yeah, try reading the updated instructions, this was highly cryptic. I hope that the new one is more understandable ^^

jgdef-tulip · 2023-10-11T11:34:44Z

README.md

-You can build oplogtoredis from source with `go build .`, which produces a
-statically-linked binary you can run. Alternatively, you can use [the public
-docker image](https://hub.docker.com/r/tulip/oplogtoredis/tags/)
+You can build oplogtoredis from source with `go build .`, which produces a statically-linked binary you can run. Alternatively, you can use [the public docker image](https://gallery.ecr.aws/u4f7y3k8/oplogtoredis). Previously we used to host these images in [Docker Hub](https://hub.docker.com/r/tulip/oplogtoredis/tags/). These won't be maintained anymore.


Now that we're approved, can't we use https://gallery.ecr.aws/tulip ?

adamtajti · 2023-10-17T09:33:33Z

bump; this should get in before the redis-sentinel support.

adamtajti · 2023-10-17T13:05:41Z

Gabor gave this another review and I merged this after his approval. We think that we are in a better shape now, but in-case you have further suggestions feel free to comment here further or put up further pull requests.

adamtajti self-assigned this Aug 23, 2023

adamtajti changed the title ~~Draft: Build and Publish to ECR~~ Build and Publish to ECR Aug 24, 2023

adamtajti marked this pull request as ready for review August 29, 2023 20:26

adamtajti requested review from jgdef-tulip and jswitzer August 29, 2023 20:26

jgdef-tulip reviewed Aug 31, 2023

View reviewed changes

adamtajti requested review from jgdef-tulip and DomZach September 4, 2023 08:25

rebase and secure the role arn to public ecr

ed84c5f

adamtajti force-pushed the adam.workflow-built-ecr-images branch from 61f21c8 to ed84c5f Compare September 25, 2023 15:17

adamtajti changed the title ~~Build and Publish to ECR~~ Build and Publish to Public ECR Sep 25, 2023

adamtajti marked this pull request as draft September 25, 2023 15:19

adamtajti added 4 commits September 25, 2023 17:26

enable the pull request build-and-push again

2cec5c8

bump versions, fix registry-type inputs typo

b672efd

fix tag for public ecr

7002f49

comment out the pr trigger again for now

ba96ded

adamtajti marked this pull request as ready for review September 27, 2023 13:07

adamtajti requested a review from kocsisa September 27, 2023 13:07

adamtajti added 2 commits September 29, 2023 19:12

implementing review suggestions from elsewhere

19b4079

ops

dfe4d84

adamtajti requested a review from blikstiender October 6, 2023 05:54

pin versions

33a35a0

adamtajti added 2 commits October 10, 2023 20:11

missing vendorHash update, documented the process on how to update it

0afa173

modify the pull request workflow to do a nix build as well, which

2e8b84f

verifies that the vendorHash has been properly updated.

adamtajti changed the title ~~Build and Publish to Public ECR~~ Build and Publish to Public ECR and Build Nix Oct 10, 2023

adamtajti changed the title ~~Build and Publish to Public ECR and Build Nix~~ GHA: Build and Publish to Public ECR and Build Nix Oct 10, 2023

adamtajti requested a review from mgaborl October 11, 2023 06:45

jgdef-tulip reviewed Oct 11, 2023

View reviewed changes

adamtajti added 5 commits October 11, 2023 18:07

test read permissions for access-id

a0a04c6

run nix-build on ubuntu-latest

16926f3

provenance: true

63571c8

readme: gallery.ecr.aws/tulip

40acf51

readme: restructure the version bump and vendorHash update instructions

78f00f5

adamtajti requested a review from jgdef-tulip October 11, 2023 17:27

Update build-and-push.yml

5e01112

adamtajti mentioned this pull request Oct 17, 2023

GoSentinel: Added redis-sentinel support #50

Merged

mgaborl approved these changes Oct 17, 2023

View reviewed changes

adamtajti merged commit 8ad2cc1 into master Oct 17, 2023
8 checks passed

delete-merged-branch bot deleted the adam.workflow-built-ecr-images branch October 17, 2023 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GHA: Build and Publish to Public ECR and Build Nix #49

GHA: Build and Publish to Public ECR and Build Nix #49

adamtajti commented Aug 23, 2023 •

edited

Loading

jgdef-tulip commented Aug 31, 2023

aranair commented Aug 31, 2023

adamtajti commented Aug 31, 2023 •

edited

Loading

jgdef-tulip Aug 31, 2023

adamtajti Sep 4, 2023

jgdef-tulip Sep 4, 2023

adamtajti Sep 4, 2023

adamtajti Oct 6, 2023

jgdef-tulip Aug 31, 2023

adamtajti Sep 4, 2023

kocsisa Sep 4, 2023

jgdef-tulip Aug 31, 2023

adamtajti Sep 4, 2023

adamtajti commented Sep 27, 2023

adamtajti commented Oct 6, 2023

adamtajti commented Oct 11, 2023

jgdef-tulip Oct 11, 2023

adamtajti Oct 11, 2023 •

edited

Loading

aranair Oct 12, 2023

jgdef-tulip Oct 11, 2023

adamtajti Oct 11, 2023 •

edited

Loading

jgdef-tulip Oct 11, 2023

adamtajti Oct 11, 2023

jgdef-tulip Oct 11, 2023

adamtajti Oct 11, 2023

jgdef-tulip Oct 11, 2023

adamtajti Oct 11, 2023

jgdef-tulip Oct 11, 2023

adamtajti Oct 11, 2023

adamtajti commented Oct 17, 2023

adamtajti commented Oct 17, 2023

GHA: Build and Publish to Public ECR and Build Nix #49

GHA: Build and Publish to Public ECR and Build Nix #49

Conversation

adamtajti commented Aug 23, 2023 • edited Loading

jgdef-tulip commented Aug 31, 2023

aranair commented Aug 31, 2023

adamtajti commented Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamtajti commented Sep 27, 2023

adamtajti commented Oct 6, 2023

adamtajti commented Oct 11, 2023

Choose a reason for hiding this comment

adamtajti Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamtajti Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamtajti commented Oct 17, 2023

adamtajti commented Oct 17, 2023

adamtajti commented Aug 23, 2023 •

edited

Loading

adamtajti commented Aug 31, 2023 •

edited

Loading

adamtajti Oct 11, 2023 •

edited

Loading

adamtajti Oct 11, 2023 •

edited

Loading