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 openSUSE/SLES support #199

Merged
merged 1 commit into from
Sep 23, 2016
Merged

Conversation

tboerger
Copy link

@tboerger tboerger commented Jun 9, 2016

In order to integrate openSUSE/SLES including the available patches for CVEs I have started to work on this pull request. I already opened this pull request to get feedback early as possible. Below you can see a checklist for the remaining items.

  • Fetcher for openSUSE
  • Write tests for openSUSE fetcher
  • Fetcher for SLES
  • Write tests for SLES fetcher

Do I miss something? Should we abstract the shared functionality from RedHat and openSUSE/SLES?

@Quentin-M
Copy link
Contributor

Quentin-M commented Jun 9, 2016

@hi,

This WIP is truly exciting. Thanks for contributing!

  • Absolutely, we should extract the OVAL parser, I believe that utils/ should be a good place for it. We would be able to test it independently
  • We may need new detectors to fully support openSUSE/SLES in Clair

Out of curiosity, do you know how often openSUSE/SLES base images are used?

@tboerger
Copy link
Author

tboerger commented Jun 9, 2016

The official opensuse:42.1 image is a pretty good example to demonstrate that it already works with the existing worker implementation.

@tboerger
Copy link
Author

tboerger commented Jun 9, 2016

Out of curiosity, do you know how often openSUSE/SLES base images are used?

I really can't tell you how often it gets used. But at least for SLES it will increase for sure. I would like to hook Clair into Portus for automated vulnerability checks.

@tboerger
Copy link
Author

We've got Hackweek now at SUSE and @jordimassaguerpla wants to finish this PR.

@jordimassaguerpla
Copy link
Contributor

I will refactor now by extracting common code first from sle and opensuse and then from redhat and suse.

@jordimassaguerpla jordimassaguerpla force-pushed the feature/opensuse branch 2 times, most recently from c5833fc to 7e1d26f Compare June 30, 2016 15:45
@jordimassaguerpla
Copy link
Contributor

I am going to rebase all in one commit

@jordimassaguerpla
Copy link
Contributor

@Quentin-M : What do you think how it looks now? I am not very good with Go, so feel free to give me any advice . I still have tomorrow and can also work on that on my spare time next week.

thanks in advance

@@ -0,0 +1,306 @@
// Copyright 2015 clair authors
Copy link
Author

Choose a reason for hiding this comment

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

That should be part of utils and not fetcher

@tboerger
Copy link
Author

The variable names are not really go-like

@jordimassaguerpla
Copy link
Contributor

can you be more concrete on the variable names?

@tboerger
Copy link
Author

tboerger commented Jul 3, 2016

can you be more concrete on the variable names?

Sure, OpenSUSE_Info should be OpenSUSEInfo, SLE_Info should be SLEInfo and so on. Separation with underscores is AFAIK pretty uncommon in Go.

@tboerger
Copy link
Author

@jordimassaguerpla hopefully you can fix the remaining NITPICKing? Afterwards @Quentin-M should be happy for sure to merge it :)

@tboerger
Copy link
Author

@guypod what are you talking about?

@Quentin-M
Copy link
Contributor

@tboerger: @guypod is talking about #175, wrong thread.

@guypod
Copy link

guypod commented Jul 13, 2016

Oops, my bad - too many similar looking GitHub tabs open ;)
Sorry for the noise!

Deleted my comment to avoid future confusion.

@jordimassaguerpla
Copy link
Contributor

sure. Sorry for the delay, hackweek ended and I didn't find the time to do it.I was refactoring the rhel code to use the oval parser and that is why I had not sent it yet. I'll do it asap.

@jordimassaguerpla
Copy link
Contributor

Hi! I just refactored everything. I extracted the oval parser from rhel and use that for suse and opensuse.
Since commiting here would be a mess, I created another PR

#218

Closing this one.

@jordimassaguerpla
Copy link
Contributor

mm. I think can't close this one. Thomas can you?

@tboerger
Copy link
Author

Why closing this one? Just rebase and squash commits? :)

@jordimassaguerpla
Copy link
Contributor

ok :-)

Branch rebased. I am closing the other one

if err != nil {
return resp, err
}
firstRHSA, err := strconv.Atoi(flagValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

The tests pass but this thing of getting the firstRHSA from the flagValue ... not sure If I extracted this correctly or if I even understood what this was for .... Can someone else just take a look and see if I missed something here? Thanks and sorry I if did.

Copy link
Contributor

Choose a reason for hiding this comment

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

This ensures that Clair doesn't try to re-parse RHSAs that have already been processed and stored - in order to make the updates faster.

var featureVersionParametersArray []database.FeatureVersion

for _, fv := range featureVersionParameters {
featureVersionParametersArray = append(featureVersionParametersArray, fv)
Copy link
Contributor

@jzelinskie jzelinskie Sep 7, 2016

Choose a reason for hiding this comment

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

Similar to the other recommendation you can use 1 append and the splat

featureVersionParametersArray = append(featureVersionParametersArray, featureVersionParamters...)

Is there a reason you're explicitly making a copy here?

@jzelinskie
Copy link
Contributor

Thanks for the PR! This is a really useful refactor of the oval stuff into a shared package. I mostly commented on code that was already there, but simply moved.

@jzelinskie jzelinskie added the reviewed/needs rework will be closed if review not addressed label Sep 7, 2016
@jordimassaguerpla jordimassaguerpla force-pushed the feature/opensuse branch 2 times, most recently from f91d1c2 to a36f994 Compare September 22, 2016 18:01
@jordimassaguerpla
Copy link
Contributor

jordimassaguerpla commented Sep 22, 2016

@jzelinskie : I've done all the changes you suggested.
Is there anything else?
I am leaving the commits for your revision but I will rebase once I get the "lgtm" from you or @Quentin-M

@jzelinskie
Copy link
Contributor

Just that last tiny thing of changing the updater flag to have an underscore and then it LGTM! Thanks a bunch for all your effort, this change is awesome!

@tboerger
Copy link
Author

Cool to see it's nearby getting merged 👏

We extracted oval parser from rhel and used that for opensuse and
SUSE Linux Enterpise

Signed-off-by: Thomas Boerger <tboerger@suse.de>
Signed-off-by: Jordi Massaguer Pla <jmassaguerpla@suse.de>
@jordimassaguerpla
Copy link
Contributor

I just did the change on the updated flag and rebased everything so it looks cleaner when you merge. Thanks a lot for taking the time to review it :) ! I am very excited to be contributing :) !

@jzelinskie jzelinskie merged commit 97347ec into quay:master Sep 23, 2016
@arjunm183
Copy link

Is this completed to integrate clair with SLES Updater.

@jzelinskie
Copy link
Contributor

This change actually got reverted because I couldn't ever get an answer from SUSE's legal department about the license of the Vulnerability data.

@flavio
Copy link
Contributor

flavio commented Sep 15, 2017

Hello from SUSE, I'm a member of the team that wrote this PR. What kind of legal clearance are you looking for and who have you contacted?

Pinging also @msmeissn from SUSE's security team. He might be able to help with your questions about the usage of the vulnerability data.

@jzelinskie
Copy link
Contributor

Hi @flavio
I simply need to know how the vulnerability data is licensed to be used. I sent an email to security@ and received a response from Marcus, but never got a follow up.

@msmeissn
Copy link

I am sorry if I failed to answer, but I got the answer from my legal department.

https://www.suse.com/de-de/support/security/oval/

The OVAL data is provided by SUSE under the Creative Commons License 4.0 with Attribution for Non-Commercial usage (CC-BY-NC-4.0).

I hope this is satisfying.

@Rots
Copy link

Rots commented Nov 1, 2017

@jzelinskie, is this expected to be merged again at some point now?

@flavio
Copy link
Contributor

flavio commented Nov 2, 2017

@Rots clair's code changed and merging the reverted code again is not possible. I'm working on that and I'll file a PR in the next weeks (I'm a bit busy ATM).

@LJosef
Copy link

LJosef commented Nov 20, 2017

@flavio looking forwarding to the progress on this merging. Great appreciate!

@chennanfei
Copy link

Looking forwarding to the progress, too!

1 similar comment
@Payback159
Copy link

Looking forwarding to the progress, too!

@maorgo
Copy link

maorgo commented Mar 5, 2018

Hi,
so is open suse supported for Clair yet?

Thanks

@flavio
Copy link
Contributor

flavio commented Mar 5, 2018

@maorgo not yet, you have to follow #503 and #506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviewed/needs rework will be closed if review not addressed
Development

Successfully merging this pull request may close these issues.

None yet