Add Security Group support #65
Conversation
…sricardor/region into feature/add-security-group-crd
pkg/apis/unikorn/v1alpha1/types.go
Outdated
| // SecurityGroupID is the security group ID. | ||
| SecurityGroupID *string `json:"securityGroupID,omitempty"` | ||
| // Rules are the security group rules. | ||
| Rules []rules.SecGroupRule `json:"rules,omitempty"` |
There was a problem hiding this comment.
I don't think we should be importing/exposing gophercloud types, there's jsut way too much unnecessary crap in there.
There was a problem hiding this comment.
😮 I updated it but forgot to push. The gophercloud type is missing the JSON tag on a few elements, which is causing the CRD generation to fail. I'll fix that - thanks for the review so far! ;)
There was a problem hiding this comment.
I'm trying to keep on top of it! It's a pain if you're having to use replace in go.mod too often... here's something I find useful:
#!/bin/bash
export GOPRIVATE=github.com/unikorn-cloud
go mod edit -replace $1=$1@$2
go mod tidy
There was a problem hiding this comment.
On the whole, I'm seriously impressed you have got this far! See the comments below, I think that perhaps the data structures and APIs aren't quite correct and that leads to algorithmic complexity which makes my COVID infested head hurt! Let me know your thoughts...
pkg/apis/unikorn/v1alpha1/types.go
Outdated
| // Port is the port or range of ports. | ||
| Port SecurityGroupRulePort `json:"port"` | ||
| // Cidr is the CIDR block to allow traffic from. | ||
| Cidr *unikornv1core.IPv4Prefix `json:"cidr"` |
There was a problem hiding this comment.
Variable should be capitalized, surprised the linter didn't complain!
pkg/providers/openstack/provider.go
Outdated
| if rule.Port.Range != nil { | ||
| return rule.Port.Range.Start, rule.Port.Range.End | ||
| } | ||
| return 0, 0 // Return 0,0 if no port information is available |
There was a problem hiding this comment.
There's a couple things we should do here
- Return an error, not magic numbers
- In the CRD, you can use CEL to indicate that at least one is required...
// +kubebuilder:validation:XValidation:message="at least one of number or range must be defined",rule=(has(self.number) || has(self.range))
type Thing struct {
Number *int
Range *Range
}For more information see:
And feel EMPOWERED!!
pkg/apis/unikorn/v1alpha1/types.go
Outdated
| Range *SecurityGroupRulePortRange `json:"range,omitempty"` | ||
| } | ||
|
|
||
| type SecurityGroupRule struct { |
There was a problem hiding this comment.
I think the best thing we can do here for general cleanliness and ease of use is to have each rule have a unique identifier and also a description e.g.
kind: SecurityGroup
spec:
ingress:
- id: 9575b2ef-2eee-4c75-a6e8-59537984185a
description: OpenSSH
port:
number: 22The description is just to make the UX a lot better, sure Adam will thank you in future!
The ID however can be used in the mapping from unikorn to openstack e.g.
kind: OpenstackSecurityGroup
spec:
id: 13a4f72a-9467-4262-be73-021d44541cc8
rules:
- id: 9575b2ef-2eee-4c75-a6e8-59537984185a
openstackID: f7fc698a-628a-4652-8355-9d3b69c9b120I think this will improve the reconcile logic a whole heap as you're just concerned with IDs and not the whole tuple of direction, port range, protocol etc etc. Obviously the API will need to be tweaked to be:
GET/POST /.../securitygroups
GET?PUT/DELETE /.../securitygroups/{id}
GET/POST /.../securitygroups/{id}/rules
GET/PUT/DELETE /.../securitygroups/{id}/rules/{id}
A call to create a rule will generate a unique UUID like creation of the security group itself. I am aware of time, so rather than have rules as a separate CRD and controller we leave that bit the way it is.
As I always say, if the data structure is not quite right, the code will suck!
There was a problem hiding this comment.
Thanks for the suggestions! Implementing a unique identifier for each rule did help simplify the reconciliation logic somewhat, but splitting the CRDs made a bigger difference. I ended up splitting the CRDs and moving the security group reconciliation to its own controller, which eliminated the need to iterate over the list of rules entirely.
|
|
|
Actually, there are still a shit load of files to review if I "viewed" the generated/boilerplate ones... I'll just test the behaviour and see what happens. That'll be quickly reduce the cognitive load further... |
|
First test... Created okay... returns an object, so we can assume auditing is working... API list works, objects look in order... Note The API doesn't actually return the identity the resource is linked to (and neither does the physical network for that matter, I shall swing back around and work out how to do this later. OpenStack looks happy. I can confirm that these rules are in place implicitly via defaults... on to rules... |
This doesn't return any defaults, good good! Oh dear a crash... So that should have resulted in an error from OpenAPI because the spec is missing... once fixed we get... Trying the following.... looking good... ah yeah... Now let's try delete the security group and see what happens 👿 ... beautiful so far... |
|
To this point I needed to:
Here's the gist so far... https://gist.github.com/spjmurray/edc1d1036b5f4083a960143409092ad1 |
There was a problem hiding this comment.
See the earlier comments from my acceptance testing, apply that patch and I'll continue onward...
One thing I will say is while the deleting the security group does cascade and delete the rules, you may want to consider my next test which will be deleting the entire identity and my expectation that the security groups will also disappear 👿 Now you appear to have handled the owner reference bit (cascading deletion), but the problem will be race conditions. If the user/project is deleted before the security group, then it'll get very stuck. See how physical networks are handled in pkg/provisioners/managers/identity/provisioner.go.
Also, f***ing great work, I've only got the 4 big files left to review, and they look (superficially) rather nice to gaze at!
|
Thanks will get on it first thing tomorrow, got a bit bogged down today, there's a merge conflict with the schema you need to sort, I did an upgrade of the tooling, so basically just delete the schema file and run |
|
Okay, add this and you'll get a 👍🏻 Seems good enough for now! diff --git a/charts/region/templates/identity-controller/clusterrole.yaml b/charts/region/templates/identity-controller/clusterrole.yaml
index 65b7fb1..9987ccf 100644
--- a/charts/region/templates/identity-controller/clusterrole.yaml
+++ b/charts/region/templates/identity-controller/clusterrole.yaml
@@ -37,6 +37,7 @@ rules:
resources:
- quotas
- physicalnetworks
+ - securitygroups
verbs:
- list
- watch |
This PR introduces the SecurityGroup CRD, the provider-specific data structure, security-group controller and the api