Skip to content

Commit ce274dc

Browse files
nkbeastnkbeast
committed
fix(leaderelection): survive malformed API error bodies and lock annotations
try_acquire_or_renew() parsed the raw error body with json.loads and indexing straight into it, on the assumption that whatever came back is a Kubernetes Status object. Anything sitting in front of the API server (in an ingress, load balancer or proxy) happily answers with an HTML error page, an empty payload or some other non-JSON body, and ApiException.body can also be None. Each of those raised out of the election loop and took the whole leader election down, which is the one failure mode this code exists to prevent - a controller that stops renewing its lease without ever calling onstopped_leading leaves the workload in limbo until an operator notices. Treat an unparsable or missing error body as 'not a 404' and retry on the next period, in both the sync and the aio elector (the aio one also crashed on an empty body through an assert). The same class of problem existed on the read path of the ConfigMap lock: a corrupted leader-election annotation raised out of get() and killed the elector. Treat a non-JSON annotation like a missing one so the next update rewrites a clean record. Signed-off-by: NK <nk@localhost.localdomain> Signed-off-by: NK <92711184+nkbeast@users.noreply.github.com>
1 parent e48904a commit ce274dc

6 files changed

Lines changed: 163 additions & 9 deletions

File tree

‎kubernetes/aio/leaderelection/leaderelection.py‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -145,11 +145,22 @@ async def try_acquire_or_renew(self) -> bool:
145145

146146
# A lock is not created with that name, try to create one
147147
if not lock_status:
148-
assert (
148+
# The error body comes straight from the API server, but anything
149+
# sitting in front of it (ingress, load balancer, proxy) can answer
150+
# with an HTML page, an empty payload or some other non-JSON body.
151+
# Only a clean 404 means the lock is absent and may be created;
152+
# everything else is retried on the next period instead of taking
153+
# the whole leader election down.
154+
error_code = None
155+
if (
149156
isinstance(old_election_record, ApiException)
150157
and old_election_record.body is not None
151-
)
152-
if json.loads(old_election_record.body)["code"] != HTTPStatus.NOT_FOUND:
158+
):
159+
try:
160+
error_code = json.loads(old_election_record.body)["code"]
161+
except (ValueError, TypeError, KeyError, AttributeError):
162+
error_code = None
163+
if error_code != HTTPStatus.NOT_FOUND:
153164
logger.error(
154165
"Error retrieving resource lock %s as %s",
155166
self.election_config.lock.name,

‎kubernetes/aio/leaderelection/leaderelection_test.py‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,5 +365,38 @@ async def update(
365365
self.lock.release()
366366

367367

368+
def test_acquire_survives_non_json_error_body(self):
369+
"""A proxy answering with an HTML error page must not kill the elector."""
370+
371+
class GatewayErrorLock:
372+
def __init__(self):
373+
self.name = "lock"
374+
self.namespace = "ns"
375+
self.identity = "candidate"
376+
377+
async def get(self, name, namespace):
378+
return False, ApiException(
379+
status=502,
380+
reason="Bad Gateway",
381+
body="<html><body>502 Bad Gateway</body></html>",
382+
)
383+
384+
async def create(self, name, namespace, election_record):
385+
return False
386+
387+
config = electionconfig.Config(
388+
lock=GatewayErrorLock(),
389+
lease_duration=4,
390+
renew_deadline=3,
391+
retry_period=1,
392+
onstarted_leading=lambda: None,
393+
onstopped_leading=lambda: None,
394+
)
395+
396+
elector = leaderelection.LeaderElection(config)
397+
result = asyncio.run(elector.try_acquire_or_renew())
398+
self.assertFalse(result)
399+
400+
368401
if __name__ == "__main__":
369402
unittest.main()

‎kubernetes/aio/leaderelection/resourcelock/configmaplock.py‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,26 @@ async def get(
8080
self.configmap_reference = api_response
8181
return True, None
8282

83-
lock_record = self.get_lock_object(
84-
json.loads(annotations[self.leader_electionrecord_annotationkey])
85-
)
83+
# A corrupted annotation must not take the elector down: treat it
84+
# like a missing one so the next update rewrites a clean record.
85+
try:
86+
annotation_record = json.loads(
87+
annotations[self.leader_electionrecord_annotationkey]
88+
)
89+
except ValueError:
90+
logger.warning(
91+
"Leader election annotation on ConfigMap %s/%s is not valid "
92+
"JSON; treating the lock as unheld",
93+
name,
94+
namespace,
95+
)
96+
api_response.metadata.annotations = {
97+
self.leader_electionrecord_annotationkey: ""
98+
}
99+
self.configmap_reference = api_response
100+
return True, None
101+
102+
lock_record = self.get_lock_object(annotation_record)
86103

87104
self.configmap_reference = api_response
88105
return True, lock_record

‎kubernetes/leaderelection/leaderelection.py‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -130,8 +130,17 @@ def try_acquire_or_renew(self):
130130

131131
# A lock is not created with that name, try to create one
132132
if not lock_status:
133-
if json.loads(old_election_record.body)[
134-
'code'] != HTTPStatus.NOT_FOUND:
133+
# The error body comes straight from the API server, but anything
134+
# sitting in front of it (ingress, load balancer, proxy) can answer
135+
# with an HTML page, an empty payload or some other non-JSON body.
136+
# Only a clean 404 means the lock is absent and may be created;
137+
# everything else is retried on the next period instead of taking
138+
# the whole leader election down.
139+
try:
140+
error_code = json.loads(old_election_record.body)['code']
141+
except (ValueError, TypeError, KeyError, AttributeError):
142+
error_code = None
143+
if error_code != HTTPStatus.NOT_FOUND:
135144
logger.info(
136145
"Error retrieving resource lock {} as {}".format(
137146
self.election_config.lock.name,

‎kubernetes/leaderelection/leaderelection_test.py‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -321,5 +321,76 @@ def update(self, name, namespace, updated_record):
321321
self.lock.release()
322322

323323

324+
def test_acquire_survives_non_json_error_body(self):
325+
"""A proxy answering with an HTML error page must not kill the elector."""
326+
class GatewayErrorLock:
327+
def __init__(self):
328+
self.name = "lock"
329+
self.namespace = "ns"
330+
self.identity = "candidate"
331+
332+
def get(self, name, namespace):
333+
return False, ApiException(
334+
status=502, reason="Bad Gateway",
335+
body="<html><body>502 Bad Gateway</body></html>")
336+
337+
config = electionconfig.Config(
338+
lock=GatewayErrorLock(), lease_duration=4, renew_deadline=3,
339+
retry_period=1, onstarted_leading=lambda: None,
340+
onstopped_leading=lambda: None)
341+
342+
result = leaderelection.LeaderElection(config).try_acquire_or_renew()
343+
self.assertFalse(result)
344+
345+
def test_acquire_survives_empty_error_body(self):
346+
class EmptyErrorLock:
347+
def __init__(self):
348+
self.name = "lock"
349+
self.namespace = "ns"
350+
self.identity = "candidate"
351+
352+
def get(self, name, namespace):
353+
return False, ApiException(status=500, reason="Server Error",
354+
body=None)
355+
356+
config = electionconfig.Config(
357+
lock=EmptyErrorLock(), lease_duration=4, renew_deadline=3,
358+
retry_period=1, onstarted_leading=lambda: None,
359+
onstopped_leading=lambda: None)
360+
361+
result = leaderelection.LeaderElection(config).try_acquire_or_renew()
362+
self.assertFalse(result)
363+
364+
def test_acquire_still_creates_on_clean_404(self):
365+
class NotFoundLock:
366+
def __init__(self):
367+
self.name = "lock"
368+
self.namespace = "ns"
369+
self.identity = "candidate"
370+
self.created = False
371+
372+
def get(self, name, namespace):
373+
if self.created:
374+
return True, LeaderElectionRecord(
375+
"candidate", "4", "now", "now")
376+
return False, ApiException(
377+
status=404, reason="Not Found",
378+
body=json.dumps({'code': 404}))
379+
380+
def create(self, name, namespace, election_record):
381+
self.created = True
382+
return True
383+
384+
def update(self, name, namespace, updated_record):
385+
return True
386+
387+
config = electionconfig.Config(
388+
lock=NotFoundLock(), lease_duration=4, renew_deadline=3,
389+
retry_period=1, onstarted_leading=lambda: None,
390+
onstopped_leading=lambda: None)
391+
392+
self.assertTrue(leaderelection.LeaderElection(config).try_acquire_or_renew())
393+
394+
324395
if __name__ == '__main__':
325396
unittest.main()

‎kubernetes/leaderelection/resourcelock/configmaplock.py‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,20 @@ def get(self, name, namespace):
6464
self.configmap_reference = api_response
6565
return True, None
6666

67-
lock_record = self.get_lock_object(json.loads(annotations[self.leader_electionrecord_annotationkey]))
67+
# A corrupted annotation must not take the elector down: treat it
68+
# like a missing one so the next update rewrites a clean record.
69+
try:
70+
annotation_record = json.loads(
71+
annotations[self.leader_electionrecord_annotationkey])
72+
except ValueError:
73+
logger.warning(
74+
"Leader election annotation on ConfigMap {}/{} is not valid "
75+
"JSON; treating the lock as unheld".format(name, namespace))
76+
api_response.metadata.annotations = {self.leader_electionrecord_annotationkey: ''}
77+
self.configmap_reference = api_response
78+
return True, None
79+
80+
lock_record = self.get_lock_object(annotation_record)
6881

6982
self.configmap_reference = api_response
7083
return True, lock_record

0 commit comments

Comments
 (0)