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

need to document the auth.openshift settings #6176

Closed
jmazzitelli opened this issue May 23, 2023 · 11 comments
Closed

need to document the auth.openshift settings #6176

jmazzitelli opened this issue May 23, 2023 · 11 comments
Labels
bug Something isn't working docs It requires changes on kiali.io

Comments

@jmazzitelli
Copy link
Collaborator

There are some auth.openshift settings that were added last year (see PR 5021) that never got documented. If someone puts them in their Kiali CR and tries to validate their CR with our validation tool, it will fail. Also, users have no documentation for these values.

So we need to document these.

https://github.com/kiali/kiali/blob/v1.67.0/config/config.go#L333-L338 :

type OpenShiftConfig struct {
	AuthTimeout    int    `yaml:"auth_timeout,omitempty"`
	ClientIdPrefix string `yaml:"client_id_prefix,omitempty"`
	ClientId       string `yaml:"client_id,omitempty"`
	ServerPrefix   string `yaml:"server_prefix,omitempty"`
	UseSystemCA    bool   `yaml:"use_system_ca,omitempty"`
	CustomCA       string `yaml:"custom_ca,omitempty"`
}

https://github.com/kiali/kiali-operator/blob/v1.67.0/crd-docs/cr/kiali.io_v1alpha1_kiali.yaml#L52-L56 :

    openshift:
      auth_timeout: 10
      client_id_prefix: "kiali"
      #token_inactivity_timeout:
      #token_max_age:

Those four are documented in the CRD here but 4 from config.go are not (ClientId, ServerPrefix, UseSystemCA, CustomCA)

We need to document these missing ones in the CRD and CR example. Also, these docs are incorrect: https://kiali.io/docs/configuration/authentication/openshift/ (notice it says, The openshift strategy doesn’t have any additional configuration.)

(I noticed all of this when trying to figure out if openshift auth could be configured to workaround this bug).

@jmazzitelli jmazzitelli added bug Something isn't working docs It requires changes on kiali.io labels May 23, 2023
@jmazzitelli
Copy link
Collaborator Author

jmazzitelli commented May 23, 2023

@israel-hdez @libesz could I ask if you two could comment here and give a little burb on these four settings that are missing in the docs?

Here's the PR you can add your review comments to: https://github.com/kiali/kiali-operator/pull/648/files

I'll update the Kiali docs where it needs to be (via the PR and kiali.io docs additions), but I would just like to hear it from you what these do since you both were involved in adding these new features last year (see #5021), but we forgot to document them and put them in our CRD schema so the validation tool will work with them.

Here's what I think they do (these are in the PR):

  • ClientID: (string, default is unset) If this defined, client_id_prefix is ignored. This is the string used by OAuth to identify the client.
  • ServerPrefix: (string, default is https://kubernetes.default.svc/) Not really a server prefix, more like a URL-prefix because it contains the protocol and full hostname. This is the https://hostname portion of the URL used to communicate with OpenShift during OAuth handshake.
  • UseSystemCA: (boolean, default is false) When false, the serviceaccount CA is used during OpenShift OAuth. When true, the system CA is used. This is ignored if CustomCA is specified.
  • CustomCA: (string, default is unset) When set, this is the base64-encoded CA cert used for OpenShift OAuth.

@israel-hdez
Copy link
Member

Those four are documented in the CRD here but 4 from config.go are not (ClientId, ServerPrefix, UseSystemCA, CustomCA)

We need to document these missing ones in the CRD and CR example. Also, these docs are incorrect: https://kiali.io/docs/configuration/authentication/openshift/ (notice it says, The openshift strategy doesn’t have any additional configuration.)

(I noticed all of this when trying to figure out if openshift auth could be configured to workaround this bug).

IIRC, those were left undocumented because it was a PR of a larger effort to add support to observe a remote cluster; i.e. the intent was to run Kiali outside of the observed cluster while using OpenShift authentication. The PR made OpenShift auth usable on this configuration, but once in the console you would see lots of errors. This effort didn't continue.

  • About ClientId. Normally, Kiali's OAuth ClientID is ClientIdPrefix + '-' + KialiNamespace. When running outside the cluster, Kiali does not have a namespace. So, ClientID sets a hard ID.
  • About ServerPrefix. Most likely, it was named this way, because the old const in the code was named like that. It was moved to a configuration to be able to set the clusterAPI endpoint.
  • About UseSystemCA and CustomCA. Since the cluster API is behind TLS and is usually self-signed, you need a way to trust the certificate and connect to the cluster API securely. Normally, Kiali uses the certificate provided together with its ServiceAccount. These two configs moved this to a 3-way config:
    • If CustomCA is set, this takes priority and is used as the CA to validate the secure connection.
    • If CustomCAis not set and UseSystemCAis false, the original behavior is used; i.e. Kiali uses the certificate provided together with its ServiceAccount.
    • If CustomCAis not set and UseSystemCAis true, Kiali uses the system certificate store.

IMHO, I think it is still OK to left those settings as undocumented because Kiali is most-likely still not stable under such configuration (lots of errors in the UI after login), unless some recent work (uhm, multi-cluster?) has made it stable.

@nrfox
Copy link
Contributor

nrfox commented May 23, 2023

IMHO, I think it is still OK to left those settings as undocumented because Kiali is most-likely still not stable under such configuration (lots of errors in the UI after login), unless some recent work (uhm, multi-cluster?) has made it stable.

That's not a configuration we've targeted or tested for the multi-cluster work so it's definitely not stable but we're probably closer to supporting that deployment model. One could just provide Kiali with a remote cluster secret and maybe it would just work if all the auth knobs already work and are supported. Like I said though, that's not stable or supported yet but it could be added to the multi-cluster roadmap to run an external Kiali.

@israel-hdez
Copy link
Member

israel-hdez commented May 23, 2023

That's not a configuration we've targeted or tested for the multi-cluster work so it's definitely not stable but we're probably closer to supporting that deployment model.

IIRC, the original goal behind these configs was to provide Kiali as a managed service. And that was the motivation to run Kiali outside the user-cluster, so that they don't do any hacking to it. It is not multi-cluster, but smells like it 🙂

Mentioning it for you to keep it in mind and choose if it somehow fits on the M/C work, or just keep this aside.

@jmazzitelli
Copy link
Collaborator Author

I hate half-baked features that we leave rotting on the vine. Can we just remove some of this? This is just technical debt that gets in the way when we are debugging things and trying to fix things.

If things don't work, and no one is fixing it so it becomes stable, then we should just get rid of it.

@israel-hdez
Copy link
Member

Can we just remove some of this? This is just technical debt

Why not? I thought about suggesting it.

Maybe nothing of this is useful?. If so, just do git revert to the merge (if it works).

@libesz
Copy link
Contributor

libesz commented May 24, 2023

Hey! Some updates from my side as I brought this use-case and implementation last year. The meaning of the parameters are correct as @israel-hdez described.
Yes, the use-case is to be able to run Kiali outside the observed cluster, as a managed service i.e. with Hypershift installed Openshift clusters. Istio already supports the external control-plane, so kiali would follow with this change.

Unfortunately the managed service that I worked on (that would use these new kiali params) never made it to GA/Production, that's why I am not submitting any further issues/PRs.
I believe the new parameters are generic enough to be part more, similar use-cases in the future, but I don't have any up-to-date information what would be missing besides the auth part.
I am happy to help reviewing the missing docs, but I am also fine to just remove these for now.

@jmazzitelli
Copy link
Collaborator Author

I can see the CA stuff maybe having some generic utility. We'll have to think about this some more.

@israel-hdez
Copy link
Member

I can see the CA stuff maybe having some generic utility. We'll have to think about this some more.

Yes and no. This is constrained to requests to the ClusterAPI. If Kiali out-of-cluster is not going to be a supported config, I'm not sure it is really useful to keep. Some work may be needed to make this generic and be applicable to other HTTP requests.

For M/C you may already be getting the CAs of remote clusters via kubeconfig secrets or some other means, to properly check the secure connection (@nrfox may confirm).

Besides that, Kiali don't do a lot of connections to other things. Maybe it is connections to Prometheus, Jaeger, Grafana, OpenID server, ¿etc? But, I think all these have their own CA configurations.

The truly generic approach was requested in #1675. It was closed by confusion, but the reporter user was happy with #4050 and wasn't reopened. So, it looks like it didn't gain traction.

Just commenting for a broader view around CAs.

jmazzitelli added a commit to kiali/kiali.io that referenced this issue May 25, 2023
@jmazzitelli
Copy link
Collaborator Author

OK, since this is an incomplete and stale feature, I will close my suggested PR to update the public docs. We'll want to consider reverting some or all of PR #5021

hhovsepy pushed a commit to hhovsepy/kiali.io that referenced this issue Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working docs It requires changes on kiali.io
Projects
Development

Successfully merging a pull request may close this issue.

4 participants