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

Remove hard-coded namespace from k8s config #5482

Merged
merged 1 commit into from
Nov 2, 2017

Conversation

jpeeler
Copy link

@jpeeler jpeeler commented Oct 31, 2017

See commit text for more details. I haven't actually tested this yet, but looking at the client code I can't see how this would fail in any previously working scenario. @tarasglek care to verify all is well now?

(#5378)

@elasticmachine
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution! I'll give this a try to make sure the client behaves as expected. Left a comment on the code. Also feel free to add an entry to CHANGELOG.asciidoc

return kubeAnnotatorConfig{
InCluster: true,
SyncPeriod: 1 * time.Second,
CleanupTimeout: 60 * time.Second,
Namespace: "kube-system",
Copy link
Contributor

@exekias exekias Oct 31, 2017

Choose a reason for hiding this comment

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

I think it's best if we remove Namespace field from kubeAnnotatorConfig, and move all references to client.Namespace

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I considered doing that initially as well. Will adjust now!

@exekias
Copy link
Contributor

exekias commented Oct 31, 2017

jenkins, test it please

@exekias exekias closed this Oct 31, 2017
@exekias exekias reopened this Oct 31, 2017
@elasticmachine
Copy link
Collaborator

Can one of the admins verify this patch?

The default namespace should be populated by the Kubernetes client. In the case
of in-cluster config, the namespace will be read from the
/var/run/secrets/kubernetes.io/serviceaccount/namespace file. (Note that this
assumes the ServiceAccount admission controller is in use.) In the case of
parsing a k8s configuration file, the default namespace is set to the client
default of "default".

Minor spelling correction of Kubernetes and some additional error information
was changed as well.

Closes elastic#5378
@exekias
Copy link
Contributor

exekias commented Nov 2, 2017

Just gave this a try, works for me, thank you 🎉

@exekias exekias merged commit fe0dcce into elastic:master Nov 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants