Identify the cluster in the NewRuntime call #574
Conversation
Contact the openshift version endpoint to verfiy what cluster the broker is running on.
openshift-ci-robot
added
the
size/M
|
Changes Unknown when pulling d798afe on rthallisey:runtime into ** on openshift:master**. |
pkg/app/app.go
Outdated
| default: | ||
| app.log.Error(err.Error()) | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| app.log.Debug("Connecting Dao") | ||
| app.dao, err = dao.NewDao(app.config.Dao, app.log.Logger) |
There was a problem hiding this comment.
Should we handle this error or should we remove the error?
There was a problem hiding this comment.
We should be handling it in case the etcd config file is not found.
pkg/runtime/runtime.go
Outdated
| panic(err.Error()) | ||
| } | ||
| log.Info("OpenShift version: %v", kubeServerInfo) | ||
| cluster = openshift{} |
There was a problem hiding this comment.
Thinking about the future, can we make these a function that returns a coe?
There was a problem hiding this comment.
I was thinking that the coe interface would stay private so that only functions in the runtime pkg have access to openshift.go and kubernetes.go. That way, the broker only asks for things it understands like "I want a new sandbox to securely create this apb" instead of "I want a few rbac rules, a new service account, and a new namespace".
What did you have in mind for the future with coe?
There was a problem hiding this comment.
well I am not saying to make it public, I am talking about making an unexported helper method that encapsulates the retrieval of an actual coe structure.
This is for something like the network joining only for openshift and only when a certain plugin is installed. This means that we would want to do extra stuff while creating a coe and it was just a preemptive thing. Once we need it we can always add it as well.
There was a problem hiding this comment.
Ok I made an assumption of what you meant based on how I was using NewRuntime. If I understand what you mean, you would like something similar to what we have with the kubernetes client? I'll post a revision with that assumption.
pkg/runtime/runtime.go
Outdated
| } | ||
| log.Info("OpenShift version: %v", kubeServerInfo) | ||
| cluster = openshift{} | ||
| case kapierrors.IsNotFound(err) || kapierrors.IsUnauthorized(err) || kapierrors.IsForbidden(err): |
There was a problem hiding this comment.
If we are accepting the scenario where we are not authorized to access the /version/openshift endpoint, should we not be ensuring we have permission to do work on a kubernetes cluster first?
I'm struggling to ask this question clearly. So I'm going to ask the same question again, worded differently, in the hopes I can communicate the idea. If we are not authorized / forbidden to access the /version/openshift and we are not authorized / forbidden to access /version/kubernetes (assuming there is a k8s version endpoint somewhere), should we fail?
There was a problem hiding this comment.
Agreed @djzager. In /pkg/app/app.go, I added a failure case on kubernetes because if this validation fails then we aren't connected to a cluster.
case kapierrors.IsNotFound(err) || kapierrors.IsUnauthorized(err) || kapierrors.IsForbidden(err):
app.log.Error("the server could not find the requested resource")
app.log.Error(err.Error())
os.Exit(1)
|
Changes Unknown when pulling 227970d on rthallisey:runtime into ** on openshift:master**. |
pkg/app/app.go
Outdated
| @@ -172,21 +172,15 @@ func CreateApp() App { | |||
| // Initialize Runtime | |||
| agnosticruntime.NewRuntime(app.log.Logger) | |||
|
|
|||
| app.log.Debug("Connecting Dao") | |||
| app.dao, err = dao.NewDao(app.config.Dao, app.log.Logger) | |||
|
|
|||
| k8scli, err := clients.Kubernetes(app.log.Logger) | |||
There was a problem hiding this comment.
I'm curious, based on the answer to my previous question, if this getting of a Kubernetes client, getting a k8s rest client, and checking the k8s version is something we should push down into the NewRuntime function. That way these checks could be collocated and maybe even simplify the error handling on app startup.
There was a problem hiding this comment.
This validation could be behind the runtime pkg. Theoretically, the runtime can be something other than k8s or openshift so we shouldn't assume.
|
Changes Unknown when pulling 4ef4c5b on rthallisey:runtime into ** on openshift:master**. |
openshift-ci-robot
added
size/L
|
Changes Unknown when pulling 8160b2d on rthallisey:runtime into ** on openshift:master**. |
@shawn-hurley's comment disappeared. Everywhere that we grab the kubernetes client and use it should be a function in runtime. There are numerous cases of this so we'll do that in different patches. Feel free to write them if you see them. Tag this issue in those PRs: #575 |
* Identify the cluster in the NewRuntime call Contact the openshift version endpoint to verfiy what cluster the broker is running on. * Capture dao error * Make sure each instance creation has it's own function * Move cluster validation to runtime pkg
Describe what this PR does and why we need it:
Contact the openshift version endpoint to verfiy what cluster
the broker is running on. If the openshift endpoint isn't present, the
broker is running on kubernetes.
Changes proposed in this pull request