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

add update-go-deps and perform initial update to k8s v0.23.1 #160

Merged
merged 1 commit into from
Mar 30, 2022

Conversation

ibihim
Copy link
Collaborator

@ibihim ibihim commented Feb 3, 2022

Signed-off-by: Krzysztof Ostrowski kostrows@redhat.com

@ibihim ibihim changed the title [wip] fix webhook retry backoff [wip] add update-go-deps and perform initial update Feb 7, 2022
@ibihim ibihim changed the title [wip] add update-go-deps and perform initial update [wip] add update-go-deps and perform initial update to k8s v0.23.1 Feb 7, 2022
@s-urbaniak
Copy link
Collaborator

the build still fails, it seems that the setup-go action has different go versions installed on the container, we have to pick the correct one, see actions/setup-go#107.

@ibihim ibihim force-pushed the make-deps branch 2 times, most recently from 1a1a9d1 to 9b3b482 Compare February 8, 2022 15:04
@ibihim
Copy link
Collaborator Author

ibihim commented Feb 8, 2022

the build still fails, it seems that the setup-go action has different go versions installed on the container, we have to pick the correct one, see actions/setup-go#107.

I think that the design of the github action could be better (no default to go1.15), but it is actually our fault. We didn't specify the go version for the make test-e2e step and so we went with the default of go-1.15

tokenAuthenticator, err := oidc.New(oidc.Options{
IssuerURL: config.IssuerURL,
ClientID: config.ClientID,
CAFile: config.CAFile,
CAContentProvider: cacp,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

using this will give us rotation of CA content for free.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@s-urbaniak, PTAL at the latest commit.

@ibihim ibihim force-pushed the make-deps branch 2 times, most recently from 8edc746 to 3253322 Compare February 9, 2022 12:10
pkg/authn/oidc.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@s-urbaniak s-urbaniak left a comment

Choose a reason for hiding this comment

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

just one comment regarding inlining

@ibihim ibihim changed the title [wip] add update-go-deps and perform initial update to k8s v0.23.1 add update-go-deps and perform initial update to k8s v0.23.1 Feb 10, 2022
if err != nil {
return nil, err
}
dyCA.Run(1, ctx.Done())
Copy link
Collaborator

@s-urbaniak s-urbaniak Feb 10, 2022

Choose a reason for hiding this comment

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

on a second thought (sorry, my bad, I didn't catch this earlier): A constructor shouldn't have these strong side-effects. Let's call the Run methods outside of the constructor, this also aligns it with the delegating authenticator:

diff --git a/main.go b/main.go
index fba74831..d26eb243 100644
--- a/main.go
+++ b/main.go
@@ -174,10 +174,12 @@ func main() {
 		ctx, cancel := context.WithCancel(context.Background())
 		defer cancel()
 
-		authenticator, err = authn.NewOIDCAuthenticator(ctx, cfg.auth.Authentication.OIDC)
+		oidcAuthenticator, err := authn.NewOIDCAuthenticator(cfg.auth.Authentication.OIDC)
 		if err != nil {
 			klog.Fatalf("Failed to instantiate OIDC authenticator: %v", err)
 		}
+		go oidcAuthenticator.Run(1, context.Background().Done())
+		authenticator = oidcAuthenticator
 	} else {
 		//Use Delegating authenticator
 		klog.Infof("Valid token audiences: %s", strings.Join(cfg.auth.Authentication.Token.Audiences, ", "))
diff --git a/pkg/authn/oidc.go b/pkg/authn/oidc.go
index 702f4ae5..75189e6c 100644
--- a/pkg/authn/oidc.go
+++ b/pkg/authn/oidc.go
@@ -17,7 +17,7 @@ limitations under the License.
 package authn
 
 import (
-	"context"
+	"net/http"
 
 	"k8s.io/apiserver/pkg/authentication/authenticator"
 	"k8s.io/apiserver/pkg/authentication/request/bearertoken"
@@ -37,13 +37,29 @@ type OIDCConfig struct {
 	SupportedSigningAlgs []string
 }
 
+type OIDCAuthenticator struct {
+	dynamicCA     *dynamiccertificates.DynamicFileCAContent
+	authenticator *bearertoken.Authenticator
+}
+
+func (o *OIDCAuthenticator) RunOnce() error {
+	return o.dynamicCA.RunOnce()
+}
+
+func (o *OIDCAuthenticator) Run(workers int, stopCh <-chan struct{}) {
+	o.dynamicCA.Run(workers, stopCh)
+}
+
+func (o *OIDCAuthenticator) AuthenticateRequest(req *http.Request) (*authenticator.Response, bool, error) {
+	return o.authenticator.AuthenticateRequest(req)
+}
+
 // NewOIDCAuthenticator returns OIDC authenticator
-func NewOIDCAuthenticator(ctx context.Context, config *OIDCConfig) (authenticator.Request, error) {
+func NewOIDCAuthenticator(config *OIDCConfig) (*OIDCAuthenticator, error) {
 	dyCA, err := dynamiccertificates.NewDynamicCAContentFromFile("kube-rbac-proxy", config.CAFile)
 	if err != nil {
 		return nil, err
 	}
-	dyCA.Run(1, ctx.Done())
 
 	tokenAuthenticator, err := oidc.New(oidc.Options{
 		IssuerURL:            config.IssuerURL,
@@ -59,5 +75,8 @@ func NewOIDCAuthenticator(ctx context.Context, config *OIDCConfig) (authenticato
 		return nil, err
 	}
 
-	return bearertoken.New(tokenAuthenticator), nil
+	return &OIDCAuthenticator{
+		authenticator: bearertoken.New(tokenAuthenticator),
+		dynamicCA:     dyCA,
+	}, nil
 }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is better to handle the control of go routines as far up as we can. Ideally the person who owns the object that runs it. So I completely agree with you on that topic and I am glad you mentioned it. I was just too much in "lets ship it finally"-mode.


kind-create-cluster: kind-delete-cluster
kind create cluster --name $(BIN) --config ./test/e2e/kind-config/kind-config.yaml
kind load docker-image $(CONTAINER_NAME)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice 👍

Copy link
Collaborator

@s-urbaniak s-urbaniak left a comment

Choose a reason for hiding this comment

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

lgtm 🎉

@s-urbaniak
Copy link
Collaborator

@ibihim please squash into meaningful commits, once merged, we can release a new version 👍

The update to kubernetes v0.23.5 had some breaking changes. While doing
the update, the dynamic CA loading is also leveraged.

Signed-off-by: Krzysztof Ostrowski <kostrows@redhat.com>
@s-urbaniak
Copy link
Collaborator

all green, merging

@s-urbaniak s-urbaniak merged commit 9f21af2 into brancz:master Mar 30, 2022
@s-urbaniak
Copy link
Collaborator

thank you @ibihim ! 🎉 now, let's do a release :-)

@ibihim ibihim deleted the make-deps branch June 29, 2022 13:38
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.

None yet

3 participants