Skip to content
This repository was archived by the owner on Jan 19, 2018. It is now read-only.

Add a new 'library' for k8s/openshift providers. - #712

Merged
cdrage merged 1 commit into
projectatomic:masterfrom
cdrage:add-openshift-k8s-library
Jun 8, 2016
Merged

Add a new 'library' for k8s/openshift providers.#712
cdrage merged 1 commit into
projectatomic:masterfrom
cdrage:add-openshift-k8s-library

Conversation

@cdrage

@cdragecdrage commented Apr 25, 2016

Copy link
Copy Markdown
Member

This is the first commit and first initialization into refactoring the
OpenShift provider in order to create a library that is both compatible
with OpenShift as well as Kubernetes.

With this 'library', the ONLY thing that is passed in is an object of
the .kube/config configuration. After passing the configuration, you may
make API calls such as .create(object) and .delete(object).

This commit integrates the library as well as converts the current Kubernetes provider from cmd-line (using kubectl) to using the API.

@cdrage
cdrageforce-pushed the add-openshift-k8s-library branch 4 times, most recently from 028c3e2 to 22d3269CompareApril 27, 2016 18:59
@cdragecdrage mentioned this pull request Apr 27, 2016
4 tasks
@cdrage
cdrageforce-pushed the add-openshift-k8s-library branch 5 times, most recently from 986f017 to 538dc65CompareMay 2, 2016 05:55
@concafconcaf mentioned this pull request May 2, 2016
7 tasks
@cdrage
cdrageforce-pushed the add-openshift-k8s-library branch 10 times, most recently from 4f17d71 to 69b1ed0CompareMay 5, 2016 04:39
@cdrage
cdrageforce-pushed the add-openshift-k8s-library branch 5 times, most recently from 720074d to fcda016CompareMay 11, 2016 19:59
@cdrage

Copy link
Copy Markdown
MemberAuthor

Functional tests failing due to: https://github.com/projectatomic/adb-tests/pull/32/files

@cdrage
cdrageforce-pushed the add-openshift-k8s-library branch 2 times, most recently from 9774d95 to 5f07c13CompareMay 17, 2016 18:47
@dustymabe

Copy link
Copy Markdown
Contributor

@dustymabe sorry, was a bit confused on what you meant. is this regarding any of the references to self.config? Since that inherits from the config of the Provider class?

So the best way to separate the two would be to simply do self.kconfig = self.config on startup and use kconfig?

config (object): An object of the .kube/config configuration

I think anything that is referencing an object of the .kube/config configuration should be known as kconfig. This would be an attempt to disambiguate between kconfig and config (the class that rtnpro is writing).

@cdrage

Copy link
Copy Markdown
MemberAuthor

@dustymabe ahhhh, that's what you meant! I was getting confused between @rtnpro 's refactor and .kube/config, I'll change the code to reflect that :)

@kadel

kadel commented May 30, 2016

Copy link
Copy Markdown
Collaborator

I tested this on Kubernetes running on AWS

When authenticating using token, everything works fine.

When using client-certificate, client-key it fails

▶ atomicapp-dev stop . --provider=kubernetes --provider-config=/home/tomas/.kube/config --namespace=default --verbose atomicapp-dev:1: command not found: INFO :: - cli/main.py :: _ _ _ _
INFO :: - cli/main.py :: /_\| |_ ___ _ __ (_)__ /_\ _ __ _ __ Version: 0.5.2
INFO :: - cli/main.py :: / _ \ _/ _ \ ' \| / _|/ _ \| '_ \ '_ \ Nulecule: 0.0.2
INFO :: - cli/main.py :: /_/ \_\__\___/_|_|_|_\__/_/ \_\ .__/ .__/ Mode: Stop
INFO :: - cli/main.py :: |_| |_|
DEBUG :: - cli/main.py :: Final parsed cmdline: stop . --provider=kubernetes --provider-config=/home/tomas/.kube/config --namespace=default --verbose
DEBUG :: - nulecule/main.py :: NuleculeManager init app_path: .
DEBUG :: - nulecule/main.py :: NuleculeManager init image: None
DEBUG :: - utils.py :: Loading answers from file: ./answers.conf
DEBUG :: - utils.py :: Loading answers from file: ./answers.conf.gen
DEBUG :: - providers/kubernetes.py :: Given config: {u'provider-tlsverify': False, u'provider-config': u'/home/tomas/.kube/config', u'provider': u'kubernetes', u'image': u'centos/httpd', u'hostport': 80, u'namespace': u'default'}
INFO :: - providers/kubernetes.py :: Using namespace default
DEBUG :: - providers/kubernetes.py :: Processing artifact: artifacts/kubernetes/.hello-apache-pod.json
DEBUG :: - utils.py :: Finding the users home directory
DEBUG :: - utils.py :: Running as user tomas. Using home directory /home/tomas for configuration data
DEBUG :: - providers/kubernetes.py :: Provider configuration provided
ERROR :: - cli/main.py :: 'token'
Traceback (most recent call last):
File "/home/tomas/dev/github/kadel/atomicapp/atomicapp/cli/main.py", line 130, in cli_func_exec
cli_func(cli_func_args)
File "/home/tomas/dev/github/kadel/atomicapp/atomicapp/cli/main.py", line 97, in cli_stop
nm.stop(**argdict)
File "/home/tomas/dev/github/kadel/atomicapp/atomicapp/nulecule/main.py", line 349, in stop
self.nulecule.stop(cli_provider, dryrun)
File "/home/tomas/dev/github/kadel/atomicapp/atomicapp/nulecule/base.py", line 223, in stop
component.stop(provider_key, dryrun)
File "/home/tomas/dev/github/kadel/atomicapp/atomicapp/nulecule/base.py", line 351, in stop
provider.init()
File "/home/tomas/dev/github/kadel/atomicapp/atomicapp/providers/kubernetes.py", line 101, in init
self.api = Client(KubeConfig.from_file(self.config_file), "kubernetes")
File "/home/tomas/dev/github/kadel/atomicapp/atomicapp/providers/lib/kubeshift/client.py", line 42, in __init__
self.connection = KubeKubernetesClient(config)
File "/home/tomas/dev/github/kadel/atomicapp/atomicapp/providers/lib/kubeshift/kubernetes.py", line 41, in __init__
self.api = KubeBase(config)
File "/home/tomas/dev/github/kadel/atomicapp/atomicapp/providers/lib/kubeshift/kubebase.py", line 69, in __init__
self.token = self.user['token']
KeyError: 'token'

Then there are two another options for user to authenticate:

  • client-certificate-data and client-key-data - only difference between this and client-certificate and client-key is that when using -data key and cert is included in config (base64 encoded)
  • basic auth - simple http basic auth with username and password

But those things are probably candidates for another PR.

@cdrage

Copy link
Copy Markdown
MemberAuthor

@kadel
Could you post your .kube/config (with your API credentials balnked out and whatever) I think this is an edge-case with user being set with no token available.

@kadel

Copy link
Copy Markdown
Collaborator

@cdrage I don't think that this is edge case, this is how authentication for system:admin user in OpenShift is setup by default (using client-certificate-data and client-key-data).

If you have config with both client-certificate/key and token for one user, you have to decide which one to use, it doesn't make sense to use both, even if in this case it is technically possible.

Examples of .kube/config with different auth methods:

@cdrage

Copy link
Copy Markdown
MemberAuthor

@kadel thank you for posting them! I'm going to go through each-one and figure out how we can work this in.

@cdrage

Copy link
Copy Markdown
MemberAuthor

@kadel how did we do certificate-authority-data in the openshift.py provider? or was this intended to be added in the cert commit for openshift?

@kadel

Copy link
Copy Markdown
Collaborator

@cdrage yes it was part of old PR with cert auth. Here is example how we handled that https://github.com/kadel/atomicapp/blob/bdc3a2bd3cc953ea1436392367a3f92a1b532b8d/atomicapp/providers/lib/kubeconfig.py#L116,L119
Temporray files was becuase requests library accepts certs only as files, or I didn't figure out how to do this differently

@cdrage
cdrageforce-pushed the add-openshift-k8s-library branch from 858de3b to ea14d6dCompareMay 31, 2016 15:23
'''

# If it starts with /, we assume it's a filename so we just return that.
if data.startswith('/'):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

it can be related path to .kube/config and than it is not starting with /

@cdrage
cdrageforce-pushed the add-openshift-k8s-library branch from ea14d6d to c848bdaCompareJune 3, 2016 15:03
@cdrage

Copy link
Copy Markdown
MemberAuthor

@kadel updated the PR with your feedback on the -data files. In particular, it's in kubebase.py in relation to the cert_file declarations around the requests library.

@kadel

kadel commented Jun 6, 2016

Copy link
Copy Markdown
Collaborator

Sorry, but It is not working like this :-(
You are still expecting that under one key in .kube/config can be either base64 encoded string or filename. But this is not right.
For example CA certificate can be in certificate-authority (than it is path to file) or in certificate-authority-data than it is base64 encoded string.

Look at this example: https://gist.github.com/kadel/29df1fee245627ed90e53b44cdb7c2e7

@cdrage
cdrageforce-pushed the add-openshift-k8s-library branch from c848bda to 4cadf1bCompareJune 6, 2016 11:59
@kadel

kadel commented Jun 6, 2016

Copy link
Copy Markdown
Collaborator

There are still some missing features to properly handle Kubernetes config file.
But we can add those in another PR.

  • support for relative paths to certificates and keys
  • support for *-data keys (certificate-authority-data, client-certificate-data, key-certificate-data)

👍 LGTM
Awesome work @cdrage

@cdrage

Copy link
Copy Markdown
MemberAuthor

@kadel

Thanks man! 👯

I'll add the -data as a separate PR as well as the support for relative paths (will have to use the utils.py functions we had before to find the proper path, due to /host being passed, etc.)

This is the first commit and first initialization into refactoring the
OpenShift provider in order to create a library that is both compatible
with OpenShift as well as Kubernetes.
With this 'library', the *ONLY* thing that is passed in is an object of
the .kube/config configuration. After passing the configuration, you may
make API calls such as .create(object) and .delete(object).
@cdrage
cdrageforce-pushed the add-openshift-k8s-library branch from 4cadf1b to 69875f6CompareJune 7, 2016 14:23
@cdrage

Copy link
Copy Markdown
MemberAuthor

@dustymabe I think this may be good to go! I've updated kubebase to reflect the wording change on self.config to self.kubeconfig

This may be ready for mergin' 👍

@cdrage

Copy link
Copy Markdown
MemberAuthor

#dotests

@dustymabe

Copy link
Copy Markdown
Contributor

@cdrage - I haven't looked at this but from what I reviewed a week ago everything looked good. Since Tomas gave +1 I'm +1. Merge if you can get all tests passing.

@cdrage

Copy link
Copy Markdown
MemberAuthor

WOOO! Kubernetes tests pass! 🎉

Merging away :)

@cdrage

Copy link
Copy Markdown
MemberAuthor

Thanks everyone for helping me out with this! It's been exciting to finally get this in.

@cdrage
cdrage merged commit 1c20c23 into projectatomic:masterJun 8, 2016
@dustymabe

Copy link
Copy Markdown
Contributor

haha openshift tests failed - thoughts about that?

@cdrage

Copy link
Copy Markdown
MemberAuthor

@dustymabe false alarm, tests passed locally for OpenShift on my system :)

Seems the previous issue is occuring again with the latest update to OpenShift:


ERROR :: - cli/main.py :: 403 {u'status': u'Failure', u'kind': u'Status', u'code': 403, u'apiVersion': u'v1', u'reason': u'Forbidden', u'details': {u'kind': u'pods'}, u'message': u'User "system:anonymous" cannot create pods in project "foo"', u'metadata': {}}

@cdrage

Copy link
Copy Markdown
MemberAuthor

Let me test one more thing ^^ updated my OpenShift image and it failed.

@cdrage

Copy link
Copy Markdown
MemberAuthor

@dustymabe should be fine, no need to revert or anything. i knew I didn't touch the openshift integration (didn't change any underlying code)

I've opened up projectatomic/adb-tests#33 to fix this.

raise KubeKubernetesError("Kubernetes API URL does not include HTTP or HTTPS")

# Gather what end-points we will be using
self.k8s_api = urljoin(url, "api/v1/")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are there plans to support the other groups of APIS accessible via the /apis endpoint (e.g. - apps, autoscaling, batch, extensions, policy, etc.)?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@khrisrichardson We hope so. Any changes to https://github.com/projectatomic/nulecule will reflect in atomic app in terms of providing support for autoscaling, batch commands, extensions, policies, etc. If the spec changes we'll reflect that as features in Atomic App :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, @cdrage

Is there a timeline in mind, because by restricting to api/v1, the behavior may be a regression for some?

ERROR :: No kind by that name: Deployment
Traceback (most recent call last):
File "/usr/local/lib/python2.7/dist-packages/atomicapp-0.5.2-py2.7.egg/atomicapp/cli/main.py", line 130, in cli_func_exec
cli_func(cli_func_args)
File "/usr/local/lib/python2.7/dist-packages/atomicapp-0.5.2-py2.7.egg/atomicapp/cli/main.py", line 84, in cli_run
nm.run(**argdict)
File "/usr/local/lib/python2.7/dist-packages/atomicapp-0.5.2-py2.7.egg/atomicapp/nulecule/main.py", line 320, in run
nulecule.run(cli_provider, dryrun)
File "/usr/local/lib/python2.7/dist-packages/atomicapp-0.5.2-py2.7.egg/atomicapp/nulecule/base.py", line 224, in run
provider.run()
File "/usr/local/lib/python2.7/dist-packages/atomicapp-0.5.2-py2.7.egg/atomicapp/providers/kubernetes.py", line 268, in run
self.api.create(artifact, self.namespace)
File "/usr/local/lib/python2.7/dist-packages/atomicapp-0.5.2-py2.7.egg/atomicapp/providers/lib/kubeshift/client.py", line 49, in create
self.connection.create(obj, namespace)
File "/usr/local/lib/python2.7/dist-packages/atomicapp-0.5.2-py2.7.egg/atomicapp/providers/lib/kubeshift/kubernetes.py", line 62, in create
kind, url = self._generate_kurl(obj, namespace)
File "/usr/local/lib/python2.7/dist-packages/atomicapp-0.5.2-py2.7.egg/atomicapp/providers/lib/kubeshift/kubernetes.py", line 142, in _generate_kurl
raise KubeKubernetesError("No kind by that name: %s" % kind)
KubeKubernetesError: No kind by that name: Deployment

I was testing kubernetes v1.3.0-alpha.5, and prior to pulling this commit, I was able to create objects belonging to the extensions/v1beta1 API, such as DaemonSet and Deployment.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@khrisrichardson Ah! That's what you meant. I'm going to open up an issue for this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have a patch to support API groups. Will fork and create PR.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@cdrage@dustymabe@kadel@khrisrichardson