Skip to content
This repository has been archived by the owner on Mar 13, 2022. It is now read-only.

Bugfix Azure AD auth config field 'expired-on': checking an int against time.struct_time #121

Closed
wants to merge 1 commit into from

Conversation

lbach
Copy link

@lbach lbach commented Mar 5, 2019

Compare two time.struct_time objects instead of an int and a
time.struct_time, which will throw a TypeError exception.

This fixes #84

This pull request is to #85 but this is an effort to revive the fix.

Compare two time.struct_time objects instead of an int and a
time.struct_time, which will throw a TypeError exception.
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Mar 5, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lbach
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: roycaihw

If they are not already assigned, you can assign the PR to them by writing /assign @roycaihw in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 5, 2019
@lbach
Copy link
Author

lbach commented Mar 5, 2019

I am having some issues registering with LinuxFoundation. I'll have the CLA signed once I get my account with LinuxFoundation sorted.

@codecov-io
Copy link

Codecov Report

Merging #121 into master will decrease coverage by 0.22%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   92.11%   91.88%   -0.23%     
==========================================
  Files          13       13              
  Lines        1217     1220       +3     
==========================================
  Hits         1121     1121              
- Misses         96       99       +3
Impacted Files Coverage Δ
config/kube_config.py 83.07% <0%> (-0.79%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e077f88...ec02f0c. Read the comment docs.

@richard-reece
Copy link

richard-reece commented Mar 15, 2019

There's an issue with this patch. When applied, I get the following exception:

  File ".../site-packages/kubernetes/config/kube_config.py", line 215, in _load_azure_token
    expires_on_ts = int(provider['config']['expires-on'])
ValueError: invalid literal for int() with base 10: '2019-03-15 13:36:54.245657'

@rabidang3ls
Copy link

rabidang3ls commented May 10, 2019

I'm new at this whole github contributing thing, but if you change the _load_azure_token(self, provider) function to the following, it fixes the ValueError: invalid literal for int() with base 10:

def _load_azure_token(self, provider):
    if 'config' not in provider:
        return
    if 'access-token' not in provider['config']:
        return
    if 'expires-on' in provider['config']:
        try:
             if time.gmtime(int(provider['config']['expires-on'])) < time.gmtime():
                self._refresh_azure_token(provider['config'])
        except ValueError:
            if time.strptime(provider['config']['expires-on'], '%Y-%m-%d %H:%M:%S.%f') < time.gmtime():
                self._refresh_azure_token(provider['config'])
    self.token = 'Bearer %s' % provider['config']['access-token']
    return self.token

Credit to @fatal-exception in #84

@mtougeron
Copy link

@lbach Any chance this can be finished? It'd be a huge help to us to have this working. Thanks!

@roycaihw
Copy link
Member

roycaihw commented Jul 9, 2019

#84 gets fixed by #141. Closing this in favor of that one

@roycaihw roycaihw closed this Jul 9, 2019
@ellieayla
Copy link

ellieayla commented Feb 11, 2020

Verified this is broken in kubernetes==10.0.1 and working in prerelease kubernetes==11.0.0b2; looking forward to a GA release of 11.

@k8s-ci-robot
Copy link
Contributor

@lbach: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2020
@roycaihw
Copy link
Member

Verified this is broken in kubernetes==10.0.1 and working in prerelease kubernetes==11.0.0b2; looking forward to a GA release of 11.

Thanks for the verification! We are working on releasing 11.0.0 GA. I added an item in the bi-weekly meeting agenda. cc @palnabarun

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
8 participants