I have some interdiffs inlined. On Mon, Jun 22, 2015 at 12:12 PM, Hrvoje Ribicic <[email protected]> wrote:
> > > On Fri, Jun 19, 2015 at 3:30 PM, 'Lisa Velden' via ganeti-devel < > [email protected]> wrote: > >> Raise an OpPrereqError if secrt parameters are expected, but missing. >> > > s/secrt/secret/ > > >> Job retries result in this error. >> >> Signed-off-by: Lisa Velden <[email protected]> >> --- >> lib/cmdlib/instance_create.py | 6 ++++++ >> src/Ganeti/Constants.hs | 4 ++++ >> 2 files changed, 10 insertions(+) >> >> diff --git a/lib/cmdlib/instance_create.py b/lib/cmdlib/instance_create.py >> index e32e55f..eea2437 100644 >> --- a/lib/cmdlib/instance_create.py >> +++ b/lib/cmdlib/instance_create.py >> @@ -809,6 +809,12 @@ class LUInstanceCreate(LogicalUnit): >> self.op.osparams_private = serializer.PrivateDict() >> if self.op.osparams_secret is None: >> self.op.osparams_secret = serializer.PrivateDict() >> + else: >> + # check for missing secret parameters >> + for secret_param in self.op.osparams_secret: >> + if self.op.osparams_secret[secret_param].Get() == >> constants.REDACTED: >> + raise errors.OpPrereqError("Please re-submit secret parameters >> to" >> + " job.", errors.ECODE_INVAL) >> > > I do not like the location where this happens. > > The issue is that this takes care of instance creation only, and not > reinstalls. > This would be bad even if the appropriate code were to be added to the > reinstall operation as well, or refactored so that one function is used (as > it should, because we are repeating VERY similar actions, slightly > differently). > We should have the special handling in one place, and not scattered over > the codebase - it will just lead to more programming errors like this at a > later time. > > Can we work on moving this to the job queue part? > yes: diff --git a/lib/cmdlib/instance_create.py b/lib/cmdlib/instance_create.py index eea2437..e32e55f 100644 --- a/lib/cmdlib/instance_create.py +++ b/lib/cmdlib/instance_create.py @@ -809,12 +809,6 @@ class LUInstanceCreate(LogicalUnit): self.op.osparams_private = serializer.PrivateDict() if self.op.osparams_secret is None: self.op.osparams_secret = serializer.PrivateDict() - else: - # check for missing secret parameters - for secret_param in self.op.osparams_secret: - if self.op.osparams_secret[secret_param].Get() == constants.REDACTED: - raise errors.OpPrereqError("Please re-submit secret parameters to" - " job.", errors.ECODE_INVAL) self.os_full = cluster.SimpleFillOS( self.op.os_type, diff --git a/lib/jqueue/__init__.py b/lib/jqueue/__init__.py index fcddaef..f5b9a26 100644 --- a/lib/jqueue/__init__.py +++ b/lib/jqueue/__init__.py @@ -905,6 +905,15 @@ class _JobProcessor(object): timeout = opctx.GetNextLockTimeout() try: + # check for missing secret parameters + for op in self.job.ops: + if hasattr(op.input, "osparams_secret"): + for secret_param in op.input.osparams_secret: + if (op.input.osparams_secret[secret_param].Get() + == constants.REDACTED): + raise errors.OpPrereqError("Please re-submit secret parameters" + " to job.", errors.ECODE_INVAL) + # Make sure not to hold queue lock while calling ExecOpCode result = self.opexec_fn(op.input, _OpExecCallbacks(self.queue, self.job, op), > > >> >> self.os_full = cluster.SimpleFillOS( >> self.op.os_type, >> diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs >> index d879cd8..b4f4256 100644 >> --- a/src/Ganeti/Constants.hs >> +++ b/src/Ganeti/Constants.hs >> @@ -5262,6 +5262,10 @@ debugModeConfidentialityWarning = >> "ALERT: %s started in debug mode.\n\ >> \ Private and secret parameters WILL be logged!\n" >> >> +-- | Use to hide secret parameter value >> +redacted :: String >> +redacted = "<redacted>" >> > > As this is already defined in Types.hs, you can export and use it: > > redacted = Types.redacted > > The constant will be generated correctly in the Python code. > Interdiff: diff --git a/src/Ganeti/Constants.hs b/src/Ganeti/Constants.hs index b4f4256..23dc457 100644 --- a/src/Ganeti/Constants.hs +++ b/src/Ganeti/Constants.hs @@ -5264,7 +5264,7 @@ debugModeConfidentialityWarning = -- | Use to hide secret parameter value redacted :: String -redacted = "<redacted>" +redacted = Types.redacted -- * Stat dictionary entries -- diff --git a/src/Ganeti/Types.hs b/src/Ganeti/Types.hs index c9bcd8c..52c30f1 100644 --- a/src/Ganeti/Types.hs +++ b/src/Ganeti/Types.hs @@ -177,6 +177,7 @@ module Ganeti.Types , Secret(..) , showSecretJSObject , revealValInJSObject + , redacted , HvParams , OsParams , OsParamsPrivate > > >> + >> -- * Stat dictionary entries >> -- >> -- The get_file_info RPC returns a number of values as a dictionary, and >> the >> -- >> 2.4.3.573.g4eafbef >> >> > Hrvoje Ribicic > Ganeti Engineering > Google Germany GmbH > Dienerstr. 12, 80331, München > > Geschäftsführer: Graham Law, Christine Elizabeth Flores > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > -- Lisa Velden Software Engineer [email protected] Google Germany GmbH Dienerstraße 12 80331 München Geschäftsführer: Graham Law, Christine Elizabeth Flores Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg
