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

Fix kubeclient v4.9.1 usage with group apis #927

Merged
merged 6 commits into from
Sep 18, 2020

Conversation

pmalek-sumo
Copy link
Contributor

Description

kubeclient's version v4.9.1 changed how group API's clients are created and broke our fluent-plugin-enhance-k8s-metadata tests (more info in #871).

To fix this problem we need to pass an almost full base URL (e.g. apis/extensions) as url to Kubeclient::Client.new() and the only the part with the version as version (e.g. v1beta1) not how it was used up until now with url (base) being only the apis/ and version being set to "full version" with part of base e.g. extensions/v1beta1.

Discussion in kubeclient's repo for reference: ManageIQ/kubeclient#318 (comment)

Testing performed
  • ci/build.sh
  • Redeploy fluentd and fluentd-events pods
  • Confirm events, logs, and metrics are coming in

@@ -33,7 +33,7 @@ class EnhanceK8sMetadataFilter < Fluent::Plugin::Filter
# Need different clients to access different API groups/versions
# https://github.com/abonas/kubeclient/issues/208
config_param :core_api_versions, :array, default: ['v1']
config_param :api_groups, :array, default: ['apps/v1', 'extensions/v1beta1']
config_param :api_groups, :hash, default: {'apps':'v1', 'extensions':'v1beta1'}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link

@cben cben left a comment

Choose a reason for hiding this comment

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

From kubeclient's side, this looks correct 👍

Just to clarify, passing foo/v1 in version was never documented nor tested in kubeclient. It just happened to work, sometimes.
But there is no question that it would be nice to make it work, nobody likes the current scheme. Tracked in ManageIQ/kubeclient#284, PRs welcome 😉

if elems.empty? then return {} end

# only a version: create a non-group api
if elems.length < 2
Copy link

Choose a reason for hiding this comment

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

Since you're supporting here both 'v1' and 'foo/v2' formats, do you still need separate core_clients vs group_clients methods and separate config params / consts?

Your call of course, just wanted to point out that from kubeclient's perspective, aside from Client construction, there is no difference between "core" API and other API groups...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @cben!
I gave it a shot.

Copy link
Contributor

@samjsong samjsong left a comment

Choose a reason for hiding this comment

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

LGTM

@pmalek-sumo pmalek-sumo merged commit d444caa into master Sep 18, 2020
@pmalek-sumo pmalek-sumo deleted the fix-kubeclient-v4.9.1-usage-with-group-apis branch September 18, 2020 16:40
@pmalek-sumo
Copy link
Contributor Author

Friday merge 🎉

@perk-sumo perk-sumo added this to the v1.3 milestone Sep 22, 2020
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.

Fix tests for fluent-plugin-enhance-k8s-metadata
6 participants