discover in cluster namespace for leader election #24
Conversation
Signed-off-by: Andrew Sy Kim <[email protected]>
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
| // inClusterNamespace returns the namespace in which the pod is running in by checking | ||
| // the env var POD_NAMESPACE, then the file /var/run/secrets/kubernetes.io/serviceaccount/namespace. | ||
| // if neither returns a valid namespace, the "default" namespace is returned | ||
| func inClusterNamespace() string { |
There was a problem hiding this comment.
I prefer not to fall back to default here and just return an error. I left the fallback to default for now since this is used in external-provisioner. Do we know of any situations where a sidecar wouldn't run in the cluster? If not I think the fallback to default should be removed and instead exit with error
There was a problem hiding this comment.
Can we say that for Lease mechanism (which should be all new), require a namespace field to be set if not found, but for the other two, we allow the default for backwards compatibility?
There was a problem hiding this comment.
We could, though the implementation will be a bit messy I think. I added the WithNamespace method for the cases where we need to account for backwards compatibility. I guess the this boils down to two questions:
- how reliable is it for the namespace file to exist (I assume this is not going to change anytime in the near future)
- would any user run this "out of cluster"
There was a problem hiding this comment.
- I don't expect this to change. It would probably be considered an API break
- It's possible someone may want to run this out of cluster, but we can wait for a feature request to add the option back.
There was a problem hiding this comment.
I'm okay with the current PR merging as is, but having the --leader-election-namespace flag as optional in all the sidecars. Setting --leader-election-namespace calls the namespace override using WithNamespace. Otherwise, default to cluster namespace. Thoughts?
There was a problem hiding this comment.
I think it's fine to maintain backwards compatibility. If the default doesn't work, then the user needs to explicitly specify something.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewsykim, msau42 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Signed-off-by: Andrew Sy Kim [email protected]
Adds support to discover namespaces in cluster by checking the env var POD_NAMESPACE, then the file
/var/run/secrets/kubernetes.io/serviceaccount/namespace(should exist in every pod). Falls back to default.