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

Add OCI registry collector #1279

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ridhoq
Copy link
Contributor

@ridhoq ridhoq commented Sep 15, 2023

Description of the PR

Support collecting all OCI artifacts in an OCI registry that has the /v2/_catalog endpoint enabled.

TODO:

  • Add unit tests
  • Should we be duplicating the logic in both guacone and guaccollect?
  • Should we be doing the collection in parallel? Huge registries will take a long time?
  • How can we ensure that we don't collect something that hasn't been collected before? IE, if I run a collection twice against a registry, can we ensure that we don't collect everything all over again?

Fixes #298

PR Checklist

  • All commits have a Developer Certificate of Origin (DCO) -- they are generated using -s flag to git commit.
  • All new changes are covered by tests
  • If GraphQL schema is changed, make generate has been run
  • If collectsub protobuf has been changed, make proto has been run
  • All CI checks are passing (tests and formatting)
  • All dependent PRs have already been merged

@google-cla
Copy link

google-cla bot commented Sep 15, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

if err != nil {
return fmt.Errorf("unable to retrieve artifacts from OCI collector: %w", err)
}
o.checkedDigest = ociCollector.checkedDigest
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm the oci collector could skip all those that are duplicates. So no checks have to be done at the OCI registry collector.

Copy link
Contributor

@naveensrinivasan naveensrinivasan left a comment

Choose a reason for hiding this comment

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

A few suggestions 👍 . Thanks


rcOpts := getRegClientOptions()
rc := regclient.New(rcOpts...)
defer rc.Close(ctx, r)
Copy link
Contributor

Choose a reason for hiding this comment

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

The error returned from ref.New is not checked before passing r to rc.Close.

This could lead to a panic if ref.New returns an error.

if err != nil {
return fmt.Errorf("failed to list repositories in registry %s: %w", o.registry, err)
}
if len(rl.Repositories) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

rl.Repositories is not checked for nil. This could cause a panic.

Suggestion: Add nil check on rl.Repositories.


rcOpts := getRegClientOptions()
rc := regclient.New(rcOpts...)
defer rc.Close(ctx, r)
Copy link
Contributor

Choose a reason for hiding this comment

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

Close can return an error. This would not be noticed if someone is trying to debug.

A recommendation would be to log this.

defer func(rc *regclient.RegClient, ctx context.Context, r ref.Ref) {
		err := rc.Close(ctx, r)
		if err != nil {
			//log error
		}
	}(rc, ctx, r)

@ridhoq ridhoq force-pushed the ridhoq/oci-registry-collector branch from ddcd05f to 4b6e5c8 Compare September 21, 2023 22:04
Copy link

stale bot commented Nov 21, 2023

This pull request has been automatically marked as stale because it has not had recent activity (60 days of inactivity).
It will be closed in 30 days if no further activity occurs.
Thank you for your contribution!

@stale stale bot added the wontfix This will not be worked on label Nov 21, 2023
Copy link

stale bot commented Dec 21, 2023

This pull request has been automatically closed because there has been no activity for 90 days.
Please feel free to reopen it (or open a new one) if the proposed change is still appropriate.
Thank you for your contribution!

@stale stale bot closed this Dec 21, 2023
@pxp928
Copy link
Collaborator

pxp928 commented Jan 9, 2024

Re-opening this pull request as this is a feature that we want to add. FYI @ridhoq

@pxp928 pxp928 reopened this Jan 9, 2024
@stale stale bot removed the wontfix This will not be worked on label Jan 9, 2024
@pxp928
Copy link
Collaborator

pxp928 commented Mar 5, 2024

@ridhoq did you have any cycles to complete this PR? Would be a great feature to add in.

@ridhoq
Copy link
Contributor Author

ridhoq commented Mar 6, 2024

@ridhoq did you have any cycles to complete this PR? Would be a great feature to add in.

@pxp928 Agreed, thanks for the ping - let's try to get this one merged. If anything, I will spin up follow up issues for this one to reduce the scope of this PR to get it merged.

@ridhoq
Copy link
Contributor Author

ridhoq commented Apr 15, 2024

Pinging this one so it doesn't close

Signed-off-by: Ridwan Hoq <ridwanhoq@microsoft.com>
@ridhoq ridhoq force-pushed the ridhoq/oci-registry-collector branch from 4b6e5c8 to 5a6ad76 Compare April 18, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Registry collector to expand OCI collector
3 participants