make tls default if security is set...#519
Conversation
riedel
force-pushed
the
default-tls-496
branch
4 times, most recently
from
191d976 to
b08ec51
Compare
riedel
changed the title
… is given and tls is set
riedel
force-pushed
the
default-tls-496
branch
from
b08ec51 to
c375029
Compare
riedel
force-pushed
the
default-tls-496
branch
from
c3a9f42 to
3d6a576
Compare
riedel
force-pushed
the
default-tls-496
branch
from
b1b2267 to
467c328
Compare
riedel
changed the title
|
467c328 fixes #521 , this needs to be very carefully reviewed! It actually skips far more test than before. Those should be all duplicates, though. Why is this with in this PR? It is a rather stupid reason. The htcondor ci image had problems installing cryptography as a test dependency. IMHO it could make sense to split this PR. |
riedel
left a comment
riedel
left a comment
There was a problem hiding this comment.
For reference I commented on the changes I made
| if value is not None and "\n" in value: | ||
| f = tempfile.NamedTemporaryFile(mode="wt") | ||
| # make sure that tmpfile survives by keeping a reference | ||
| setattr(self, "_job_" + key, f) |
There was a problem hiding this comment.
this is ugly, but I having to clean up manually seemed more ugly. Ideas are welcome
There was a problem hiding this comment.
Can you explain what must be done here? Maybe @jacobtomlinson understands better.
There was a problem hiding this comment.
I am setting a reference to keep to the temp file from being deconstructed (which triggers its removal)
There was a problem hiding this comment.
Could you at least do this part in a separated function?
And just to be sure, does this creates a file in /tmp directory (which would not been shared with workers)? Or is this in the job execution folder? I guess it needs to be created in a shared folder?
|
I am happy to strip down some parts of the PR if anyone is too scared of merging it :) . |
guillaumeeb
left a comment
guillaumeeb
left a comment
There was a problem hiding this comment.
Overall this looks good, I'm just asking for some details in order to decide if we should merge it as is or not.
| if value is not None and "\n" in value: | ||
| f = tempfile.NamedTemporaryFile(mode="wt") | ||
| # make sure that tmpfile survives by keeping a reference | ||
| setattr(self, "_job_" + key, f) |
There was a problem hiding this comment.
Can you explain what must be done here? Maybe @jacobtomlinson understands better.
|
Like I said: I am happy to try moving the issues related to #521 out of this PR. It is true that the failing HTCondor CI dependency is not really a reason to include. The reason was that fixing the prebuild CI-Images cannot easily be done in the same PR and there seems not really a reason to fix it in a separate PR, because only this PR breaks it. So it was like a bit of a chicken and egg decision. Would fix this after this PR. However, if you think that #521 should not be fixed, I am happy to take another path. |
2 participants
… or add temporary security if none is given and tls is set. Further it adds support for in memory TLS keys for workers. Further unneccessary tests are skipped for cluster ci targets. that weren't marked.
fixes #496. #520 and #521