Skip to content

Conversation

spadgett
Copy link
Member

@itewk
Copy link

itewk commented Oct 13, 2016

Not to be nit picky but is it possible to make the warning message more specific? Like specifically say the SSL cert needs to be accepted for it to work. The way it is now if I see this I am going to assume something is wrong with my metrics deployment and I am not going to know just accepting the cert will fix all my problems.

@spadgett
Copy link
Member Author

@itewk I'm concerned about encouraging users to accept invalid SSL certificates. I specifically avoided that. You shouldn't need to manually accept the cert in a properly configured cluster.

Also we can't know with certainty that certificates are the problem. We get back status code 0, which can have other causes like not having network connectivity.

@itewk
Copy link

itewk commented Oct 13, 2016

@spadgett alright. is there at least a KB article somewhere? Right now its like this magic trick. You accept the cert and whoosh everything starts working. No indication that was what fixed it. And it seems to be a thing that anyone who has done this before "just knows" but if someone is installing this for the first time without outside help it is confusing.

As far as using "valid certs". That would be great if that was the case but more often then not I see people who "don't bother" putting signed certs on their underlying infrastructure and say the self signed are "good enough" for something that is completely internal. Not saying that is the 'best' or 'correct' choice, but it is common enough to be a concern when talking about a feature like metrics.

@spadgett
Copy link
Member Author

Right now its like this magic trick.

It's hard because it's something you should not do except in a development environment. There's a reason browsers show that warning. In a real cluster, the cluster admin should configure metrics with a valid certificate.

type: 'warning',
message: 'An error occurred getting metrics.',
links: [{
href: data.url,
Copy link
Member

Choose a reason for hiding this comment

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

Is this opening the particular request that failed, or is it going to the splash page?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the splash page.

@jwforres
Copy link
Member

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to 29811dd

@openshift-bot
Copy link

openshift-bot commented Oct 13, 2016

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/498/) (Base Commit: bc14c3f)

@openshift-bot openshift-bot merged commit b7fa730 into openshift:master Oct 13, 2016
@spadgett spadgett deleted the overview-metrics-warning branch October 17, 2016 12:54
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.

4 participants