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

job-info: Add guest access to eventlogs #2098

Merged
merged 2 commits into from Mar 26, 2019

Conversation

@chu11
Copy link
Contributor

commented Mar 22, 2019

Per discussion i #2041. I know it's not much, but wanted to get the structure of this in as well as the tests. The tests fake job submission under a guest account for the time being.

Nice chunk of code lifted from @garlick's branch in https://github.com/garlick/flux-core/tree/flux_job_eventlog

Copy link
Member

left a comment

Looks reasonable.

@@ -14,6 +14,8 @@
#include <czmq.h>
#include <jansson.h>
#include <flux/core.h>
#include <argz.h>

This comment has been minimized.

Copy link
@garlick

garlick Mar 22, 2019

Member

argz/envz probably not needed anymore

@garlick

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

Was just beginning to comment:

Oh yeah, on the ci failures: when running --with-flux-security, the ingest module checks that the signed jobspec matches the authenticated user. Those env vars are influencing the authenticated user but the signature comes from munge which can't be spoofed.

but it looks like you just pushed changes to that effect?

@chu11 chu11 force-pushed the chu11:issue2041_part2-eperm-checks branch from 191b4cb to 6cb98a2 Mar 22, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Oh yeah, on the ci failures: when running --with-flux-security, the ingest module checks that the signed jobspec matches the authenticated user. Those env vars are influencing the authenticated user but the signature comes from munge which can't be spoofed. but it looks like you just pushed changes to that effect?

I'm not 100% following. I was aware of the job-ingest module not working with the USERID & ROLEMASK environment variable test hacks. So in the tests, job submission does not use the environment variables. The environment variable hacks are only done on the flux job eventlog calls.

Most of the test failures appear to be a portability issue w/ printf and the -v option.

@chu11 chu11 force-pushed the chu11:issue2041_part2-eperm-checks branch 2 times, most recently from 64ee164 to 5d5af14 Mar 23, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

re-pushed hoping

+        eventlognew=$(echo -e "${eventlogsub}\nX")
+        eventlognew=${eventlognew%?}

is a portable way to add a newline to a string.

@chu11 chu11 force-pushed the chu11:issue2041_part2-eperm-checks branch from 5d5af14 to 7f4afb4 Mar 23, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

@garlick I just noticed the flux-job: 0: signer=2000 != requestor=9000 error in one of the CI tests. That's what I assume you were referring to. What's interesting is it only occurs on one test, not both of the times I try to submit. I'm guessing the first test fails and does not clear the environment variables. Leading to the issue on the second test.

My fix newline fix above did not appear to work. Ugh ...

@chu11 chu11 force-pushed the chu11:issue2041_part2-eperm-checks branch from 7f4afb4 to 21d28b2 Mar 23, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2019

ok, repush, lets try

+        eventlognew=$(printf "${eventlogsub}"'\n'"X")
+        eventlognew=${eventlognew%?}
@garlick

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

On my earlier comment re: ci failures: I had gone back to look at code I had briefly scanned and thought it had changed out from under me. Probably what happened was I misinterpreted it on first glance. Sorry for the noise!

@chu11 chu11 force-pushed the chu11:issue2041_part2-eperm-checks branch from 21d28b2 to d49162c Mar 25, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

rebased

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

hmmm, got a valgrind error, but not much to go on. I didn't get this error before I rebased.

expecting success: 
	run_timeout 120 \
	flux start -s ${VALGRIND_NBROKERS} --wrap=libtool,e,${VALGRIND} \
		--wrap=--tool=memcheck \
		--wrap=--leak-check=full \
		--wrap=--gen-suppressions=all \
		--wrap=--trace-children=no \
		--wrap=--child-silent-after-fork=yes \
		--wrap=--num-callers=30 \
		--wrap=--leak-resolution=med \
		--wrap=--error-exitcode=1 \
		--wrap=--suppressions=$VALGRIND_SUPPRESSIONS \
		-o,--shutdown-grace=${VALGRIND_SHUTDOWN_GRACE} \
		 ${VALGRIND_WORKLOAD}

flux-start: warning: setting --bootstrap=selfpmi due to --size option
==10922== Memcheck, a memory error detector
==10922== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==10922== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==10922== Command: /usr/src/src/broker/.libs/flux-broker --setattr=broker.rundir=/tmp/flux-10876-ePezWi/1 --setattr=tbon.endpoint=ipc://%B/req --shutdown-grace=16
==10922== 
==10921== Memcheck, a memory error detector
==10921== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==10921== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==10921== Command: /usr/src/src/broker/.libs/flux-broker --setattr=broker.rundir=/tmp/flux-10876-ePezWi/0 --setattr=tbon.endpoint=ipc://%B/req --shutdown-grace=16 /usr/src/t/valgrind/valgrind-workload.sh
==10921== 
FLUX_URI=local:///tmp/flux-10876-ePezWi/0
Running job
id=40248541184
id=46053457920
id=54911827968
id=61924704256
id=68182605824
id=75363254272
id=82409684992
id=89489670144
id=99069460480
id=105478356992
1553551433.554582 submit userid=2000 priority=16 flags=0
1553551438.502286 alloc note="rank0/core0"
1553551438.795803 start
1553551438.809766 finish status=0
1553551439.117221 release ranks="all" final=true
1553551439.147715 free
1553551439.148024 clean
++ flux job submit
++ flux jobspec srun -t 1 -n 1 /bin/true
+ id=208574349312
+ flux job wait-event 208574349312 start
1553551440.206890 start
+ flux job cancel 208574349312
+ flux job wait-event 208574349312 clean
1553551440.939137 clean
==10922== 
==10922== HEAP SUMMARY:
==10922==     in use at exit: 22,695 bytes in 58 blocks
==10922==   total heap usage: 32,636 allocs, 32,578 frees, 18,519,522 bytes allocated
==10922== 
==10922== LEAK SUMMARY:
==10922==    definitely lost: 0 bytes in 0 blocks
==10922==    indirectly lost: 0 bytes in 0 blocks
==10922==      possibly lost: 0 bytes in 0 blocks
==10922==    still reachable: 22,695 bytes in 58 blocks
==10922==         suppressed: 0 bytes in 0 blocks
==10922== Reachable blocks (those to which a pointer was found) are not shown.
==10922== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==10922== 
==10922== For counts of detected and suppressed errors, rerun with: -v
==10922== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
flux-start: 0 (pid 10921) Killed
not ok 1 - valgrind reports no new errors on 2 broker run
@grondo

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

flux-start: 0 (pid 10921) Killed

Looks like the killer-timeout exceeded?

json_decref (o);
return 0;
error:
free (event);

This comment has been minimized.

Copy link
@garlick

garlick Mar 26, 2019

Member

I think we need to save / restore errno in this block.

This comment has been minimized.

Copy link
@chu11

chu11 Mar 26, 2019

Author Contributor

ahhh yes, when we converted to json formatted contexts the json_decref was added. Thanks!

@garlick

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

Apologies, just noticed a small nit that I missed before and commented above.

I think it would be good actually to get #2104 in before this one if you have a sec to go look at it. @grondo and I both reviewed it, but we both contributed so it would be better form to have someone else push the button.

@chu11 chu11 force-pushed the chu11:issue2041_part2-eperm-checks branch from d49162c to c7234a3 Mar 26, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

rebased

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

hmmm. diff coverage isn't as good as I'd like it to be. With the recent change fell below 70%. Let me add some fake job submissions to try and eek out some extra percentage points.

@chu11 chu11 force-pushed the chu11:issue2041_part2-eperm-checks branch from c7234a3 to 40e6594 Mar 26, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

re-pushed with some extra tests to hopefully cover more error paths. Maybe it's excessive.

@chu11 chu11 force-pushed the chu11:issue2041_part2-eperm-checks branch from 40e6594 to 6db9150 Mar 26, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

i already changed my mind, I added too many error path tests, cut it back to something more reasonable. Just want to get close to 80% coverage.

@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

woo, hoo, 88% coverage

chu11 added 2 commits Mar 14, 2019
Add check that allows a guest user to access to an eventlog.

Closes #2041
Add new tests that allow guest users the ability to access eventlogs
for jobs they submitted.
@chu11 chu11 force-pushed the chu11:issue2041_part2-eperm-checks branch from 6db9150 to 0d81934 Mar 26, 2019
@chu11

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

rebased

@codecov-io

This comment has been minimized.

Copy link

commented Mar 26, 2019

Codecov Report

Merging #2098 into master will increase coverage by 0.01%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master    #2098      +/-   ##
==========================================
+ Coverage   80.26%   80.28%   +0.01%     
==========================================
  Files         197      197              
  Lines       31449    31483      +34     
==========================================
+ Hits        25244    25275      +31     
- Misses       6205     6208       +3
Impacted Files Coverage Δ
src/modules/job-info/job-info.c 71.29% <88.23%> (+3.29%) ⬆️
src/common/libflux/handle.c 83.71% <0%> (-0.68%) ⬇️
src/cmd/flux-job.c 84.51% <0%> (+0.27%) ⬆️
src/common/libflux/message.c 81.64% <0%> (+0.36%) ⬆️
@garlick

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

Awesome! Thanks @chu11!

@garlick garlick merged commit 5a5fce4 into flux-framework:master Mar 26, 2019
4 checks passed
4 checks passed
Mergify — Summary 1 potential rule
Details
codecov/patch 88.23% of diff hit (target 80.26%)
Details
codecov/project 80.28% (+0.01%) compared to 9628070
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.