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

🐛 Fix frontproxy #3023

Merged
merged 1 commit into from
Oct 17, 2023
Merged

🐛 Fix frontproxy #3023

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/proxy/options/authentication.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ func (c *Authentication) ApplyTo(authenticationInfo *genericapiserver.Authentica
versionedInformers.Core().V1().Secrets().Lister(),
versionedInformers.Core().V1().ServiceAccounts().Lister(),
)
authenticatorConfig.SecretsWriter = tokenGetterClient.CoreV1().Secrets()
Copy link
Member

Choose a reason for hiding this comment

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

what does a writer here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used in LegacyServiceAccount validator. It just fails to start if this is not set and APIServer fails .
I think its for SA management of secrets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"ts":1697038810878.5505,"caller":"dynamiccertificates/dynamic_serving_content.go:113","msg":"Loaded a new cert/key pair","v":2,"name":"serving-cert::/etc/kcp-front-proxy/tls/tls.crt::/etc/kcp-front-proxy/tls/tls.key"}
{"ts":1697038811067.4224,"caller":"dynamiccertificates/dynamic_cafile_content.go:119","msg":"Loaded a new CA Bundle and Verifier","v":2,"name":"client-ca-bundle::/etc/kcp-front-proxy/client-ca/tls.crt"}
{"ts":1697038811067.9343,"caller":"transport/cert_rotation.go:137","msg":"Starting client certificate rotation controller\n","v":3}
{"ts":1697038811068.258,"caller":"cli/run.go:74","msg":"command failed","err":"while creating legacy validator, err: SecretsWriter must be provided"}

API server fails to start with this. We missed this in rebase

Copy link
Member

Choose a reason for hiding this comment

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

k

Copy link
Member

Choose a reason for hiding this comment

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

Though cert rotation also worries me. Why in the frontproxy? Or in general: why is this part of the authentication plugin? Seems odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might be how the front proxy was worried together. At this point it's not even starting. But you say authentication plugin should not even be there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is the case, this means we have wrong api-server flavor config in frontproxy case.
Maybe worth merging this for now as it blocks usage of main branch all together with front proxy and looking how to get rid of it as followup?

Copy link
Member

Choose a reason for hiding this comment

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

The auth plugin is expected to be there. We use it in the proxy. I just wonder what else ppl have put into the auth plugins and whether we want to inherit it.

Copy link
Member

Choose a reason for hiding this comment

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

It's used to set kubernetes.io/legacy-token-last-used on the service account secret. It's properly cluster-scoped, and only active when we enable service account lookup (which we do it seems, and that's probably not a good idea from the front-proxy).

}

// Sets up a union Authenticator for all enabled auth methods
Expand Down