-
Notifications
You must be signed in to change notification settings - Fork 161
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
Create the oauthConfig session secrets #152
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mrogers950 If they are not already assigned, you can assign the PR to them by writing 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 |
@@ -76,7 +76,7 @@ oauthConfig: | |||
sessionConfig: | |||
sessionMaxAgeSeconds: 300 | |||
sessionName: ssn | |||
sessionSecretsFile: "" | |||
sessionSecretsFile: /var/run/secrets/session-secret/secrets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't this break the bootstrap phase?
"github.com/openshift/cluster-kube-apiserver-operator/pkg/operator/v311_00_assets" | ||
"github.com/openshift/library-go/pkg/operator/resource/resourceapply" | ||
"github.com/openshift/library-go/pkg/operator/resource/resourcemerge" | ||
"github.com/openshift/library-go/pkg/operator/resource/resourceread" | ||
v1helpers "github.com/openshift/library-go/pkg/operator/v1helpers" | ||
) | ||
|
||
var ( | ||
Scheme = runtime.NewScheme() | ||
Codecs = serializer.NewCodecFactory(Scheme) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do these have to be public?
@@ -76,7 +76,7 @@ oauthConfig: | |||
sessionConfig: | |||
sessionMaxAgeSeconds: 300 | |||
sessionName: ssn | |||
sessionSecretsFile: "" | |||
sessionSecretsFile: /var/run/secrets/session-secret/secrets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't expect session configuration until after the oeprator is up.
@@ -40,3 +42,6 @@ spec: | |||
- hostPath: | |||
path: /etc/kubernetes/static-pod-resources/kube-apiserver-pod-REVISION | |||
name: resource-dir | |||
- secret: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must be optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must be optional
err, must be removed. You only get static-pod-resources
pkg/operator/crypto/keybits.go
Outdated
@@ -0,0 +1,33 @@ | |||
package crypto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
library-go?
@@ -106,6 +127,33 @@ func manageKubeApiserverConfigMap_v311_00_to_latest(client coreclientv1.ConfigMa | |||
return resourceapply.ApplyConfigMap(client, requiredConfigMap) | |||
} | |||
|
|||
func manageSessionSecret_v311_00_to_latest(client coreclientv1.SecretsGetter) (*corev1.Secret, bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this approach doesn't look right. We know this is likely to move and we know that it needs to rotate. Make a different control loop that manages creation and rotation by itself. Then you simply list the name to be copied down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok. I'll revise this.
Pretty sure that this results in a pod that brings down the kube-apiserver and never comes back up because the static pod cannot mount a secret. |
@mrogers950: PR needs rebase. 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. |
305f042
to
c2030d6
Compare
@deads2k @sttts I've updated with what should be closer to the right approach. There's a controller that creates the initial session secret and regenerates it on delete/update. Then I added a config observer that checks to see if the secret is there and updates sessionSecretsFile with the static pod resource path. |
/retest |
1 similar comment
/retest |
return err | ||
} | ||
|
||
func (sc *SessionSecretController) updateSessionSecret() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deads2k wanted this to handle rotation, meaning you need the old data as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically, what is the rotation behavior that we want?
return | ||
} | ||
|
||
// Generate the session secret if it doesn't exist initially. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just looks weird to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what way?
"k8s.io/client-go/tools/cache" | ||
"k8s.io/client-go/util/workqueue" | ||
|
||
legacyv1 "github.com/openshift/api/legacyconfig/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, do not import this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without that I'm unclear on how to handle the encoding for SessionSecret
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deads2k osin still expects it to be in the legacy format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deads2k osin still expects it to be in the legacy format.
serialize by hand if need be. Do not rely on this package. You should fix it to be bilingual
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should fix it to be bilingual
If you do this, then you won't have a problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what you mean by bilingual here..
c2030d6
to
677144e
Compare
This update addresses nits and other comments and handles the initial creation of session-secret by using a watch on the openshift-kube-apiserver-* pods, rather than doing it all in the Run method. Like @enj has mentioned we want to be able to cycle the session secret, but as to which event to trigger this on is not clear. Should a delete of session-secret result in a recreation containing the original SessionSecret with an addition of a new one to the SessionSecrets slice? How about on update with a check for some annotation to tell it to rotate? Also, manual serialization of SessionSecrets instead of relying on the current legacy encoder might be easier than it sounds to me, is there a good example of how to do this properly? |
677144e
to
ec158e6
Compare
I've pushed an update that removes the dependency on the legacy package. I was able to confirm that the manual encoding I'm doing now matches the legacy encoder. (turned out to be doing JSON not YAML) |
/retest |
cache.FilteringResourceEventHandler{ | ||
FilterFunc: isKubeAPIServerPod, | ||
Handler: cache.ResourceEventHandlerFuncs{ | ||
AddFunc: sc.enqueueSecret, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sets off my spidey sense. We need to be level driven and this is watching for an edge. Why would we do this once ever?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There needs to be something that triggers the creation of the secret initially, so watching for the pod that will be the consumer of it seems to be the most logical to me. If there is a better solution please suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There needs to be something that triggers the creation of the secret initially, so watching for the pod that will be the consumer of it seems to be the most logical to me. If there is a better solution please suggest.
? You're trying to create a thing when the namespace is present, right? Why not watch for the namespace?
|
||
_, err = listers.SecretLister.Secrets(sessionSecretNamespace).Get(sessionSecretName) | ||
if errors.IsNotFound(err) { | ||
glog.Warningf("session secret %s/%s not found", sessionSecretNamespace, sessionSecretName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the event recorder if it is this important
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't really seem event-worthy to me. there will be some time before the secret is created so even warning it might be chatty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't really seem event-worthy to me. there will be some time before the secret is created so even warning it might be chatty.
You're debugging a cluster where this secret isn't present. Would you like this information? Then it's not chatty.
pkg/operator/configobservation/authconfig/observe_sessionsecret.go
Outdated
Show resolved
Hide resolved
pkg/operator/configobservation/authconfig/observe_sessionsecret.go
Outdated
Show resolved
Hide resolved
pkg/operator/configobservation/configobservercontroller/observe_config_controller.go
Show resolved
Hide resolved
ec158e6
to
7c6439c
Compare
/retest |
}, | ||
} | ||
_, err = sc.secretClient.Secrets(secret.Namespace).Create(secret) | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore already exists
if err != nil { | ||
errs = append(errs, err) | ||
} | ||
if currentSessionSecretsFilePath == sessionSecretPath { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no
observedConfig := map[string]interface{}{} | ||
oauthConfigSessionSecretsFilePath := []string{"oauthConfig", "sessionConfig", "sessionSecretsFile"} | ||
|
||
currentSessionSecretsFilePath, _, err := unstructured.NestedString(existingConfig, oauthConfigSessionSecretsFilePath...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build the prevObservedConfig
and use it like all the others
if err != nil { | ||
errs = append(errs, err) | ||
} | ||
if currentSessionSecretsFilePath == sessionSecretPath { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
secret is deleted after it is created, this block prevents the value from ever being cleared.
7c6439c
to
d01b3bf
Compare
d01b3bf
to
5173411
Compare
I've revised the controller and config observer to handle a removal of the field if the secret is deleted. Although the config appears to update correctly, now I'm seeing one of the kube-apiserver pods staying stuck in a crashloop (the mounted session-secret file is not available) and I can't figure out why that is. It appears to never catch up with the latest config. Here's what the log looks like around at time the secret is created the first time and the config gets updated:
|
/test all |
@mrogers950: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
@mrogers950: PR needs rebase. 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. |
We'll be handling the session secrets file in the auth operator at this point. |
This PR starts filling in the default oauthConfig, by generating the session secrets to include in the openshift-kube-apiserver pod. Getting the oauthConfig filled in here to start a working oauth server is part of openshift/origin#21580
/cc @openshift/sig-auth @smarterclayton @derekwaynecarr