-
Notifications
You must be signed in to change notification settings - Fork 24
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
check cert and token for import config #96
Conversation
1105b5c
to
d0158fd
Compare
Signed-off-by: zhujian <jiazhu@redhat.com>
d0158fd
to
62eb4c9
Compare
/assign @skeeey |
@@ -224,6 +224,10 @@ func getValidCertificatesFromURL(serverURL string, rootCAs *x509.CertPool) ([]*x | |||
// create kubeconfig from bootstrap secret | |||
func createKubeconfigData(client client.Client, bootStrapSecret *corev1.Secret) ([]byte, error) { | |||
saToken := bootStrapSecret.Data["token"] | |||
if len(saToken) == 0 { | |||
log.Error(nil, "No token value found in the boot strap secret", "secret name", bootStrapSecret.Name) |
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.
we may not need log this, this err will be requeued, so we can find it in the controller-runtime
|
||
if !knownCAs { | ||
// fallback to service account token ca.crt | ||
if v := bootStrapSecret.Data["ca.crt"]; len(v) > 0 { |
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.
It seems the ca.crt is not required for corev1.SecretTypeServiceAccountToken
,
SecretTypeServiceAccountToken contains a token that identifies a service account to the API
Required fields: - Secret.Annotations["kubernetes.io/service-account.name"] - the name of the ServiceAccount the token identifies - Secret.Annotations["kubernetes.io/service-account.uid"] - the UID of the ServiceAccount the token identifies - Secret.Data["token"] - a token that identifies the service account to the API
so do we need this? I think we may keep original logical in current phase
@zhiweiyin318 what's your opinion? and is the ca.crt
missed in the QE env? if it is not missed, I think we don't need change 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.
ca and token are both missed on the managed cluster ,but they exist in the import-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.
I checked the OCP on AWS, commonly there is no cert in apiservers.config.openshift.io/cluster. so the ca is fetched from the bootstrap-sa-token secret in bootstrap-sa.
the token and ca are both fetched from the bootstrap-sa-token secret. they will be empty in the import secret in the case the secret is not created before import.
we can only use token in the kubeconfig, but need add insecure-skip-tls-verify: true
in it.
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.
// AddFlags adds flags related to ServiceAccountController for controller manager to the specified FlagSet
...
fs.StringVar(&o.RootCAFile, "root-ca-file", o.RootCAFile, "If set, this root certificate authority will be included in service account's token secret. This must be a valid PEM-encoded CA bundle.")
if root-ca-file
is set, the ca.crt will be in the sa token secret.
in OCP, root-ca-file
is set.
$ oc get pods -n openshift-kube-controller-manager kube-controller-manager-ip-10-0-137-110.ec2.internal -o yaml | grep root-ca-file
--root-ca-file=/etc/kubernetes/static-pod-resources/configmaps/serviceaccount-ca/ca-bundle.crt
Signed-off-by: zhujian <jiazhu@redhat.com>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zhujian7 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Kudos, SonarCloud Quality Gate passed! |
if v := bootStrapSecret.Data["ca.crt"]; len(v) > 0 { | ||
certData = v | ||
} else { | ||
log.Info("No ca.crt found in the boot strap secret", "secret name", bootStrapSecret.Name) |
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.
if we allow empty ca, we should add insecure-skip-tls-verify: true
in bootstrap-kubeconfig. And need to check if we can registry a cluster using a kubeconfig without ca.
I suggest to return error here since ca is set in sa token secret in OCP.
Ref to: https://github.com/open-cluster-management/backlog/issues/16871
Signed-off-by: zhujian jiazhu@redhat.com