charmed-kubernetes / actions-operator

Apache License 2.0
5 stars 18 forks source link

Initialize microk8s: Error from server (Forbidden): selfsubjectaccessreviews.authorization.k8s.io is forbidden: User "me" cannot create resource "selfsubjectaccessreviews" in API group "authorization.k8s.io" at the cluster scope #51

Closed taurus-forever closed 6 months ago

taurus-forever commented 1 year ago

Hi,

We have noticed the following error in our repos, e.g. mysql-k8s:

Run charmed-kubernetes/actions-operator@main
...
Initialize microk8s
...
  /usr/bin/sg microk8s -c microk8s kubectl auth can-i create pods --as=me
  Error from server (Forbidden): selfsubjectaccessreviews.authorization.k8s.io is forbidden: User "me" cannot create resource "selfsubjectaccessreviews" in API group "authorization.k8s.io" at the cluster scope

the same error reported for the current repository tests.

1) Should it be cleaned / fixed? 2) Why GH action continued and didn't report an error?

P.S. I am not able to reproduce the same issue locally using the same microk8s version:

> /usr/bin/sg microk8s -c "microk8s kubectl auth can-i create pods --as=me"
no

> microk8s version
MicroK8s v1.26.1 revision 4595

Tnx!

taurus-forever commented 1 year ago

The answer on 2 above: https://github.com/charmed-kubernetes/actions-operator/blob/3d3eb35d73c0f326d07880d83a99876d321f823c/src/bootstrap/index.ts#L109

We just check the exit code... and not "yes/no" value in reply.

The interesting part: it is sometimes "no" and sometimes "Error" in our tests...

lucabello commented 1 year ago

@addyess could you clarify the purpose of that line? In our use cases that assertion seems to always be wrong (meaning microk8s kubectl auth can-i create pods --as=me always succeeds). What's the expected behavior and why? If it's just a bug, could we remove the line altogether since it doesn't really stop the action anyway?

addyess commented 1 year ago

@lucabello and @taurus-forever looking at this a year after having put it there, i don't recall the context for it being this way. @stonepreston you reviewed this PR with me but i don't recall a reason. It likely has something to do with waiting for microk8s to be ready to create pods for the test -- but i can't say i remember why it's there.

You're both right in that it's simply left adding setup time to the tests and providing no other value other than filler. Hopefully that filler time isn't necessary to someone

lucabello commented 6 months ago

Hey @addyess, that filler time is starting to impact us more and more :) It looks like an extremely simple change to make, can we make it happen?

addyess commented 6 months ago

Good news, @lucabello ! I still don't know why it's there. What if we add an input to the actions operator to allow your jobs to skip it? Is that something you can contribute?

I don't want to remove it b/c then we'll have new issues show up asking why it's gone.

lucabello commented 6 months ago

I can indeed make a contribution, but I doubt people will open issues to ask why an error is gone if we remove that line.

As it is right now, that piece of code doesn't change the execution flow at all. It just displays 12 error messages, then skips that check. If this check failing meant something happened (e.g., the job was cancelled) I would agree with you. However, the job flow is the exact same; there is no "follow-up" action or change that a person noticing that error would have to take.

Removing that line means that:

I can open a PR removing that line, but adding an input to choose if you want to display or not an extra log message (because that's what that line does) seems like a bad idea to me.

DanielArndt commented 6 months ago

related: #59