Repository navigation
Fix for Syself tests by cluster admin bound to system:masters group to run tests - #1292
Conversation
79b0448 to
c711ac7
Compare
|
@ibnjunaid Thanks for the contribution! This is some food for thought. I think I would like to extract the bulk of the code into some kind of strategy class or something -- I can explain this later. For now, let me ask this. I was told that Hydrophone wouldn't have the same problem (and if that was true, I would be willing to switch to Hydrophone), but when I tested Hydrophone just now, the dreaded problem did in fact occur. So is it false that Hydrophone doesn't have the problem, or do I need to invoke it in a certain way? |
|
@mbuechse I believe that Hydrophone does it in a similar way then. Changing to it would certainly have the benefit of using the tooling that stems directly from the kubernetes-sigs. But that would be another topic. I think here the point is still that nobody defined precisely which permissions you are supposed to have, and there is more than good reason to believe that an "admin" in Kubernetes has the system:masters group: https://kubernetes.io/docs/reference/access-authn-authz/rbac/#user-facing-roles Sorry, I believe that we forgot to put this link here earlier. You can see the definition of a ClusterAdmin, and you can compare that also with the test that is labeled with "[Privileged:ClusterAdmin]". However, the current test suite doesn't run it as ClusterAdmin. Apparently nobody cared about this mismatch until now, but now we do 😅 |
|
@janiskemper Sounds reasonable. Personally, I'm willing to accept this change (with minor alterations), but I would ask you to try and bring the issue regarding this mismatch upstream. |
|
@mbuechse Thanks for checking Hydrophone. Your result makes sense, and I think we should explain the credentials involved more clearly. When we start Sonobuoy or Hydrophone with a kubeconfig, that kubeconfig allows the tool to access the cluster and start the test pod. It does not automatically mean that the tests inside that pod use the same credentials. Hydrophone creates a separate ServiceAccount for the test pod and grants it broad RBAC permissions. The tests then access the Kubernetes API as that ServiceAccount. So even if we start Hydrophone with a kubeconfig belonging to system:masters, the tests themselves do not run with that group membership. The code shows where Hydrophone creates the ServiceAccount, grants its permissions and assigns it to the test pod: This matters because RBAC and admission policies are separate checks. RBAC can allow a request, while an admission policy can still reject it. Giving the test ServiceAccount the cluster-admin role therefore does not necessarily give it the same access as the supplied kubeconfig. In our case, Cluster API uses workload-cluster management credentials belonging to system:masters. Its kubeconfig generation code sets that group explicitly: Our management controllers need that access to reconcile, upgrade and repair the workload clusters. We have admission protections intended to prevent customer configuration from blocking those operations. These protections exempt the management identity, but still apply to the test ServiceAccount. To clarify our earlier wording: the ServiceAccount does have cluster-admin RBAC permissions. It simply does not receive the same admission exemption. The proposed option lets the tests use the supplied kubeconfig directly, which is why changing the runner alone would not solve the problem. The failing test makes the distinction more concrete. It registers two webhooks: one tries to block deletion of webhook configurations, and the other tries to add a label to them. It then creates and deletes dummy webhook configurations and checks that neither webhook interferes. The required Kubernetes behavior is that admission webhooks must not be invoked for operations on webhook configuration objects. The test description and implementation are here: Running this test requires sufficient access to create those configurations and perform the operations being tested. Its [Privileged:ClusterAdmin] label identifies the need for administrative access. It does not specify a particular authenticated user, ServiceAccount or group membership. This is the point I think matters for the conformance discussion: where is the required test identity defined? The CNCF instructions define the suite through the upstream [Conformance] tests and require all of them to run. They list Sonobuoy and Hydrophone as execution tools, but do not prescribe a ServiceAccount, require RBAC-only credentials or prohibit supplying an administrative kubeconfig: The upstream conformance documentation instead tells users to provide a kubeconfig with appropriate credentials. It also explicitly distinguishes the Kubernetes behavior being verified from the infrastructure needed to set up and clean up a test: The implementation supports that choice. The official conformance container passes KUBECONFIG to the e2e binary through --kubeconfig. The framework falls back to in-cluster credentials when no explicit kubeconfig or host is supplied: This PR uses that existing upstream mechanism. We do not modify the conformance image, test code or assertions. We do not skip conformance tests or disable our platform protections. We pass the supplied kubeconfig into the test pod so the tests themselves can use it. Treating the helper’s default ServiceAccount as mandatory would therefore add an identity requirement that I cannot find in either the conformance instructions or the definition of this test. The fact that both helpers choose a similar default does not establish that requirement. Actually I'm also surprised that this definition apparently does not exist upstream... The execution must still verify the behavior specified by the tests, and users must be able to reproduce the submitted results. The latter is an explicit CNCF requirement, so the necessary credentials and execution steps need to be documented: If a specific identity or admission-policy scope is required beyond that, it would be useful to identify the upstream requirement. Otherwise, I think this should be assessed as an option for supplying the credentials needed to run the unchanged suite, rather than as a departure from conformance simply because it differs from the runner’s default. Extracting the implementation into a strategy, as you suggested, sounds reasonable. The capability we need is to pass the supplied kubeconfig through to the actual e2e process, while keeping the current behavior as the default. |
|
@batistein Thanks for the clarification. Almost a little dissertation :) |
|
@mbuechse I think that this depends a bit. The tests currently fail and there is no reason that we found, other than unclear communication about kubernetes conformance. I also believe that there will be no short-term solution from upstream that really helps us here. So I believe we should do the two things together. Bring this to upstream to make them clear this up, and do this. |
|
@janiskemper Oh, maybe you got me wrong. I didn't mean to stall this. We would do this now, but I would like to see some upstream support so we can one day (sooner rather than later) remove some of the cumbersome logic. |
|
absolutely! |
mbuechse
left a comment
mbuechse
left a comment
There was a problem hiding this comment.
I have suggestions regarding the concrete implementation, mostly details. I'm afraid you have to rebase on main, and that won't be the most joyful experience. Alternatively, you create a new branch from (current!) main and re-enact your change, however incorporating my suggestions as you go. That could work as well.
c711ac7 to
db6f868
Compare
mbuechse
left a comment
mbuechse
left a comment
There was a problem hiding this comment.
Thanks for the great job! I have a few minor things, but it looks like we can merge this soon.
db6f868 to
c31a979
Compare
…est pods Signed-off-by: ibnjunaid <osama.junaid@syself.com>
c31a979 to
47fae2c
Compare
Fix multiple blank lines Co-authored-by: Matthias Büchse <m43+github@posteo.de> Signed-off-by: Osama Bin Junaid <32925504+ibnjunaid@users.noreply.github.com>
mbuechse
left a comment
mbuechse
left a comment
There was a problem hiding this comment.
Thank you very much for your contribution and the smooth collab!
4 participants
The conformance tests for Syself fail in 1.36 because of the following test.
[sig-api-machinery] AdmissionWebhook [Privileged:ClusterAdmin] [It] should not be able to mutate or prevent deletion of webhook configuration objects [Conformance]Starting from 1.36 Syself platform disallows operations on AdmissionWebhooks APIs for all except cluster admins in the Kubernetes definition, i.e. those belonging to
system:mastersgroup.Since we provide dedicated control planes to our users at Syself which they have full control over, it becomes important for us to prevent users from accidentally removing platform's access to the cluster. Syself platform continously reconciles and self heals the cluster so it is very important that the platform doesnt loose access to the cluster. This change was made to make this less likely to happen.
We propose to implement the above choice by using a different kubeconfig for the tests.
Sonobuoy's standard kubeconfig has admin privileges, but it not part
system:mastersgroup.Since Syself has reasons for its change and the official Kubernetes documentation highlights the importance of the
system:mastersgroup, the required change would just be for Syself, without necessarily changing it for everyone.This is not a question of conformance, but rather of implementation details. Therefore, a change in the kubeconfig of sonobuoy should be everything that is needed to make the tests pass.