HACK: Ensure the clusterName is managed consistently...
#7
Conversation
|
This PR is a followup of PR #4, especially this comment especially this sentence:
|
| } else { | ||
| klog.Infof("NO SUB: %T %s", curObj, clusterName) | ||
| } | ||
| if clusterName == "" { |
There was a problem hiding this comment.
You can't make this change because it would break normal usage of kubernetes starting kube-apiserver. That's what can't change in the serving chain. Inside kube-apiserver's lower layers you can't assume the cluster context is present. In other places (like client, or code that MUST have cluster name), you can error if it's not present.
Principle: our "added hacks" still have to not break the behavior of the k/k repo.
There was a problem hiding this comment.
I agree on the principle of not to break the behavior of the k/k repo so I obviously missed some of the impacts here.
There was a problem hiding this comment.
Do you mean that I should only emit error logs but not fail at all the places where I check for valid cluster presence in the storage/etcd3 files such as the follownig places:
There was a problem hiding this comment.
Can we add some flag to reject requests without a cluster context? I agree we shouldn't break k/k but it's also useful in our usage to reject requests that don't make sense for us.
|
@smarterclayton I changed the code to only emit error logs, but not fail, at all the places where I check for a valid cluster presence in the |
| if headerCluster != "" { | ||
| req.Header.Add("X-Kubernetes-Cluster", headerCluster) | ||
| if headerCluster == "" { | ||
| return nil, fmt.Errorf("Cluster should not be empty for request '%s' on resource '%s' (%s)", requestInfo.Verb, requestInfo.Resource, requestInfo.Path) |
There was a problem hiding this comment.
You can use %q instead of '%s' to get quoted strings using fmt.
staging/src/k8s.io/apiserver/pkg/endpoints/request/context_cluster.go
Outdated
Show resolved
Hide resolved
We should not allow any operation that results in an object having an empty `clusterName` For etcd3 storage functions, we only emit a warning, in order to avoid breaking normal usage of kubernetes starting kube-apiserver. Signed-off-by: David Festal <dfestal@redhat.com>
davidfestal
force-pushed
the
ensure-no-empty-clusterName
branch
from
9e47213
to
ff838d2
Compare
davidfestal
changed the title
clusterName is managed consistently...clusterName is managed consistently...
We should not allow any operation that results in an object having an empty
clusterName.