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

Kubernetes configured via kubeconfig #46244

Merged
merged 13 commits into from Mar 12, 2018

Conversation

Projects
None yet
4 participants
@mcalmer
Copy link
Contributor

commented Feb 28, 2018

What does this PR do?

Newer versions of python-kubernetes (>2.0) has changed a lot in the way they want to have the configuration parameters. Also new types of authentication were added and more will come in near future.
All this make the current way to provide authentication data a bit useless.

The only future-proof way seems to be to provide the full kubeconfig file and let the python library
do the rest.

What issues does this PR fix or reference?

Mainly it support new python-kubernetes library as requested in #44701 .

Configuring via kubeconfig was requested in #43514 .

Last but not least this PR make it also possible to provide the full kubeconfig as data to overwrite configured kubeconfig like requested in #46086 .

Previous Behavior

Configuring the cluster access and authentication was possible via single values:

  • kubernetes.user
  • kubernetes.password
  • kubernetes.api_url
  • kubernetes.certificate-authority-data/file
  • kubernetes.client-certificate-data/file
  • kubernetes.client-key-data/file

New Behavior

This will be replaced by just configure via kubeconfig and context

  • kubernetes.kubeconfig
  • kubernetes.context

Tests written?

Yes (adapted the existing)

Commits signed with GPG?

No

Related PR

2017.7 - add warning about config option change : #46320

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2018

@mcalmer Since Kubernetes is moving pretty quickly and this change will make it easier to maintain this module moving forward, I am fine with changing these configuration items in a feature release. However, I want to make sure that this is documented appropriately - there should be warnings at the top of the module that these configuration options have changed, and it probably would be good to add this to the Fluorine release notes.

It would also be prudent to add some deprecation warnings to the old configuration style in the 2017.7 branch.

@rallytime rallytime requested a review from saltstack/team-suse Feb 28, 2018

@mcalmer

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2018

@rallytime do you have an example for such a text? Just to use a similar format and wording.

@isbm
Copy link
Contributor

left a comment

@mcalmer could you please take a look at these changes? Thanks!

kubeconfig = kwargs.get('kubeconfig')
elif 'kubeconfig_data' in kwargs:
kubeconfig_data = kwargs.get('kubeconfig_data')
kubeconfig_data_overwrite = True

This comment has been minimized.

Copy link
@isbm

isbm Feb 28, 2018

Contributor

You don't need this (the whole elif).

if 'context' in kwargs:
context = kwargs.get('context')

if (kubeconfig_data and not kubeconfig) or (kubeconfig_data and kubeconfig_data_overwrite):

This comment has been minimized.

Copy link
@isbm

isbm Feb 28, 2018

Contributor

You don't need kubeconfig_data_overwrite too.


kubeconfig_data_overwrite = False

This comment has been minimized.

Copy link
@isbm

isbm Feb 28, 2018

Contributor

You don't need this.

client_cert_file = __salt__['config.option']('kubernetes.client-certificate-file')
client_key_file = __salt__['config.option']('kubernetes.client-key-file')
kubeconfig = __salt__['config.option']('kubernetes.kubeconfig')
kubeconfig_data = __salt__['config.option']('kubernetes.kubeconfig-data')

This comment has been minimized.

Copy link
@isbm

isbm Feb 28, 2018

Contributor

Just that:

kubeconfig_data = kwargs.get('kubeconfig_data') or __salt__['config.option']('kubernetes.kubeconfig-data')

This comment has been minimized.

Copy link
@mcalmer

mcalmer Mar 1, 2018

Author Contributor

Your suggestion is not directly working like it should. But I modify it to go in that direction.

if 'kubeconfig' in kwargs:
kubeconfig = kwargs.get('kubeconfig')
if os.path.exists(kubeconfig) and os.path.basename(kubeconfig).startswith('salt-kubeconfig-'):
salt.utils.files.safe_rm(kubeconfig)

This comment has been minimized.

Copy link
@isbm

isbm Feb 28, 2018

Contributor

No need to stat file here, you could just try/except and check for errno.ENOENT:

try:
    os.unlink(kubeconfig)
except (IOError, OSError) as err:
    if err.errno != errno.ENOENT:
        log.exception()

If you think no logging should be happening, then the safe_rm all it does it try/except:pass. 😉 So you can simply safe_rm it as is w/o pre-checks.

This comment has been minimized.

Copy link
@isbm

isbm Feb 28, 2018

Contributor

BTW, interesting that you don't know what is inside kubeconfig... 😉

This comment has been minimized.

Copy link
@mcalmer

mcalmer Mar 1, 2018

Author Contributor

No, this is not working. I only want to remove it when it is a temp file created by the module itself.
So just unlink is not an option. I can either just check if I have something in kubeconfig or call
path.exists(). I like to check if kubeconfig is a "path" and if it exists before I remove it.

'kubernetes.kubeconfig': '/home/testuser/.minikube/kubeconfig.cfg',
'kubernetes.context': 'minikube'
}
return data.get(name, value)

This comment has been minimized.

Copy link
@isbm

isbm Feb 28, 2018

Contributor

Why copypaste the same thing all over again? It can be just a static method and you could do something like:

    @staticmethod
    def kube_settings(name, value=None):
        data = {....}
        return data.get(name, value)

    def test......:
        with mock_kubernetes_library() ....:
            with patch.dict(kubernetes.__salt__, {'config.option': Mock(side_effect=KubernetesTestCase.kube_settings)}):
                ...

...and then just reuse it twice. I am not fan with multiple nested with statements (which you still can have multiple conditions in just one statement) and I would use @patch decorator instead, but for now it is OK.

Please refactor this.

config['key_file'],
)
with mock_kubernetes_library() as mock_kubernetes_lib:
with patch.dict(kubernetes.__salt__, {'config.option': Mock(side_effect=settings)}):

This comment has been minimized.

Copy link
@isbm

isbm Feb 28, 2018

Contributor

It can be just one statement, BTW.

os.unlink(kubeconfig)
except (IOError, OSError) as err:
if err.errno != errno.ENOENT:
log.exception('Removing kubeconfig failed: {0}'.format(err))

This comment has been minimized.

Copy link
@isbm

isbm Mar 1, 2018

Contributor

In Salt core normally we just so: log.exception(err) — and it will do all the job for you.

@isbm

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2018

@mcalmer just a little nitpick with the log.exception(...) but the rest is great and much simpler now! Thanks.

For an item only one field should be provided. Either a `data` or a `file` entry.
In case both are provided the `file` entry is prefered.
Only `kubeconfig` or `kubeconfig-data` should be provided. In case both are
provided `kubeconfig` entry is prefered.

This comment has been minimized.

Copy link
@isbm

isbm Mar 1, 2018

Contributor

Typo: preferred.

These settings can be also overrided by adding `context and `kubeconfig` or

This comment has been minimized.

Copy link
@isbm

isbm Mar 1, 2018

Contributor

overridden

@mcalmer mcalmer force-pushed the mcalmer:k8s-with-kubeconfig branch from 5005e6e to fdf8fdb Mar 1, 2018

@isbm

isbm approved these changes Mar 2, 2018

Copy link
Contributor

left a comment

@mcalmer thanks!

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Mar 2, 2018

@mcalmer I don't have any doc examples handy off the top of my head, but something along the lines of "The following configuration options have been removed in favor of x, y, z" and list out which items were removed (and why they were removed) and what to use instead. For example, if someone is using 2017.7.x or 2018.3.x and they upgrade and now their modules/states don't work, they need to know why and how to remediate the issue. This needs to be added to the Fluorine release notes.

I also would like to see a PR implementing the warn_util warnings in the 2017.7 branch so people will know that the old config options will be removed in Fluorine.

@jryberg

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2018

Will overrides in states still be possible using this method? I'm connecting to multiple clusters from the same minion.

@mcalmer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2018

@rallytime I have adapted the warning message here, but I am not sure if using warn_until in older versions is a good idea. These messages would just pollute the logfiles with tons of warnings and
the admin has no chance to remove them because the new options are not available in the old version.

So I would like to go with just a message in the docs.

An alternative would be to backport the feature and try to make both options available - let's say in oxygen. Then warn_until would make sense because the admin could switch to the new config style and prevent the warnings.

What do you prefer?

@mcalmer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2018

@jryberg - yes. Overwrite is still possible. But now you use a full kubeconfig instead of single values out of it.

@isbm

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2018

@mcalmer I would say, we should have indeed some sort of warning and a proper deprecation process. For this we actually using deprecation mechanism and it is just a decorator on top of a function. This decorator makes possible to retire old signatures without losing the API names. Full doc of which you can read here. This is something like:

def _stuff():
    'This has old signature. Underscore in front is hiding it initially'

@with_deprecated(globals(), "Beryllium")
def stuff(foo=None):
    'This has new signature'

Then, depending on what user wants to use (new or old Kubernetes), they just configure the minion accordingly.

@mcalmer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2018

@isbm - I fear it will not work as the signatures stay unchanged for all methods. Only configuration in the background will be different and you can provide different kwargs but they are optional.

@isbm

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2018

@mcalmer technically, surely it will. It is just in this case it is equivalent to:

@with_deprecated(globals(), "Fluorine")
def foo(**kwargs):
    ... # all the real code

_foo = foo

The problem is here that due to the nature of the **kwargs that can be anything inside, it does not matter which you use: new or old, since it is technically the same function reference. So the workaround would be a bit of refactoring:

def _version_blocker(func, **kwargs):
    '''
    This will raise an exception if kubernetes is more than 2.0, for example.
    '''
    if _get_version_of_kubernetes() > (2, 0):  # Check what API we are linked to
        raise CommandExecutionError('Sticky bit become loose!')
    return func(**kwargs)

@with_deprecated(globals(), "Fluorine")
def foo(**kwargs):
    ... # all the real code

_foo = lambda **kwargs: _version_blocker(foo, **kwargs)

@with_deprecated(globals(), "Fluorine")
def bar(**kwargs):
    ... # all the real code

_bar = lambda **kwargs: _version_blocker(bar, **kwargs)

Well, you've got the idea.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2018

@mcalmer Regarding your questions about a proper deprecation process - what if we keep the old way of doing things for now with a warn_util message when the old configs are found, and then users can update over the next couple of feature releases and then we can remove the old behavior after the deprecation period has passed. This is what we usually do and I should have suggested doing that the first time around. My apologies for the run-around on that!

@mcalmer mcalmer force-pushed the mcalmer:k8s-with-kubeconfig branch from cd33e0c to 3d357a6 Mar 10, 2018

@mcalmer

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2018

@rallytime - It is not easy as new python-kubernetes client lib do not support this old way.
So I bring just the old code back and try to catch all exceptions which might get thrown because of invlaud use of the API. Old configuration can only be used upto v2.0.0 . New style works with all.

@mcalmer mcalmer force-pushed the mcalmer:k8s-with-kubeconfig branch from 3d357a6 to ada08c2 Mar 10, 2018

@mcalmer mcalmer force-pushed the mcalmer:k8s-with-kubeconfig branch from 0f74c6c to 7e39a95 Mar 11, 2018

@rallytime
Copy link
Contributor

left a comment

👍

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2018

re-run ubuntu

@rallytime rallytime merged commit bcf8436 into saltstack:develop Mar 12, 2018

4 of 10 checks passed

codeclimate 28 issues to fix
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #3066 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #20740 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #7796 — FAILURE
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #17113 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #23146 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #15433 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #20046 — SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.