Repository navigation
CKS: allow the APIs of the Kubernetes provider in the project role - #14690
Open
adestis-mf wants to merge 1 commit into
Open
adestis-mf wants to merge 1 commit into
adestis-mf wants to merge 1 commit into
Conversation
The CloudStack Kubernetes Provider in a cluster in a project uses the API keys of the account that CKS creates in the project. The role of that account does not allow several APIs that the provider calls: - listCapabilities: since v1.3.0 the provider reads the CloudStack version with it when it starts, unless its cloud config sets the version. The cloud config that CKS writes does not. - listNetworkACLLists: since v1.2.0 the provider reads the ACL list of a VPC tier before it adds an ACL rule for a LoadBalancer Service. - updateNetworkACLItem: since v1.3.0 the provider takes over the ACL rules that earlier releases created. - listLBStickinessPolicies, createLBStickinessPolicy and deleteLBStickinessPolicy: since v1.3.0 the provider manages stickiness policies. - listVPCs: to find out whether the VPC of a tier has the Firewall service, which a VPC offering can include since 4.23. Allow them in the role.
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
1 open issue
What changed in this PR
Updates the “Project Kubernetes Service Role” permissions so the CloudStack Kubernetes Provider can call required CloudStack APIs, and adds a unit test to prevent regressions.
Changes:
- Add missing provider-required APIs (capabilities, VPCs, ACLs, stickiness policies) to the project Kubernetes service role allow-list.
- Add a unit test asserting the new APIs are allowed and the final deny-all rule remains last.
| File | Description |
|---|---|
| plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImpl.java | Extends the project Kubernetes role allow-list with additional API commands required by the provider. |
| plugins/integrations/kubernetes-service/src/test/java/com/cloud/kubernetes/cluster/KubernetesClusterManagerImplTest.java | Adds a regression test validating the role includes the required API permissions and ends with deny-all. |
Review effort: Lite
Comment on lines
+936
to
+949
| List<String> ruleStrings = new ArrayList<>(); | ||
| for (Rule rule : rules.getAllValues()) { | ||
| ruleStrings.add(rule.getRuleString()); | ||
| } | ||
|
|
||
| for (String api : Arrays.asList("listCapabilities", "listVPCs", "listNetworkACLLists", "updateNetworkACLItem", | ||
| "listLBStickinessPolicies", "createLBStickinessPolicy", "deleteLBStickinessPolicy")) { | ||
| int index = ruleStrings.indexOf(api); | ||
| Assert.assertTrue(api + " is not in the role: " + ruleStrings, index >= 0); | ||
| Assert.assertEquals(api, RolePermissionEntity.Permission.ALLOW, permissions.getAllValues().get(index)); | ||
| } | ||
| int last = ruleStrings.size() - 1; | ||
| Assert.assertEquals("*", ruleStrings.get(last)); | ||
| Assert.assertEquals(RolePermissionEntity.Permission.DENY, permissions.getAllValues().get(last)); |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
For a cluster in a project, the CloudStack Kubernetes Provider uses the API keys of the service account that CKS creates there. The role of that account (
Project Kubernetes Service Role) does not allow several APIs the provider calls:listCapabilities: the provider reads the CloudStack version with it when it starts, unless the cloud config setsversion, which the one written by CKS does not. Reading the code, provider v1.3.0 fails to initialize without it.listNetworkACLLists: needed before the provider adds an ACL rule for a LoadBalancer Service on a VPC tier (since provider v1.2.0).updateNetworkACLItem: takes over ACL rules of earlier provider releases (v1.3.0).listLBStickinessPolicies,createLBStickinessPolicy,deleteLBStickinessPolicy: stickiness policies (v1.3.0).listVPCs: to find out whether a VPC has the Firewall service (4.23 and later), see LoadBalancer services in a VPC with the Firewall service get no firewall rule cloudstack-kubernetes-provider#114.This PR adds them to the role. CKS creates the role only once, so existing installations have to add them by hand, before the rule that denies everything else.
A few colleagues and I worked on this, since CKS in VPCs with the Firewall service matters for our cloud environment.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
A new unit test checks that the role allows these APIs and still ends with the deny rule; it fails without the new entries.
mvn -pl plugins/integrations/kubernetes-service -am test: 129 tests pass, checkstyle clean.On a 4.23.0.0 installation, adding
listVPCsby hand to an existing role was enough for the provider change for apache/cloudstack-kubernetes-provider#114 to read the VPC; before, the call was refused with error 432.How did you try to break this feature and the system with this change?
Only roles created after this change get the new permissions. Existing roles are not changed, and the deny rule stays last.