diff --git a/CHANGELOG.md b/CHANGELOG.md index dcda0f7d6..21de3b356 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - `dr workload config --import-env` adds the `.env` variables a manifest does not declare yet, and changes nothing else. Until now `.env` was read exactly once, when setup wrote `.datarobot.yaml`, so a variable added afterwards never reached the container and no command said so: the only route was a hand edit of the manifest plus a hand-made credential for anything secret. The import is additive and one-directional. A name the file already carries keeps its value, because the manifest is what is deployed and `.env` is a local copy allowed to drift; a name dropped from `.env` is left alone, because removing a line from your own file says nothing about what the workload should run. Secrets are stored as credentials the way setup stores them, and the file is edited in place, so comments and hand-tuned keys survive. It is opt-in, and exclusive with `--skip-env` and with every setup answer: re-reading `.env` on every run would carry whatever it grew since into a deploy nobody asked to change, and a flag an import cannot apply is refused rather than dropped. The edit is refused outright, before any credential is created, for a manifest with nowhere to put the result: one that names its artifact by id, one where any step from the artifact down to the variable list is shared through a YAML anchor, alias or merge key, one whose `environmentVars` is not a list, and one holding more than a single YAML document. The refusal is the edit itself, rendered and thrown away, so it cannot disagree with the write it guards. The edited file is then parsed and validated before it reaches the disk. - `dr workload config --update-env` brings the variables a manifest already declares back in line with `.env`: a literal is rewritten in the file, and the credential behind a secret is re-sent so a rotated key reaches the workload without a new credential, a new id or an edit to the manifest. It is the counterpart of `--import-env` and deliberately a separate flag, because it is the opposite act: one adds a name and never touches a value, the other touches values and never adds a name. Pass both to do both. A secret is re-sent without being compared first, because it cannot be: the platform never returns a stored value, so nothing can tell a rotated key from an untouched one, and a run reporting "no change" for a key you had just rotated would be worse than one request too many. A re-send reaches a running workload only when its containers restart, because a container reads its credentials at startup and `dr workload up` finds the manifest unchanged and does nothing, so the run says which commands make the new value the one being served. A re-send that fails is named, because it leaves the workload serving the value it already had and nothing downstream would notice; under `--output-format json` there is no stderr for that line to have used, so the envelope carries the counts of what was re-sent and what was not. A dry run says how many secrets it would re-send rather than staying silent about the one part of the flag that reaches the tenant. A run that only re-sent secrets reports that the manifest was not changed, because it was not: the value went to the credential store and the file naming it is byte for byte what it was. Only a credential this project created is ever re-sent, checked by name before anything is sent: the store is tenant-wide, so an id pasted by hand may name one other workloads read, and a flag about this project's `.env` has no business overwriting it. A reference to any field other than the one the CLI writes is refused for the same reason. +- `dr workload up --import-env` and `dr workload up --update-env` do the same two edits as part of a deploy, so adding a variable or rotating a key is one command rather than two. They are the same act `up` already performs on its first run, where a project with no manifest gets the wizard, which reads `.env`, classifies it and mints the credentials: without these the second run was the only one that could not, which made the file a deploy reads something you had to leave the command to change. Both stay opt-in, which is what keeps a deploy a function of the committed repo: a run without them reads nothing but the manifest, so a fresh CI clone with no `.env` deploys exactly what your working copy deploys. The edit runs before the plan, so what the plan shows and what the deploy carries is the file as edited, and `--dry-run` previews the edit without writing it, saying outright that the plan below is for the manifest as it stands. A re-sent secret is the one edit a deploy cannot finish: it changes no file, so a deploy with nothing else to do replaces no container, and the run says so and names the restart rather than reporting the workload as up to date and leaving it at that. - `dr workload config` over an existing manifest now names the `.env` variables the file does not declare, and the flag that adds them. Names only, never values, matching what the deploy plan already redacts. Silent when there is no `.env`, which is the ordinary CI case, and under `--skip-env`. Under `--output-format json` the wizard is given no stderr at all, so `2>&1 | jq .` parses; the one thing that would otherwise be lost, a `.env` that could not be read, becomes an error there rather than a warning nobody sees. It also names a declared variable whose `.env` value no longer matches the manifest, and the variables stored as credentials, whose values cannot be compared at all: saying so outright beats a silence that reads as "those are fine" while a rotated key sits unused. And it names what was held back rather than added: a variable the classifier read as local-only, and an entry left naming the credential placeholder by a store that could not be reached, which a later import skips because the name counts as declared. - `dr workload status`, `dr workload get`, `dr workload endpoint` and `dr workload logs` now take the workload id as an optional argument. Left out, it is read from the `workloadId` in the nearest `.datarobot.yaml`, searched upward from the new `--dir` flag (the current directory by default), so the commands `dr workload up` points you at can be run as printed from the project it just deployed. A typed id still wins, and is used without reading any manifest. The workload that was picked is named on stderr, except under `--output-format json`, where anything on stderr would break `2>&1 | jq .`. - `dr workload stop`, `dr workload start` and `dr workload delete` take the workload id optionally too, on the same terms. Because they change something, a workload named by the manifest rather than by you is confirmed first; a typed id is never questioned. `--yes` skips the question, and `stop` and `start` gained that flag for this. `DATAROBOT_CLI_NON_INTERACTIVE=1` also skips it on `stop` and `start`, but not on a manifest-named `delete`: that variable is set once across a pipeline, and deleting something nobody named is not what it was set for. diff --git a/cmd/workload/up/cmd.go b/cmd/workload/up/cmd.go index 135793a72..f310ec063 100644 --- a/cmd/workload/up/cmd.go +++ b/cmd/workload/up/cmd.go @@ -69,6 +69,54 @@ type upResult struct { Action string `json:"action"` Locked bool `json:"locked"` Plan up.PlanJSON `json:"plan"` + // Env is what --import-env and --update-env did before the plan was + // computed. A rotation edits no file and shows in no plan, so without + // these a run that re-sent a secret is indistinguishable from one that + // did nothing at all. + Env envJSON `json:"env"` +} + +// envJSON is the .env re-entry's side of a deploy. +type envJSON struct { + KeysAdded int `json:"keysAdded"` + ValuesUpdated int `json:"valuesUpdated"` + SecretsRotated int `json:"secretsRotated"` + SecretsFailed int `json:"secretsNotRotated"` + SecretsPending int `json:"secretsPending"` +} + +// warnSecretStillServing covers the one thing --update-env can do that a +// deploy cannot finish: re-send a secret. +// +// The credential store takes the new value immediately, but a container reads +// its credentials when it starts, so the workload keeps serving the old one +// until it is replaced. A deploy that had something else to do replaces it on +// the way past; a deploy that found nothing else leaves the rotation sitting +// in the store, under a summary that says the workload is up to date. It is, +// about the manifest, which is why this says the other half out loud. +func warnSecretStillServing(stderr io.Writer, result up.Result, f flags) { + if f.dryRun || result.Env.SecretsRotated == 0 || result.Action != up.ActionUnchanged { + return + } + + dir := manifest.DirFlag(f.dir) + + fmt.Fprintf(stderr, + "\n %s %d re-sent %s reached the credential store, and this deploy replaced no container, "+ + "so the workload still serves the value it started with.\n Restart it to pick %s up:\n"+ + " dr workload stop --yes%s\n dr workload start --yes%s\n", + tui.WarnStyle.Render("!"), result.Env.SecretsRotated, + plural(result.Env.SecretsRotated, "secret", "secrets"), + plural(result.Env.SecretsRotated, "it", "them"), dir, dir) +} + +// plural picks the word for count, the way the wizard's own reporting does. +func plural(count int, one, many string) string { + if count == 1 { + return one + } + + return many } // buildID is the envelope's build reference: the id when a build ran, null @@ -82,12 +130,14 @@ func buildID(id string) *string { } type flags struct { - dir string - yes bool - dryRun bool - detach bool - lock bool - force bool + dir string + yes bool + dryRun bool + detach bool + lock bool + force bool + importEnv bool + updateEnv bool // bindingFlags exist only to be refused. Cobra's own "unknown flag" // message would leave the user guessing where binding lives, and these @@ -202,6 +252,18 @@ func addFlags(cmd *cobra.Command, f *flags, poll *pollflags.Set) { cmd.Flags().BoolVar(&f.force, "force-build", false, "Rebuild the image even when the working tree matches what was last synced.") + // The same two flags `dr workload config` takes, because the first run of + // this command already reads .env: with no manifest it is the wizard. A + // deploy stays a function of the committed repo, since neither flag does + // anything unless it is passed. + cmd.Flags().BoolVar(&f.importEnv, "import-env", false, + "Before deploying, add the .env variables the manifest does not declare yet. "+ + "Secrets are stored as credentials, the same way setup stores them.") + cmd.Flags().BoolVar(&f.updateEnv, "update-env", false, + "Before deploying, bring the variables the manifest already declares back in line with .env: "+ + "a literal is rewritten and the credential behind a secret is re-sent. A re-sent secret reaches "+ + "the containers this deploy replaces; if the deploy has nothing else to do, it will not replace them.") + cmd.Flags().StringVar(&f.workloadID, "workload-id", "", "") cmd.Flags().StringVar(&f.name, "name", "", "") _ = cmd.Flags().MarkHidden("workload-id") @@ -243,6 +305,8 @@ func run(cmd *cobra.Command, f flags, poll pollflags.Set, format outputformat.Ou Lock: f.lock, Confirm: rollConfirm(cmd, yes), ForceBuild: f.force, + ImportEnv: f.importEnv, + UpdateEnv: f.updateEnv, PollInterval: poll.Interval, PollTimeout: poll.Timeout, Stderr: cmd.ErrOrStderr(), @@ -411,9 +475,18 @@ func render(cmd *cobra.Command, f flags, format outputformat.OutputFormat, resul Action: result.Action, Locked: result.Locked, Plan: result.Plan.JSON(), + Env: envJSON{ + KeysAdded: result.Env.KeysAdded, + ValuesUpdated: result.Env.ValuesUpdated, + SecretsRotated: result.Env.SecretsRotated, + SecretsFailed: result.Env.SecretsFailed, + SecretsPending: result.Env.SecretsPending, + }, }) } + warnSecretStillServing(cmd.ErrOrStderr(), result, f) + draft := draftIsServing(f, result, failed) if f.dryRun { diff --git a/internal/workload/up/load.go b/internal/workload/up/load.go index c412788e6..a2f2fc9a3 100644 --- a/internal/workload/up/load.go +++ b/internal/workload/up/load.go @@ -44,6 +44,10 @@ type Loaded struct { // shell happened to be sitting in. ProjectDir string + // Env is what a --import-env or --update-env run did to this file before + // it was read, zero when neither was asked for. + Env EnvEdit + // Manifest is the parse tree, kept so later stages can ask it questions // without re-reading the file. Manifest *manifest.Manifest diff --git a/internal/workload/up/load_test.go b/internal/workload/up/load_test.go index cf8fc4b31..35a91373c 100644 --- a/internal/workload/up/load_test.go +++ b/internal/workload/up/load_test.go @@ -17,9 +17,11 @@ package up import ( "os" "path/filepath" + "strings" "testing" "github.com/datarobot/cli/internal/workload/manifest" + "github.com/datarobot/cli/internal/workload/wizard" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -261,3 +263,87 @@ func TestLoad_ADirectoryAtThePathIsNotAManifest(t *testing.T) { require.Error(t, err) assert.ErrorIs(t, err, ErrNoManifest) } + +// The flag exists because the first run of this command already reads .env: +// with no manifest `up` is the wizard. The edit has to happen before the file +// is read, or the deploy would carry the version from before it. +func TestLoad_ImportEnvEditsTheManifestBeforeReadingIt(t *testing.T) { + dir := t.TempDir() + path := writeManifest(t, dir, boundManifest) + + swap(t, &runWizardFn, func(opts wizard.Options) (wizard.Result, error) { + assert.True(t, opts.ImportEnv) + assert.Equal(t, dir, opts.Dir, "the edit lands next to the manifest being deployed") + + // What the real wizard does: the variable is in the file by the time + // it returns. + edited := strings.Replace(boundManifest, + " - name: LOG_LEVEL", + " - name: REGION\n value: eu-west-1\n - name: LOG_LEVEL", 1) + require.NoError(t, os.WriteFile(path, []byte(edited), 0o600)) + + return wizard.Result{Path: path, Action: wizard.ActionUpdated, EnvKeysListed: 1}, nil + }) + + loaded, err := load(dir, Options{ImportEnv: true}) + require.NoError(t, err) + + assert.Equal(t, 1, loaded.Env.KeysAdded) + assert.Contains(t, loaded.Manifest.EnvVarNames(), "REGION", + "the deploy plans from the file as edited, not as it was") +} + +// A rotation writes to the credential store and leaves the file alone, so the +// count is the only trace of it: the plan below has nothing to show. +func TestLoad_UpdateEnvCarriesTheRotationCount(t *testing.T) { + dir := t.TempDir() + writeManifest(t, dir, boundManifest) + + swap(t, &runWizardFn, func(opts wizard.Options) (wizard.Result, error) { + assert.True(t, opts.UpdateEnv) + + return wizard.Result{Action: wizard.ActionUnchanged, EnvSecretsRotated: 2}, nil + }) + + loaded, err := load(dir, Options{UpdateEnv: true}) + require.NoError(t, err) + + assert.Equal(t, 2, loaded.Env.SecretsRotated) + assert.Equal(t, 0, loaded.Env.KeysAdded) +} + +// Without the flags nothing reads .env, which is what keeps a deploy a +// function of the committed repo: a fresh CI clone has no .env at all. +func TestLoad_WithoutTheFlagsTheWizardIsNotRun(t *testing.T) { + dir := t.TempDir() + writeManifest(t, dir, boundManifest) + + swap(t, &runWizardFn, func(wizard.Options) (wizard.Result, error) { + t.Fatal("the deploy read .env without being asked to") + + return wizard.Result{}, nil + }) + + _, err := load(dir, Options{}) + require.NoError(t, err) +} + +// A dry run promises the file is not touched, and an import that minted a +// credential and rewrote the manifest would have changed two things the plan +// then says it is not going to do. +func TestLoad_DryRunPreviewsTheEdit(t *testing.T) { + dir := t.TempDir() + writeManifest(t, dir, boundManifest) + + swap(t, &runWizardFn, func(opts wizard.Options) (wizard.Result, error) { + assert.True(t, opts.DryRun, "the preview must reach the wizard, or it writes") + + return wizard.Result{Action: wizard.ActionPlanned, EnvKeysListed: 1}, nil + }) + + loaded, err := load(dir, Options{ImportEnv: true, DryRun: true}) + require.NoError(t, err) + + assert.Equal(t, 1, loaded.Env.KeysAdded) + assert.NotContains(t, loaded.Manifest.EnvVarNames(), "REGION") +} diff --git a/internal/workload/up/run.go b/internal/workload/up/run.go index b3b4e0773..d648b66be 100644 --- a/internal/workload/up/run.go +++ b/internal/workload/up/run.go @@ -109,6 +109,19 @@ type Options struct { // fields the file moved, and the run would have to be read to know which. Lock bool + // ImportEnv and UpdateEnv are the .env re-entry `dr workload config` + // takes, offered here because the first run of this command already does + // it: with no manifest `up` is the wizard, and the wizard reads .env, + // classifies it and mints the credentials. Without these the second run + // is the only one that cannot, which makes the file the deploy reads + // something you have to leave the command to change. + // + // Opt-in, and that is what keeps a deploy a function of the committed + // repo: a run without them reads nothing but the manifest, so a fresh CI + // clone with no .env deploys exactly what your laptop deploys. + ImportEnv bool + UpdateEnv bool + // PollInterval and PollTimeout tune the waits. PollInterval time.Duration PollTimeout time.Duration @@ -138,6 +151,22 @@ type Result struct { // the artifact was still current. A build that failed still sets it, so // the caller can say which logs to read. BuildID string + + // Env is what --import-env and --update-env did to the manifest before + // the plan was computed, zero when neither was asked for. The counts + // travel because a rotation is the one edit the plan cannot show: it + // changes no file, so a run that re-sent a secret and deployed nothing + // would otherwise report itself as a deploy that did nothing at all. + Env EnvEdit +} + +// EnvEdit is the .env re-entry's side of a deploy. +type EnvEdit struct { + KeysAdded int + ValuesUpdated int + SecretsRotated int + SecretsFailed int + SecretsPending int } // Run reads, plans, and applies as much of the plan as this release can. @@ -191,6 +220,10 @@ func Run(opts Options) (Result, error) { // below this. What was wanted stays readable under plan.action. Action: ActionUnchanged, Locked: live.Locked, + // What the .env flags did before any of this, so a run that re-sent a + // secret and found nothing else to do can say so: that edit changes no + // file and appears in no plan. + Env: loaded.Env, } if err := bindLocked(loaded, plan, &result); err != nil { @@ -484,7 +517,11 @@ func noteUnusedForce(plan Plan, opts Options) { // is the one thing this command must never do. func load(dir string, opts Options) (Loaded, error) { loaded, err := Load(dir) - if err == nil || !errors.Is(err, ErrNoManifest) { + if err == nil { + return opts.editEnv(loaded) + } + + if !errors.Is(err, ErrNoManifest) { return loaded, err } @@ -508,6 +545,115 @@ func load(dir string, opts Options) (Loaded, error) { return Load(dir) } +// editEnv applies the .env re-entry this run asked for, against the manifest +// this deploy is about, and re-reads the file when the edit moved it. +// +// It runs before the plan rather than after, so what reaches the platform is +// the file as edited: an import that landed and a deploy that did not carry it +// would leave the variable in the manifest, absent from the container, and +// nothing to say which of the two runs was wrong. +// +// The wizard is given the manifest's own directory rather than this command's, +// because `up` searches upward for the file and the edit has to land on the one +// being deployed, next to the .env that pairs with it. +func (o Options) editEnv(loaded Loaded) (Loaded, error) { + if !o.ImportEnv && !o.UpdateEnv { + return loaded, nil + } + + dir := filepath.Dir(loaded.Path) + + // A dry run previews the edit and writes nothing, which is the only way + // `up --dry-run` can keep its promise: an import that minted a credential + // and rewrote the manifest would have changed two things the plan below + // then reports it is not going to do. + edit, err := runWizardFn(wizard.Options{ + Dir: dir, + NonInteractive: true, + DryRun: o.DryRun, + ImportEnv: o.ImportEnv, + UpdateEnv: o.UpdateEnv, + Stderr: o.Stderr, + }) + if err != nil { + return Loaded{}, err + } + + // Only a real write moves the file the plan reads. A dry run reports + // planned and leaves the manifest where it was, so the deploy below plans + // from what is on disk, which is what a dry run is for. + o.reportEnvEdit(edit) + + if edit.Action != wizard.ActionUpdated { + return withEnvEdit(loaded, edit), nil + } + + // The file the plan is about to be computed from is not the one already + // read, so it is read again rather than patched in memory: the deploy + // validates and compiles what is on disk, and that is what a later run + // will compare against. + reloaded, err := Load(dir) + if err != nil { + return Loaded{}, err + } + + return withEnvEdit(reloaded, edit), nil +} + +// reportEnvEdit says what the flags did to the manifest, before the plan that +// was computed from the result of it. +// +// The wizard has already reported what it sent to the credential store, which +// is the half that happens off this machine. What is left is the half that +// happened to the file, and on a dry run the fact that the plan below knows +// nothing about it: nothing was written, so the plan describes the manifest as +// it stands rather than as the flags would leave it. +func (o Options) reportEnvEdit(edit wizard.Result) { + if o.Stderr == nil { + return + } + + var parts []string + + if edit.EnvKeysListed > 0 { + parts = append(parts, fmt.Sprintf("%d %s added from %s", + edit.EnvKeysListed, wizard.Plural(edit.EnvKeysListed, "variable", "variables"), wizard.EnvFileName)) + } + + if edit.EnvValuesUpdated > 0 { + parts = append(parts, fmt.Sprintf("%d declared %s updated to match %s", + edit.EnvValuesUpdated, wizard.Plural(edit.EnvValuesUpdated, "value", "values"), wizard.EnvFileName)) + } + + if len(parts) == 0 { + return + } + + if o.DryRun { + fmt.Fprintf(o.Stderr, " %s would have %s. The plan below is for %s as it stands.\n", + tui.HintStyle.Render("~"), strings.Join(parts, ", "), manifest.FileName) + + return + } + + fmt.Fprintf(o.Stderr, " %s %s: %s.\n", + tui.SuccessStyle.Render("✓"), manifest.FileName, strings.Join(parts, ", ")) +} + +// withEnvEdit carries the wizard's counts onto the load, so the run can report +// an edit the plan has no way to show. +func withEnvEdit(loaded Loaded, edit wizard.Result) Loaded { + loaded.Env = EnvEdit{ + KeysAdded: edit.EnvKeysListed, + ValuesUpdated: edit.EnvValuesUpdated, + SecretsRotated: edit.EnvSecretsRotated, + SecretsFailed: edit.EnvSecretsNotRotated, + SecretsPending: edit.EnvSecretsPending, + } + + return loaded +} + // boundID is the workload this run is about, for the envelope and for a // failure that has to report something. //