Fix regression with `OKTETO_TOKEN` and improve DevX for invalid token #4297

andreafalzetti · 2024-05-17T20:52:08Z

Proposed changes

This PR mainly includes a fix for a regression introduced in 2.27.1 causing the Okteto CLI to attempt calling the sunsetted Okteto Cloud when setting OKTETO_TOKEN without specifying the context. Additionally, the change also aims to improve the DevX of other scenario or misconfiguration of credentials when using the environment variable OKTETO_TOKEN.

In a previous PR we changed the order in which credentials are checked, causing the unsedired behavior because of left over logic from Okteto Cloud.

I've listed below the tested scenarios. A is the actual regression. From B onwards, are improvements, mainly making sure that for invalid token the error message is meaningful.

Note: all the tests below are okteto deploy tests, but this regression also applies to destroy and other commands. I've tested some of them, there are a lot of permutations, but I've tested the main ones.

How to validate

Scenario A (Regression)

Description: Context file configured correctly with an Okteto context + setting a valid OKTETO_TOKEN ✅
Command: OKTETO_TOKEN=$VALID_TOKEN okteto deploy (export VALID_TOKEN with a valid token)

2.26.0	2.27.1	NEW

Scenario B

Description: Context file configured correctly with an Okteto context + setting an invalid OKTETO_TOKEN ❌
Command: OKTETO_TOKEN=123 okteto deploy

2.26.0	2.27.1	NEW

Scenario C

Description: Empty context file + invalid OKTETO_TOKEN ❌
Command: OKTETO_HOME=$(mktemp -d) OKTETO_TOKEN=123 okteto-2.26.0 deploy

2.26.0	2.27.1	NEW

Scenario D

Description: Empty context file + valid OKTETO_TOKEN ✅
Command: OKTETO_HOME=$(mktemp -d) OKTETO_TOKEN=$VALID_TOKEN okteto-2.26.0 deploy (export VALID_TOKEN with a valid token)

2.26.0	2.27.1	NEW

Scenario E

Description: context file without current context + invalid OKTETO_TOKEN ❌
Command:

cd $(mktemp -d)
export OKTETO_HOME=$(pwd)
okteto context use <CTX> # open browser and login successfully

# delete the property `current-context` from the config.json
vim .okteto/context/config.json

OKTETO_TOKEN=123 okteto deploy`

2.26.0	2.27.1	NEW

Scenario F

Description: context file without current context + valid OKTETO_TOKEN ✅
Command:

cd $(mktemp -d)
export OKTETO_HOME=$(pwd)
okteto context use <CTX> # open browser and login successfully

# delete the property `current-context` from the config.json
vim .okteto/context/config.json

OKTETO_TOKEN=$VALID_TOKEN okteto deploy` (export `VALID_TOKEN` with a valid token)

2.26.0	2.27.1	NEW

CLI Quality Reminders

For both authors and reviewers:

Scrutinize for potential regressions
Ensure key automated tests are in place
Build the CLI and test using the validation steps
Assess Developer Experience impact (log messages, performances, etc)
If too broad, consider breaking into smaller PRs
Adhere to our code style and code review guidelines

pkg/okteto/client.go

andreafalzetti · 2024-05-17T21:29:33Z

cmd/context/create.go

@@ -146,10 +146,6 @@ func (c *Command) UseContext(ctx context.Context, ctxOptions *Options) error {
 		ctxOptions.IsOkteto = true
 	}

-	if ctxOptions.Context == okteto.CloudURL {


removing this fixes the regression

codecov · 2024-05-17T21:55:54Z

Codecov Report

Attention: Patch coverage is 22.22222% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 43.19%. Comparing base (1c72192) to head (b039f9b).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4297      +/-   ##
==========================================
- Coverage   43.21%   43.19%   -0.03%     
==========================================
  Files         371      371              
  Lines       30006    30015       +9     
==========================================
- Hits        12967    12964       -3     
- Misses      15916    15926      +10     
- Partials     1123     1125       +2

ifbyol

There is a variant of scenario B is not covered and it returns an Internal Server Error.

Instead of setting an invalid value, try to set a token from another instance and you would get the internal server error. Could we also cover that case?

ifbyol · 2024-05-20T11:53:06Z

cmd/test/cmd.go

@@ -111,6 +111,9 @@ func doRun(ctx context.Context, servicesToTest []string, options *Options, ioCtr

 	// Loads, updates and uses the context from path. If not found, it creates and uses a new context
 	if err := contextCMD.LoadContextFromPath(ctx, options.Namespace, options.K8sContext, options.ManifestPath, contextCMD.Options{Show: true}); err != nil {
+		if err.Error() == fmt.Errorf(oktetoErrors.ErrNotLogged, okteto.GetContext().Name).Error() {


We should probably create a specific error and being able to use error.Is() instead of comparing string values of the error. For example, not sure if we could have some problem with a trailing slash (/) at the end of the context name at some point.

Not needed to do as part of this PR, but it should be the way to go

@ifbyol I agree, that is what I've attempted to begin with, then realized that ErrNotLogged is being used like this all over, so it's tech debt that we need to adress. I didn't want to extend the scope of this change too much, but we should look into improve it!

Signed-off-by: Andrea Falzetti <andrea@okteto.com>

maroshii

All scenarios with the "NEW" changes work as expected. Deferring approval to @jLopezbarb or @ifbyol that have more context

…#4297) * improve devx for misconfigured credentials Signed-off-by: Andrea Falzetti <andrea@okteto.com> * handle additional scenario Signed-off-by: Andrea Falzetti <andrea@okteto.com> --------- Signed-off-by: Andrea Falzetti <andrea@okteto.com> (cherry picked from commit 0f2d6e0)

andreafalzetti force-pushed the af/dev-344-okteto-token-env-var-unexpected-err branch from 3e088c7 to 24b356f Compare May 17, 2024 21:19

andreafalzetti commented May 17, 2024

View reviewed changes

pkg/okteto/client.go Show resolved Hide resolved

andreafalzetti added release/bug-fix backport release-2.27 Backport this PR to CLI version 2.27 labels May 17, 2024

andreafalzetti commented May 17, 2024

View reviewed changes

andreafalzetti force-pushed the af/dev-344-okteto-token-env-var-unexpected-err branch from 24b356f to c01ab95 Compare May 17, 2024 21:44

andreafalzetti marked this pull request as ready for review May 17, 2024 22:14

andreafalzetti requested a review from a team as a code owner May 17, 2024 22:14

andreafalzetti added the run-e2e When used on a PR run windows & unix e2e label May 17, 2024

andreafalzetti changed the title ~~[🏗️ WIP] Improve DevX for misconfigured credentials~~ Fix regression and improve DevX for misconfigured credentials May 17, 2024

andreafalzetti changed the title ~~Fix regression and improve DevX for misconfigured credentials~~ Fix regression with OKTETO_TOKEN and improve DevX for invalid token May 17, 2024

ifbyol reviewed May 20, 2024

View reviewed changes

andreafalzetti added 2 commits May 20, 2024 14:08

improve devx for misconfigured credentials

582641f

Signed-off-by: Andrea Falzetti <andrea@okteto.com>

handle additional scenario

b039f9b

Signed-off-by: Andrea Falzetti <andrea@okteto.com>

andreafalzetti force-pushed the af/dev-344-okteto-token-env-var-unexpected-err branch from c01ab95 to b039f9b Compare May 20, 2024 13:11

maroshii reviewed May 20, 2024

View reviewed changes

jLopezbarb approved these changes May 20, 2024

View reviewed changes

andreafalzetti merged commit 0f2d6e0 into master May 20, 2024
12 of 14 checks passed

andreafalzetti deleted the af/dev-344-okteto-token-env-var-unexpected-err branch May 20, 2024 15:55

github-actions bot mentioned this pull request May 20, 2024

[Backport release-2.27] Fix regression with OKTETO_TOKEN and improve DevX for invalid token #4300

Merged

BrewTestBot mentioned this pull request Jun 5, 2024

okteto 2.28.0 Homebrew/homebrew-core#173784

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix regression with `OKTETO_TOKEN` and improve DevX for invalid token #4297

Fix regression with `OKTETO_TOKEN` and improve DevX for invalid token #4297

andreafalzetti commented May 17, 2024 •

edited

Loading

andreafalzetti May 17, 2024

codecov bot commented May 17, 2024 •

edited

Loading

ifbyol left a comment

ifbyol May 20, 2024

andreafalzetti May 20, 2024 •

edited

Loading

maroshii left a comment

Fix regression with OKTETO_TOKEN and improve DevX for invalid token #4297

Fix regression with OKTETO_TOKEN and improve DevX for invalid token #4297

Conversation

andreafalzetti commented May 17, 2024 • edited Loading

Proposed changes

How to validate

Scenario A (Regression)

Scenario B

Scenario C

Scenario D

Scenario E

Scenario F

CLI Quality Reminders

andreafalzetti May 17, 2024

Choose a reason for hiding this comment

codecov bot commented May 17, 2024 • edited Loading

Codecov Report

ifbyol left a comment

Choose a reason for hiding this comment

ifbyol May 20, 2024

Choose a reason for hiding this comment

andreafalzetti May 20, 2024 • edited Loading

Choose a reason for hiding this comment

maroshii left a comment

Choose a reason for hiding this comment

Fix regression with `OKTETO_TOKEN` and improve DevX for invalid token #4297

Fix regression with `OKTETO_TOKEN` and improve DevX for invalid token #4297

andreafalzetti commented May 17, 2024 •

edited

Loading

codecov bot commented May 17, 2024 •

edited

Loading

andreafalzetti May 20, 2024 •

edited

Loading