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

fix(caching): K8s cache affected when not enough permissions in service account #5742

Merged

Conversation

danielGz
Copy link
Contributor

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.

… 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 =
Copy link
Member

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

Copy link
Contributor Author

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

@link108 link108 merged commit 632fc6a into spinnaker:master Jun 24, 2022
@link108 link108 deleted the fix/k8s-caching-interrupts-on-forbidden branch June 24, 2022 01:25
@danielGz
Copy link
Contributor Author

@Mergifyio backport release-1.28.x release-1.27.x release-1.26.x

@mergify
Copy link
Contributor

mergify bot commented Jun 28, 2022

backport release-1.28.x release-1.27.x release-1.26.x

✅ Backports have been created

link108 pushed a commit that referenced this pull request Jul 7, 2022
…ce account (backport #5742) (#5746)

Co-authored-by: danielGz <dan.gz.are@gmail.com>
link108 pushed a commit that referenced this pull request Jul 7, 2022
…ce account (backport #5742) (#5745)

Co-authored-by: danielGz <dan.gz.are@gmail.com>
link108 added a commit that referenced this pull request Jul 7, 2022
…ce account (backport #5742) (#5744)

Co-authored-by: danielGz <dan.gz.are@gmail.com>
Co-authored-by: Cameron Motevasselani <cmotevasselani@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants