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.

Sign up for GitHub

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

Jump to bottom

Move broadcast of key updates into sync, fixup of externaljwt generation / test #128670

Merged
merged 3 commits into from
Nov 7, 2024
Merged

Move broadcast of key updates into sync, fixup of externaljwt generation / test #128670

merged 3 commits into from
Nov 7, 2024
+328 −48

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Nov 7, 2024 •
edited
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:

  1. Bugfix: Moves broadcast into syncKeys method, so any change to keys (periodic or on-demand) always notifies listeners immediately.
  2. Test flake fix: Address race between setting new keys and ensuring they've been read (Add plugin and key-cache for ExternalJWTSigner integration #128190 (comment))
  3. Add proto definition file needed for local generation (fixes Issue with protobindings codegen #128672)

Does this PR introduce a user-facing change?

NONE

/assign @enj
/sig auth

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/auth Categorizes an issue or PR as relevant to SIG Auth. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 7, 2024
@k8s-ci-robot k8s-ci-robot added area/test approved Indicates a PR has been approved by an approver from all required OWNERS files. sig/testing Categorizes an issue or PR as relevant to SIG Testing. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 7, 2024
@liggitt liggitt mentioned this pull request Nov 7, 2024
Merged
enj
enj reviewed Nov 7, 2024
View reviewed changes
Copy link
Member

@enj enj left a comment

Choose a reason for hiding this comment

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

Minor comments.

pkg/serviceaccount/externaljwt/plugin/keycache.go Outdated Show resolved Hide resolved
pkg/serviceaccount/externaljwt/plugin/keycache.go Show resolved Hide resolved
@liggitt liggitt force-pushed the externaljwt-broadcast branch from 81c1892 to 070f74b Compare November 7, 2024 19:17
@liggitt liggitt changed the title Move broadcast of key updates into sync Move broadcast of key updates into sync, fixup of externaljwt generation / test Nov 7, 2024
@liggitt liggitt mentioned this pull request Nov 7, 2024
Closed
@liggitt
Copy link
Member Author

liggitt commented Nov 7, 2024

addressed comments, also folded in distinct follow-up commits to deflake an integration flake @benluddy observed and make proto generation work locally

@enj
Copy link
Member

enj commented Nov 7, 2024

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 332ba8c4ff8717de761d02f6a4ee419274632020

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@liggitt liggitt added this to the v1.32 milestone Nov 7, 2024
@liggitt liggitt added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Nov 7, 2024
@k8s-ci-robot k8s-ci-robot removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 7, 2024
@k8s-ci-robot k8s-ci-robot merged commit b711380 into kubernetes:master Nov 7, 2024
15 checks passed
@liggitt liggitt deleted the externaljwt-broadcast branch December 13, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HarshalNeelkamal HarshalNeelkamal HarshalNeelkamal left review comments

@enj enj enj left review comments

@deads2k deads2k Awaiting requested review from deads2k

@SataQiu SataQiu Awaiting requested review from SataQiu

Assignees

@enj enj

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Auth
Archived in project
[sig-release] Bug Triage
Archived in project
Milestone
v1.32
Development

Successfully merging this pull request may close these issues.

Issue with protobindings codegen
4 participants
@liggitt @enj @k8s-ci-robot @HarshalNeelkamal