fdk-node
fdk-node copied to clipboard
Adding Anchore security checks
We need to make CLI be capable to offer various Node runtimes:
- "8"
- "10"
- "11"
This PR adds multistage base images:
8and8-dev10and10-dev11and11-dev
Related to: https://github.com/fnproject/cli/issues/553
How to test this PR:
- Build two images
{node-version}and{node-version}-dev. - Create a sample function.
- Go to
func.yamland addbuild_image: ...andrun_image: ...to that. - Deploy a function.
- Run it.
The same thing, Node images belongs here.
One of the reasons for this change is that we havefnproject/node, which node version is totally unclear unless you'd actually investigate it. So, for node runtimes, it does matter to have explicit versioning.
This change is similar to https://github.com/fnproject/fdk-python/pull/77
Some work necessary to be done:
- [x] add Snyk token into CI config
- [x] add Docker user/pass into CI config
Blocked-By: https://github.com/anchore/ci-tools/issues/10
@carimura @rdallman with help of @Btodhunter we now have a new Ahcnore orb and faster checks running within CircleCI: 3 jobs in parallel, each job takes 7 mins per 2 images (dev and runtime). I'm going to populate this change to the rest of FDKs.
sweet.
should we be running this on every PR? since the images are built on master and generally not touched by any PR submissions, it's not really the fault of the PR that the checks fail (usually) - we're not expecting users to fix this.
these seem like checks that should run against master and we should probably have image updates on a weekly cron or something to pick up language/image updates, since fdk's are infrequently and irregularly updated via PRs, the process as proposed here does not seem ideal at least (still OK with doing them in general, just in a different manner most likely). any thoughts?
should we be running this on every PR? since the images are built on master and generally not touched by any PR submissions, it's not really the fault of the PR that the checks fail (usually) - we're not expecting users to fix this.
It’s true, but we have to be sure that we are safe all the time, with every PR.
these seem like checks that should run against master and we should probably have image updates on a weekly cron or something to pick up language/image updates
That’s true as well. It seems like circle ci has these options available, but still, while nightly build is a good option, having these checks on every commit still makes sense.
I think I only agree with that thesis if the PR itself changes the images. most PRs and users are not going to do this, and are not responsible for fixing those builds (arguably, the point of CI on PRs).
I see 2 things:
- we should have a regularly scheduled image update
- on any commits where the images change (like in the above), we should run security checks
this is different from running against every PR, but runs against any relevant PRs. both of these are possible afaik.
on any commits where the images change (like in the above), we should run security checks
I see where you going, but I’m not entirely sure that’s valid case, because you have no idea when Debian maintainers will release a fix to one of their packages. So, you need to run security checks at least once in a day. Docker file may remain the same while apt-get upgrade will bring new packages that you are not aware of and this IS the case when image will change.
So, the only options that is acceptable: per-PR checks even if dockerfile remain unchanged, nightly (cron) builds.
checking on PRs seems unnecessary if we're doing nightly builds? again, most PRs are not doing anything to change the state of the images, and users are not expected to fix these.
checking on PRs seems unnecessary if we're doing nightly builds?
You have no guarantees that at that moment when PR will appear images would be up to date.
most PRs are not doing anything to change the state of the images
It’s an assumption, you have no guarantees.
and users are not expected to fix these.
Any user’s PR must be delayed until we fix image check. Images have higher priority by default. So, users will have to wait and rebase once we will fix an image.
You have no guarantees that at that moment when PR will appear images would be up to date.
why do we care? I get the point that users could change the image and make it insecure. the reality is that we're the only ones doing that.
i guess part of my whole confusion with this movement in general to conflate images into the fdk repo is that the images have almost nothing to do with the fdk code. aside from the recent addition of the fn user (which we could even do in build stage), we're not doing anything substantial over whatever image we're building from from docker hub.
if the images existed in a repo with only images, I could kind of get running the security checks on every PR, however this repo has 2 things, code and images. i'm not sure there was ever a formally agreed upon proposal to put images into fdk repos to begin with, when we start doing things like this it seems to make a lot less sense. not to mention the hassle of propagating all of this stuff across every individual fdk repo, it kinda smells like we should take a step back.
I tend to disagree. Initial concern was that dockers repo is a dump and cemetery for images we have since iron and the dump was still growing.
Runtime images are the artifacts to FDKs only (there’s only 1 images shared across repos: go image). So, they have to live and be tested all the time along with an FDK code.
why do we care? I get the point that users could change the image and make it insecure. the reality is that we're the only ones doing that.
Why do we care? Because we committed to deliver secure code and images. Users could make images insecure but as long as they rely on our images we must ensure them that images are up to date all the time (all security issues resolved if possible).
i'm not sure there was ever a formally agreed upon proposal to put images into fdk repos to begin with
Well, I saw zero objections when we merged images to Java FDK, Python FDK and Go FDK. So, there are no technical or any other reasons for not keeping images where they actually belong.
I tend to disagree. Initial concern was that dockers repo is a dump and cemetery for images we have since iron and the dump was still growing.
sure, but it doesn't have to be. it doesn't have to be anything. we can create a new repo, etc.
Why do we care? Because we committed to deliver secure code and images. Users could make images insecure but as long as they rely on our images we must ensure them that images are up to date all the time (all security issues resolved if possible).
yes, but merging code changes does not mean we are delivering the image. I should have made this more clear. even if we merge something that breaks it, we don't have to deploy that [image] just because it hit master. point being, these are separate.
Runtime images are the artifacts to FDKs only (there’s only 1 images shared across repos: go image).
the runtime images do not even contain the FDK code, I realize that our verbiage is the source of confusion here. FDK [code] and runtime image are separate things.
point stands that we have never had a formal discussion or proposal about how to manage the images, it's been done differently for different fdks since this project started, without any discussion really. now that we're deficient and adding a significant amount of process and needing to support these better, it seems wise to have this discussion to take into account all of the tradeoffs before making a decision.
maybe discuss IRL in next community call? then we can figure out how to move forward... or slack anytime.
Since PR to FDK Python was merged I'll duplicate all changes here for the consistency.
@crush-157 can you please help to figure out what's wrong with tests? here's the log from the build:
TAP version 13
# print logFramer
(node:120) UnhandledPromiseRejectionWarning: TypeError: console.warn is not a function
at /home/circleci/fdk-node/fn-fdk.js:295:17
at processTicksAndRejections (internal/process/task_queues.js:89:5)
(node:120) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 2)
(node:120) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
Okay, nwm, i figured it out, it's all about Node.JS version, it seems like FDK requires at least Node 9. (cc @rdallman).