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

runtime/lava: Add default way to get API token if not set #2258

Closed
wants to merge 1 commit into from

Conversation

nuclearcat
Copy link
Member

As pipeline.yaml is not considered "secure" file, we might not set API token in it, then retrieve it from os environment.

@nuclearcat nuclearcat force-pushed the add-api-token branch 2 times, most recently from 2eb6474 to 5fbbf35 Compare December 21, 2023 15:06
@gctucker
Copy link
Contributor

But, why would the API token be retrieved from YAML anyway? It's meant to be loaded from the TOML secrets. That's already implemented in kernelci.settings.Secrets.

Copy link
Contributor

@gctucker gctucker left a comment

Choose a reason for hiding this comment

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

I think you misunderstood how this was meant to work.

@@ -180,6 +181,16 @@ def get_params(self, job, api_config=None):
return params

def generate(self, job, params):
# if LAVA runtime dont have set notify.callback.token
Copy link
Contributor

Choose a reason for hiding this comment

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

And, why set some default callback attributes? The idea is that if there's no callback or "notify" attribute then the job just doesn't generate a callback when it completes. Loading API tokens from the environment is not going to work here. Also, the tokens are stored in the LAVA database anyway.

@gctucker
Copy link
Contributor

But, why would the API token be retrieved from YAML anyway?

Actually, I see this is for callbacks. The API tokens are stored in the LAVA database, they don't need to be known when generating the job.

@nuclearcat nuclearcat force-pushed the add-api-token branch 2 times, most recently from 8770b78 to 1bfcdb7 Compare December 21, 2023 21:57
@nuclearcat
Copy link
Member Author

But, why would the API token be retrieved from YAML anyway?

Actually, I see this is for callbacks. The API tokens are stored in the LAVA database, they don't need to be known when generating the job.

I think you are confusing API token used for callbacks and LAVA token. LAVA token is fine, but i'm having issues with callback token which is equal to API/pipeline API token and set in LAVA job template separately.
Ref: kernelci/kernelci-pipeline#377 (comment)

@gctucker
Copy link
Contributor

That's what I mean, the API token is stored in the LAVA database and used in the callback request sent by LAVA. I don't see what you're trying to fix here.

@nuclearcat
Copy link
Member Author

nuclearcat commented Dec 21, 2023

It's fixed now:

today at 1:07:00 AM[pid: 9|app: 0|req: 2/2] 159.223.222.84 () {44 vars in 672 bytes} [Thu Dec 21 23:06:58 2023] POST /node/6584c4c8866c410a6ca19fcc => generated 6260 bytes in 1419 msecs (HTTP/1.1 200) 2 headers in 73 bytes (2 switches on core 0)

LAVA callbacks was not working.
As i said, with current code:

  1. it is taking from config:
  lava-collabora: &lava-collabora-staging
    lab_type: lava
    url: https://lava.collabora.dev/
    priority_min: 40
    priority_max: 60
    notify:
      callback:
        token: kernelci-api-token-staging
        url: https://staging.kernelci.org:9100

token literally set to "kernelci-api-token-staging"
2)LAVA job template created have this code:

{% if notify %}
notify:
  callback:
    content-type: {{ notify.callback['content-type']|default('json') }}
    dataset: {{ notify.callback.dataset|default('all') }}
    method: {{ notify.callback.method|default('POST') }}
    {%- if notify.callback.token %}
    token: {{ notify.callback.token }}

It will add value in yaml: notify.callback.token and send to LAVA. This means it will be string "kernelci-api-token-staging" here.

3)LAVA after completing job will request callback URL with header "Authentication: kernelci-api-token-staging".
4)lava-callback.py retrieve this header value and trying to use as API token. Obviously "kernelci-api-token-staging" is not valid API token. I was getting error 401 and no node are created.

Now i am using 16 chars truncated API_TOKEN as callback token and it is working.
UPD: I use truncated token just for callback authentication, and real api token for lava-callback i get from environment.

@gctucker
Copy link
Contributor

OK, the problem here is that there isn't a callback token set up as expected in the LAVA database. So instead of inserting the API token it just puts the name by default. The API tokens are never stored in YAML, it's just a LAVA "feature" which should probably be changed. Maybe with some private jobs it's OK to have the token directly in the job definition, but that's never the case for KernelCI.

As pipeline.yaml is not considered "secure" file, we might not
set API token in it, then retrieve it from os environment.
Also because LAVA have limit on token length - truncate it to 16
characters.

Signed-off-by: Denys Fedoryshchenko <[email protected]>
@pawiecz
Copy link
Contributor

pawiecz commented Jan 4, 2024

OK, the problem here is that there isn't a callback token set up as expected in the LAVA database.

True - I verified this in both staging and production LAVA instances: there is no kernelci-api-token-staging auth token present.

I had a look at the docs which could clarify this process a bit: https://kernelci.org/docs/labs/lava/#steps-to-add-a-lava-lab-to-kernelciorg

@nuclearcat do you think that extending them with e.g. flow diagram (who generates tokens, how to store them) would make this process less error prone?

To address another issue that did not come up in this thread but was raised during various discussions: auth token value can be set arbitrarily through admin panel:

Screenshot from 2024-01-04 11-38-55

To sum up - I would propose three actions:

  1. Revise docs: https://kernelci.org/docs/labs/lava/#steps-to-add-a-lava-lab-to-kernelciorg
  2. Extend token management docs with flow diagram: who generates tokens, how to store them
  3. Verify tokens presence in respective systems (LAVA, KCI instances)

After these tasks are complete this PR could be closed (or converted to the issue earlier).

(1) and (2) can be assigned to me, (3) to the extent I'm allowed with my current access privileges to KCI instances.

nuclearcat added a commit to nuclearcat/kernelci-pipeline that referenced this pull request Jan 8, 2024
This is follow up on discussion of PR:
kernelci#377
kernelci/kernelci-core#2258

Now workflow of enabling LAVA lab is simplified:
LAVA lab need to enable one or two tokens (two a bit more secure,
as one will be used to submit job, another for callback),
and provide name(description) of one of tokens to specify
in pipeline.yaml.
Also in toml file we need to specify callback token value.

Signed-off-by: Denys Fedoryshchenko <[email protected]>
nuclearcat added a commit to nuclearcat/kernelci-pipeline that referenced this pull request Jan 8, 2024
This is follow up on discussion of PR:
kernelci#377
kernelci/kernelci-core#2258

Now workflow of enabling LAVA lab is simplified:
LAVA lab need to enable one or two tokens (two a bit more secure,
as one will be used to submit job, another for callback),
and provide name(description) of one of tokens to specify
in pipeline.yaml.
Also in toml file we need to specify callback token value.

Signed-off-by: Denys Fedoryshchenko <[email protected]>
nuclearcat added a commit to nuclearcat/kernelci-pipeline that referenced this pull request Jan 8, 2024
This is follow up on discussion of PR:
kernelci#377
kernelci/kernelci-core#2258

Now workflow of enabling LAVA lab is simplified:
LAVA lab need to enable one or two tokens (two a bit more secure,
as one will be used to submit job, another for callback),
and provide name(description) of one of tokens to specify
in pipeline.yaml.
Also in toml file we need to specify callback token value.

Signed-off-by: Denys Fedoryshchenko <[email protected]>
nuclearcat added a commit to nuclearcat/kernelci-pipeline that referenced this pull request Jan 8, 2024
This is follow up on discussion of PR:
kernelci#377
kernelci/kernelci-core#2258

Now workflow of enabling LAVA lab is simplified:
LAVA lab need to enable one or two tokens (two a bit more secure,
as one will be used to submit job, another for callback),
and provide name(description) of one of tokens to specify
in pipeline.yaml.
Also in toml file we need to specify callback token value.

Signed-off-by: Denys Fedoryshchenko <[email protected]>
nuclearcat added a commit to nuclearcat/kernelci-pipeline that referenced this pull request Jan 9, 2024
This is follow up on discussion of PR:
kernelci#377
kernelci/kernelci-core#2258

Now workflow of enabling LAVA lab is simplified:
LAVA lab need to enable one or two tokens (two a bit more secure,
as one will be used to submit job, another for callback),
and provide name(description) of one of tokens to specify
in pipeline.yaml.
Also in toml file we need to specify callback token value.

Signed-off-by: Denys Fedoryshchenko <[email protected]>
@gctucker
Copy link
Contributor

gctucker commented Jan 9, 2024

OK after a lot of discussion around the issue, it turns out the actual problem is that JWT tokens are now always too long with the new user management system. Previously, they could work with short user names which is why it was set to kci. So this TOML-based approach is fine as a temporary workaround to keep things going with LAVA jobs but it should eventually be removed and a proper solution with persistent API keys should be implemented as per kernelci/kernelci-api#414.

nuclearcat added a commit to nuclearcat/kernelci-pipeline that referenced this pull request Jan 9, 2024
This is follow up on discussion of PR:
kernelci#377
kernelci/kernelci-core#2258

Now workflow of enabling LAVA lab is simplified:
LAVA lab need to enable one or two tokens (two a bit more secure,
as one will be used to submit job, another for callback),
and provide name(description) of one of tokens to specify
in pipeline.yaml.
Also in toml file we need to specify callback token value.

Signed-off-by: Denys Fedoryshchenko <[email protected]>
nuclearcat added a commit to nuclearcat/kernelci-pipeline that referenced this pull request Jan 9, 2024
This is follow up on discussion of PR:
kernelci#377
kernelci/kernelci-core#2258

Now workflow of enabling LAVA lab is simplified:
LAVA lab need to enable one or two tokens (two a bit more secure,
as one will be used to submit job, another for callback),
and provide name(description) of one of tokens to specify
in pipeline.yaml.
Also in toml file we need to specify callback token value.

Signed-off-by: Denys Fedoryshchenko <[email protected]>
@nuclearcat
Copy link
Member Author

Thanks, as follow up: kernelci/kernelci-pipeline#381

@nuclearcat nuclearcat closed this Jan 9, 2024
nuclearcat added a commit to nuclearcat/kernelci-pipeline that referenced this pull request Jan 9, 2024
This is follow up on discussion of PR:
kernelci#377
kernelci/kernelci-core#2258

Now workflow of enabling LAVA lab is simplified:
LAVA lab need to enable one or two tokens (two a bit more secure,
as one will be used to submit job, another for callback),
and provide name(description) of one of tokens to specify
in pipeline.yaml.
Also in toml file we need to specify callback token value.

Signed-off-by: Denys Fedoryshchenko <[email protected]>
nuclearcat added a commit to nuclearcat/kernelci-pipeline that referenced this pull request Jan 10, 2024
This is follow up on discussion of PR:
kernelci#377
kernelci/kernelci-core#2258

Now workflow of enabling LAVA lab is simplified:
LAVA lab need to enable one or two tokens (two a bit more secure,
as one will be used to submit job, another for callback),
and provide name(description) of one of tokens to specify
in pipeline.yaml.
Also in toml file we need to specify callback token value.

Signed-off-by: Denys Fedoryshchenko <[email protected]>
nuclearcat added a commit to nuclearcat/kernelci-pipeline that referenced this pull request Jan 10, 2024
This is follow up on discussion of PR:
kernelci#377
kernelci/kernelci-core#2258

Now workflow of enabling LAVA lab is simplified:
LAVA lab need to enable one or two tokens (two a bit more secure,
as one will be used to submit job, another for callback),
and provide name(description) of one of tokens to specify
in pipeline.yaml.
Also in toml file we need to specify callback token value.

Signed-off-by: Denys Fedoryshchenko <[email protected]>
nuclearcat added a commit to nuclearcat/kernelci-pipeline that referenced this pull request Jan 10, 2024
This is follow up on discussion of PR:
kernelci#377
kernelci/kernelci-core#2258

Now workflow of enabling LAVA lab is simplified:
LAVA lab need to enable one or two tokens (two a bit more secure,
as one will be used to submit job, another for callback),
and provide name(description) of one of tokens to specify
in pipeline.yaml.
Also in toml file we need to specify callback token value.

Signed-off-by: Denys Fedoryshchenko <[email protected]>
nuclearcat added a commit to nuclearcat/kernelci-pipeline that referenced this pull request Jan 10, 2024
This is follow up on discussion of PR:
kernelci#377
kernelci/kernelci-core#2258

Now workflow of enabling LAVA lab is simplified:
LAVA lab need to enable one or two tokens (two a bit more secure,
as one will be used to submit job, another for callback),
and provide name(description) of one of tokens to specify
in pipeline.yaml.
Also in toml file we need to specify callback token value.

Signed-off-by: Denys Fedoryshchenko <[email protected]>
nuclearcat added a commit to nuclearcat/kernelci-pipeline that referenced this pull request Jan 10, 2024
This is follow up on discussion of PR:
kernelci#377
kernelci/kernelci-core#2258

Now workflow of enabling LAVA lab is simplified:
LAVA lab need to enable one or two tokens (two a bit more secure,
as one will be used to submit job, another for callback),
and provide name(description) of one of tokens to specify
in pipeline.yaml.
Also in toml file we need to specify callback token value.

Signed-off-by: Denys Fedoryshchenko <[email protected]>
github-merge-queue bot pushed a commit to kernelci/kernelci-pipeline that referenced this pull request Jan 16, 2024
This is follow up on discussion of PR:
#377
kernelci/kernelci-core#2258

Now workflow of enabling LAVA lab is simplified:
LAVA lab need to enable one or two tokens (two a bit more secure,
as one will be used to submit job, another for callback),
and provide name(description) of one of tokens to specify
in pipeline.yaml.
Also in toml file we need to specify callback token value.

Signed-off-by: Denys Fedoryshchenko <[email protected]>
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.

3 participants