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

change(jwt-auth): unify apisix/core/vault.lua and apisix/secret/vault.lua #8660

Merged
merged 15 commits into from
Jan 30, 2023

Conversation

shreemaan-abhishek
Copy link
Contributor

@shreemaan-abhishek shreemaan-abhishek commented Jan 12, 2023

Description

Fixes #8424

Checklist

  • I have explained the need for this PR and the problem it solves. Check the issue
  • I have explained the changes or the new features added to this PR NA
  • I have added tests corresponding to this change. Not yet
  • I have updated the documentation to reflect this change. NA
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

local new_tab = require ("table.new")
local secrets = require("apisix.admin.secrets")
Copy link
Contributor

Choose a reason for hiding this comment

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

All functions in apisix.admin.secrets are only used to configure secret manager. It should not be called from plugins.

If you want to use the secret manager function, you should use the get function in apisix.secret

@@ -252,15 +253,12 @@ end
local function get_secret(conf, consumer_name)
local secret = conf.secret
if conf.vault then
Copy link
Contributor

Choose a reason for hiding this comment

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

Just delete the vault-related configuration directly, because we have already retrieved and replaced the configuration of all auth plugins (type = "auth") in the consumer.
https://github.com/apache/apisix/blob/master/apisix/consumer.lua#L107

@shreemaan-abhishek shreemaan-abhishek changed the title feat: use get method from kms module unify apisix/core/vault.lua and apisix/secret/vault.lua Jan 12, 2023
@shreemaan-abhishek shreemaan-abhishek changed the title unify apisix/core/vault.lua and apisix/secret/vault.lua fix: unify apisix/core/vault.lua and apisix/secret/vault.lua Jan 12, 2023
@shreemaan-abhishek shreemaan-abhishek marked this pull request as ready for review January 12, 2023 05:06
@soulbird
Copy link
Contributor

soulbird commented Jan 12, 2023

After deleting a configuration, a break change is introduced in APISIX, so the title of the PR should be change: xxx, here we change the jwt-auth plugin, so it is best to change(jwt-auth): xxx

Other legitimate headers can be seen here: https://github.com/apache/apisix/blob/master/.github/workflows/semantic.yml#L23

@shreemaan-abhishek shreemaan-abhishek changed the title fix: unify apisix/core/vault.lua and apisix/secret/vault.lua change(jwt-auth): unify apisix/core/vault.lua and apisix/secret/vault.lua Jan 12, 2023
@soulbird
Copy link
Contributor

Should also update doc: https://github.com/apache/apisix/blob/master/docs/en/latest/plugins/jwt-auth.md

If you are interested, you can update the Chinese documentation at the same time, you can use the tool to translate.
https://github.com/apache/apisix/blob/master/docs/zh/latest/plugins/jwt-auth.md

@shreemaan-abhishek
Copy link
Contributor Author

How should this be handled?

image

@soulbird
Copy link
Contributor

A link should be added after the field, pointing to APISIX Secret

@soulbird
Copy link
Contributor

How should this be handled?

image

As described in the comments, this code fits into three possible scenarios. So, just remove some of the comments

@soulbird
Copy link
Contributor

Vault related links in this chapter should be updated https://github.com/apache/apisix/blob/master/docs/en/latest/getting-started.md#features
https://github.com/apache/apisix/blob/master/docs/en/latest/getting-started.md#features

soulbird
soulbird previously approved these changes Jan 18, 2023
if public_key and private_key then
return public_key, private_key
elseif public_key and not private_key then
return public_key, nil, "missing private key"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@shreemaan-abhishek shreemaan-abhishek Jan 18, 2023

Choose a reason for hiding this comment

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

should we add a nil check there? I think we should. Even the previous implementation would return nil in some cases. Not sure why it is not being checked.

Copy link
Member

Choose a reason for hiding this comment

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

if not public_key then

Here we check if public_key is nil to detect the err.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not quite sure what you are trying to say, could you please elaborate? 😅

Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

Can we drop

# HashiCorp Vault storage backend for sensitive data retrieval. The config shows an example of what APISIX expects if you
now?

soulbird
soulbird previously approved these changes Jan 29, 2023
elseif not public_key and private_key then
return nil, nil, "missing public key"
else
return nil, nil, "public and private keys are missing"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't remove the elseif statements so that a better error message could be returned. Let me know if this is not up to the mark.

@spacewander spacewander merged commit c8d5afb into apache:master Jan 30, 2023
hongbinhsu added a commit to fitphp/apix that referenced this pull request Jan 30, 2023
* upstream/master: (67 commits)
  fix: grpc-transcode plugin: fix map data population (apache#8731)
  change(jwt-auth): unify apisix/core/vault.lua and apisix/secret/vault.lua (apache#8660)
  feat: stream subsystem support consul_kv service discovery (apache#8633)
  fix(proxy-mirror): use with uri rewrite (apache#8718)
  ci: move helper script to the right dir (apache#8691)
  refactor(pubsub): simpify the get_cmd implementation (apache#8608)
  feat: stream subsystem support kubernetes service discovery (apache#8640)
  docs: fix deployment links (apache#8714)
  fix: remove backslash before slash when encoding (apache#8684)
  ci: kafka should register port in the zookeeper same as exposed (apache#8672)
  docs: fix plugin config naming (apache#8701)
  docs: fix code block (apache#8700)
  docs: rename kms to secret (apache#8697)
  docs: replace transparent logos with white background logos (apache#8689)
  fix: upgrade lua-resty-etcd to 1.10.3 (apache#8668)
  fix: upgrade casbin to 1.41.3 to improve performance (apache#8676)
  chore: make send_stream_request more clear (apache#8627)
  feat: stream subsystem support nacos service discovery (apache#8584)
  feat: stream subsystem support dns service discovery (apache#8593)
  refactor(admin): refactor resource routes (apache#8611)
  ...
@shreemaan-abhishek shreemaan-abhishek deleted the fix#8424 branch February 23, 2023 15:44
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.

Unified apisix/core/vault.lua and apisix/secret/vault.lua
3 participants