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

libflux: drop flux_sec_t class from public API #1846

Merged
merged 5 commits into from
Nov 20, 2018

Conversation

garlick
Copy link
Member

@garlick garlick commented Nov 19, 2018

As suggested in #1843, the flux_sec_t code is only used by the broker and flux-keygen and doesn't have an out of tree use case that I can think of. It's also old code in need of a makeover.

Hence, move it to libutil/zecurity.[ch] and re-namespace from flux_sec_* to zsecurity_*

@SteVwonder
Copy link
Member

sec.py should be removed from fluxpy_PYTHON in src/bindings/python/flux/Makefile.am.

In t/valgrind/valgrind.supp there is a reference to fun:flux_sec_comms_init that should change to fun:zsecurity_comms_init.

Otherwise, LGMT! Thanks @garlick!

@garlick
Copy link
Member Author

garlick commented Nov 19, 2018

Thanks @SteVwonder! That was timely as I've got to put my laptop away and hit the road.

I had misspelled "python" in one of those commits, so I just went ahead and forced a push with your suggested changes.

Problem: libflux/security calls flux_strerror() which
creates an unnecessary dependency on libflux.

Switch to zmq_strerror() or strerror() where appropriate.
Problem: flux_sec_t is being removed from the public API.

Drop the python wrapper for it.
Problem: the flux_sec_t "class" is only used by the
broker and flux-keygen, and should be removed from
the public API.

Relocate to libutil/zsecurity.

Fixes flux-framework#1843
Problem: flux_sec_ functions, now intended to be private
to flux-core, still leak out because of flux_ prefix.

Rename flux_sec_* to zsecurity_*.
Rneame FLUX_SEC_* to ZSECURITY_*.

Leave the FLUX_SEC_DIRECTORY environment variable that
points to the location of certs as is.

Update valgrind.supp.
@garlick
Copy link
Member Author

garlick commented Nov 19, 2018

Crud, still looks like it's failing something in python land. I went ahead and rebased.

@SteVwonder
Copy link
Member

Looks like pylint is unhappy because flux/__init__.py still lists sec under __all__: https://github.com/flux-framework/flux-core/blob/master/src/bindings/python/flux/__init__.py#L15 Removing it from the list should fix that. The binding tests still work because none of them leverage __all__ like via a wildcard import (from flux import *), which is bad form anyway.

Pylint error message:

************* Module flux
E: 15,11: Undefined variable name 'sec' in __all__ (undefined-all-variable)

I'm not sure why the first builder (with no configure flags) failed. All I found was:

       t2202-job-manager.t:  PASS: N=30  PASS=30  FAIL=0 SKIP=0 XPASS=0 XFAIL=0
ERROR: t2202-job-manager.t - exited with status 137 (terminated by signal 9?)

Maybe just a transient error? Restarting that builder just to see what happens.

@codecov-io
Copy link

Codecov Report

Merging #1846 into master will decrease coverage by 0.03%.
The diff coverage is 71.64%.

@@            Coverage Diff             @@
##           master    #1846      +/-   ##
==========================================
- Coverage   79.91%   79.87%   -0.04%     
==========================================
  Files         196      196              
  Lines       35267    35267              
==========================================
- Hits        28185    28171      -14     
- Misses       7082     7096      +14
Impacted Files Coverage Δ
src/broker/broker.c 77.59% <37.5%> (ø) ⬆️
src/cmd/flux-keygen.c 54.83% <55.55%> (ø) ⬆️
src/broker/overlay.c 84.58% <57.14%> (ø) ⬆️
src/common/libutil/zsecurity.c 76.47% <83.72%> (ø)
src/common/libflux/response.c 79.62% <0%> (-1.24%) ⬇️
src/modules/connector-local/local.c 73.77% <0%> (-1.04%) ⬇️
src/broker/modservice.c 78.84% <0%> (-0.97%) ⬇️
src/common/libflux/message.c 81.15% <0%> (-0.49%) ⬇️

@garlick
Copy link
Member Author

garlick commented Nov 20, 2018

Looks like that restarted builder must've completed OK.

Thanks for the __init.py__ hint. Just pushed that and it seems like it worked.

@garlick
Copy link
Member Author

garlick commented Nov 20, 2018

This might be OK to merge despite the so-so coverage, since it's for the most part a straightforward move, coverage problems are pre-existing, and code needs to be reworked at some point.

Copy link
Member

@SteVwonder SteVwonder left a comment

Choose a reason for hiding this comment

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

Yeah. I'm happy to merge as is. I'll wait for one other person to weigh in though (@chu11 or @grondo?).

Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

Yeah, seems fine to me too

@grondo
Copy link
Contributor

grondo commented Nov 20, 2018

Merging so we can move other PRs along

@grondo grondo merged commit 8dd15c9 into flux-framework:master Nov 20, 2018
@garlick
Copy link
Member Author

garlick commented Nov 20, 2018

Thanks!

@garlick garlick deleted the flux_sec_private branch November 21, 2018 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants