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

KEP-3331: Structured Authentication Config #3332

Merged
merged 28 commits into from
Jun 13, 2023

Conversation

nabokihms
Copy link
Member

  • One-line PR description: Structured config for OIDC authentication
  • Other comments: -

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 2, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @nabokihms. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 2, 2022
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Jun 2, 2022
@nabokihms nabokihms mentioned this pull request Jun 2, 2022
41 tasks
@k8s-ci-robot k8s-ci-robot assigned mikedanese and unassigned mikedanese Jun 6, 2022
@enj
Copy link
Member

enj commented Jun 6, 2022

/assign

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
@enj
Copy link
Member

enj commented Sep 9, 2022

@nabokihms are you targeting this for v1.26?

@nabokihms
Copy link
Member Author

@enj Yeah, I'd like to take it to v1.26 if it is possible.

@liggitt
Copy link
Member

liggitt commented Sep 28, 2022

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 28, 2022
Copy link
Member

@liggitt liggitt left a comment

Choose a reason for hiding this comment

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

did a first pass on the proposed config structure

@liggitt liggitt self-assigned this Sep 28, 2022
Copy link
Member

@andrewsykim andrewsykim left a comment

Choose a reason for hiding this comment

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

(shadowing @deads2k on PRR)


###### How can a rollout or rollback fail? Can it impact already running workloads?

It cannot fail.
Copy link
Member

Choose a reason for hiding this comment

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

Some possible rollout/rollback failures I can imagine are:

  • General bugs in kube-apiserver's parsing of the structured config file. Maybe more relevant in future versions where there could be bugs in the conversion logic between multiple versions (v1alpha1, v1beta1, etc) of the config.
  • Cluster admin rolls out the feature with the addition of some validation rules that may have some untended consequences
  • Rolling back would mean losing validation functionality offered from structured config that some other component may depend on

(not suggesting much can be done about all of these, especially if they are due to user error, but probably worth noting here)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. And noting that failures are intended to prevent process startup means that HA clusters that ripple-start will survive intact.

aramase and others added 5 commits May 30, 2023 04:54
Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
address API review comments for extra mappings
Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 6, 2023
@enj
Copy link
Member

enj commented Jun 6, 2023

This is ready from an API perspective (have addressed all of Jordan's API review feedback).

/approve

Just waiting on PRR now.

/assign @deads2k

- Feature implemented behind a feature flag
- Unit tests to validate CEL semantics
- Unit tests for config validation
- Initial e2e tests completed and enabled
Copy link
Member

Choose a reason for hiding this comment

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

Any reason integration tests aren't included here? Is it because we need to test with a OIDC provider?

Copy link
Member

Choose a reason for hiding this comment

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

This is a typo and should say integration tests, will fix.

[supported limits]: https://git.k8s.io/community//sig-scalability/configs-and-limits/thresholds.md
-->

TBA.
Copy link
Member

Choose a reason for hiding this comment

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

I would just say No here, reading and serializing the config on start-up is negligible, unless we plan to watch for file changes and allow for dynamic changes to the authentication config

Copy link
Member

Choose a reason for hiding this comment

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

We do plan to watch for config changes and dynamically update the authenticators (and likely throw away the token cache). This also involves re-parsing the CEL functions (and we have new overhead of CEL functions per authentication as well).

@deads2k
Copy link
Contributor

deads2k commented Jun 13, 2023

PRR is complete enough for alpha. Please consider what metrics you'll add for beta when this gets enabled by default.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, enj, nabokihms

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 13, 2023
@enj
Copy link
Member

enj commented Jun 13, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2023
@k8s-ci-robot k8s-ci-robot merged commit 696e620 into kubernetes:master Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
Status: API review completed, 1.28
Archived in project
Development

Successfully merging this pull request may close these issues.