-
Notifications
You must be signed in to change notification settings - Fork 1k
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(caching): K8s cache affected when not enough permissions in service account #5742
fix(caching): K8s cache affected when not enough permissions in service account #5742
Conversation
… not neough permissions in the service account exist but because we are throwing an error even partial entries are not being cached
@@ -453,8 +453,14 @@ public ImmutableList<KubernetesManifest> list( | |||
parseManifestList()); | |||
|
|||
if (status.getResult() != JobResult.Result.SUCCESS) { | |||
throw new KubectlException( | |||
"Failed to read " + kinds + " from " + namespace + ": " + status.getError()); | |||
boolean permissionError = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is using Apache Commons necessary? The next if block below just does a simple contains
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really I just wanted to minimize the risk in case the k8s message could change in a future version. aka from "forbidden" -> Forbidden/FORBIDDEN
@Mergifyio backport release-1.28.x release-1.27.x release-1.26.x |
✅ Backports have been created
|
k8s-kind listing is able to return partial results when not enough permissions in the service account exist but because we are throwing an error even partial entries are not being cached.
This fix will not throw an error when it is permission related only for k8s caching.