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

fix: timed read kubernetes service discovery token #12057

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Baoyuantop
Copy link
Contributor

Description

Get the token from token_path at regular intervals and update the configuration if the value is changed to avoid token expiration errors.

Fixes #11779

Checklist

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

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. enhancement New feature or request labels Mar 17, 2025
Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

  1. it is not a new feature, it should be a bug, pls update your title
  2. we should use cache, eg: 1hour or 1day, and then we can refresh it, this is more acceptable

local function start_fetch(handle)
local timer_runner
timer_runner = function(premature)
if premature then
return
end

if handle.token_file then
refresh_token(handle, handle.token_file)
Copy link
Member

Choose a reason for hiding this comment

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

bad name

cache -> refresh: this is a good name
no cache -> refresh: it is weird

it should be name to read token or fetch token

@membphis membphis changed the title feat: add kubernetes service discovery token refresh bugfix: add kubernetes service discovery token refresh Mar 19, 2025
@Baoyuantop Baoyuantop changed the title bugfix: add kubernetes service discovery token refresh fix: timed read kubernetes service discovery token Mar 19, 2025
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Mar 19, 2025
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Mar 20, 2025
end

-- remove possible extra whitespace
local trimmed_token = token:gsub("%s+", "")
Copy link
Member

Choose a reason for hiding this comment

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

waiting


local token, err = read_token(token_file_path)
if err then
return nil, err
Copy link
Member

Choose a reason for hiding this comment

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

we only can choose one of them:

if not attributes then
        core.log.error("failed to fetch ", token_file_path, " attributes: ", err)
        return
    end
if err then
        return nil, err
end


handle.apiserver.token = token
handle.token_file_mtime = last_modification_time
core.log.warn("kubernetes service account token has been updated")
Copy link
Member

Choose a reason for hiding this comment

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

use log.infolog.notice is suitable


local function update_token(handle)
if not handle.apiserver.token_file or handle.apiserver.token_file == "" then
return
Copy link
Member

Choose a reason for hiding this comment

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

for succ or fail, we should return different value

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: apisix with kubernetes discovery will fail after token file expires
3 participants