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 DataDetector to support ACI and other layout format #49

Merged
merged 6 commits into from
Jan 7, 2016
Merged

Add DataDetector to support ACI and other layout format #49

merged 6 commits into from
Jan 7, 2016

Conversation

liangchenye
Copy link
Contributor

Now we have DetectOS and DetectPackage, I think it also makes sense to have a 'DetectData' to handle different image formats. The PR implements this feature. It adds 'imageFormat' to the parameters for DataDetector to choose the right detector. For most use cases, user knows what format a image is. If not, the detector checks the suffix at present.

It solves my issue (#48) and (#8)
Other potential benefits:

  1. folder (add a folder.go to detectors/data)
  2. OCI (add a oci.go to detectors/data)

Signed-off-by: liangchenye <liangchenye@huawei.com>
Signed-off-by: liangchenye <liangchenye@huawei.com>
@Quentin-M
Copy link
Contributor

The aci spec specifies:

Image archives MAY be compressed with gzip, bzip2, or xz.

Only tar/gzip is supported at the moment.
Also, there is the encryption thing but I am unsure that it is necessary for Clair to handle it.

Image archives MAY be encrypted using PGP symmetric encryption with AES cipher, after optional compression.

@liangchenye
Copy link
Contributor Author

Thanks for collecting './'.

I hope 'Clair' could handle this feature, or the user have to untar the ACI and tar the rootfs to make it work.
Or is it suggested to implement such feature in 'contrib' ?

@Quentin-M
Copy link
Contributor

It is definitely not difficult to do from what you've already done.

  • You have to modify your PR to always trim ./ and then an optional prefix (rootfs in the case of ACI).
  • If you want Clair to be able to handle bzip2/xz compressed images, modify SelectivelyExtractArchive to support it. However, that is not strictly required as stated by the spec. Same thing with PGP.

@jzelinskie
Copy link
Contributor

linking #48

Signed-off-by: liangchenye <liangchenye@huawei.com>
Signed-off-by: liangchenye <liangchenye@huawei.com>
@liangchenye
Copy link
Contributor Author

@Quentin-M @jzelinskie bzip2/xz are done.
Since there is no golang compress/xz, the current xz implementation is copied from github.com/appc/spec/aci/file.go.
Add utils_test.tar.bz2 and utils_test.tar.xz at the same time.

The PGP support is not implemented yet, I don't have a concrete idea. It requires an extra signature file. Maybe we need to change the API?

)

// TarDataDetector implements DataDetector and detects layer data in 'tar' format
type TarDataDetector struct{}
Copy link
Contributor

Choose a reason for hiding this comment

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

It should probably not be called TarDataDetector. Both this/ACI are tar anyways. Should probably be DockerDataDetector instead.

@Quentin-M
Copy link
Contributor

That looks cool. we are really looking forward to having ACI support !

I quickly tested it and did some comments.
As far as I can tell:

  • ACI support works after a small fix (see comments)
  • bzip2 / xz support work as well

Also, I discussed a bit with the team and we think that we should not try to do format detection automatically - it could become tricky at the end of the day if we support more than Docker/ACI. In other words, ImageFormat should be a mandatory parameter, with currently two choices: Docker and ACI. Not giving the format should fail (BadRequest) - asking the user to specify it.

Thanks!

@Quentin-M
Copy link
Contributor

@alban Adding to the discussion about PGP. See #49 (comment).

Signed-off-by: Liang Chenye <liangchenye@huawei.com>
Signed-off-by: Liang Chenye <liangchenye@huawei.com>
@liangchenye
Copy link
Contributor Author

@Quentin-M Thanks for the review, the new commits solve problems you mentioned above.

One thing is missing: a ACI image for testing. I'm not sure if I should make a fake one based on the rootfs in utils_test.tar or use the official 'etcd' one in https://github.com/coreos/rkt.

@Quentin-M
Copy link
Contributor

@liangchenye Thanks for the fixes and your contribution! I am really excited by seeing you contributing to Clair. I will take a deeper look at it tomorrow (Almost 1 AM here) and ensure Clair still works fine for the general usage (well, I bet it does), but I think that's pretty great! @jzelinskie has the last words on the code-reviewing procedure. He may have some comments too. After that and a commit squashing with an adequate commit message, I'll be happy to merge your work!

About testing, I think that, generally speaking, we should not include big files in the project as it slows down repository pulling and increase the size of the project. Instead, we should use minimal data samples - that are relevant to the tests we want to run. As you can notice in worker/packages for instance, we have an excerpt from a /var/lib/dpkg/status file (that I modified slightly to cover more test surface) and a shrunken (minimized) rpm database from /var/lib/rpm/Packages. Depending on what you want to do, couple files would be enough.

@liangchenye
Copy link
Contributor Author

my pleasure :)

Quentin-M pushed a commit that referenced this pull request Jan 7, 2016
Add DataDetector to support ACI and other layout format
@Quentin-M Quentin-M merged commit e834301 into quay:master Jan 7, 2016
@jzelinskie jzelinskie added kind/feature request wishes for new functionality/docs reviewed/lgtm labels Mar 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature request wishes for new functionality/docs
Development

Successfully merging this pull request may close these issues.

None yet

3 participants