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

Add Security Group support #65

Merged
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
---
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.16.1
name: openstacksecuritygroups.region.unikorn-cloud.org
spec:
group: region.unikorn-cloud.org
names:
categories:
- unikorn
kind: OpenstackSecurityGroup
listKind: OpenstackSecurityGroupList
plural: openstacksecuritygroups
singular: openstacksecuritygroup
scope: Namespaced
versions:
- additionalPrinterColumns:
- jsonPath: .metadata.creationTimestamp
name: age
type: date
name: v1alpha1
schema:
openAPIV3Schema:
description: OpenstackSecurityGroup has no controller, its a database record
of state.
properties:
apiVersion:
description: |-
APIVersion defines the versioned schema of this representation of an object.
Servers should convert recognized schemas to the latest internal value, and
may reject unrecognized values.
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources
type: string
kind:
description: |-
Kind is a string value representing the REST resource this object represents.
Servers may infer this from the endpoint the client submits requests to.
Cannot be updated.
In CamelCase.
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
type: string
metadata:
type: object
spec:
properties:
securityGroupID:
description: SecurityGroupID is the security group ID.
type: string
type: object
status:
type: object
required:
- spec
type: object
served: true
storage: true
subresources: {}
174 changes: 174 additions & 0 deletions charts/region/crds/region.unikorn-cloud.org_securitygroups.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
---
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.16.1
name: securitygroups.region.unikorn-cloud.org
spec:
group: region.unikorn-cloud.org
names:
categories:
- unikorn
kind: SecurityGroup
listKind: SecurityGroupList
plural: securitygroups
singular: securitygroup
scope: Namespaced
versions:
- additionalPrinterColumns:
- jsonPath: .status.conditions[?(@.type=="Available")].reason
name: status
type: string
- jsonPath: .metadata.creationTimestamp
name: age
type: date
name: v1alpha1
schema:
openAPIV3Schema:
description: SecurityGroup defines a security group beloning to an identity.
properties:
apiVersion:
description: |-
APIVersion defines the versioned schema of this representation of an object.
Servers should convert recognized schemas to the latest internal value, and
may reject unrecognized values.
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources
type: string
kind:
description: |-
Kind is a string value representing the REST resource this object represents.
Servers may infer this from the endpoint the client submits requests to.
Cannot be updated.
In CamelCase.
More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds
type: string
metadata:
type: object
spec:
properties:
ingress:
description: Ingress are the ingress rules.
items:
properties:
port:
description: Port is the port or range of ports.
properties:
number:
description: Number is the port number.
type: integer
range:
description: Range is the port range.
properties:
end:
description: End is the end of the range.
maximum: 65535
type: integer
start:
description: Start is the start of the range.
minimum: 1
type: integer
required:
- end
- start
type: object
required:
- number
- range
type: object
protocol:
description: Protocol is the protocol of the rule.
enum:
- tcp
- udp
type: string
required:
- port
- protocol
type: object
type: array
pause:
description: Pause, if true, will inhibit reconciliation.
type: boolean
provider:
description: Provider defines the provider type.
enum:
- openstack
type: string
tags:
description: |-
Tags are an abitrary list of key/value pairs that a client
may populate to store metadata for the resource.
items:
description: Tag is an arbirary key/value.
properties:
name:
description: Name of the tag.
type: string
value:
description: Value of the tag.
type: string
required:
- name
- value
type: object
type: array
required:
- provider
type: object
status:
properties:
conditions:
description: Current service state of a cluster manager.
items:
description: |-
Condition is a generic condition type for use across all resource types.
It's generic so that the underlying controller-manager functionality can
be shared across all resources.
properties:
lastTransitionTime:
description: Last time the condition transitioned from one status
to another.
format: date-time
type: string
message:
description: Human-readable message indicating details about
last transition.
type: string
reason:
description: Unique, one-word, CamelCase reason for the condition's
last transition.
enum:
- Provisioning
- Provisioned
- Cancelled
- Errored
- Deprovisioning
- Deprovisioned
type: string
status:
description: |-
Status is the status of the condition.
Can be True, False, Unknown.
type: string
type:
description: Type is the type of the condition.
enum:
- Available
type: string
required:
- lastTransitionTime
- message
- reason
- status
- type
type: object
type: array
type: object
required:
- spec
type: object
served: true
storage: true
subresources:
status: {}
24 changes: 24 additions & 0 deletions pkg/apis/unikorn/v1alpha1/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,27 @@ func (c *PhysicalNetwork) StatusConditionWrite(t unikornv1core.ConditionType, st
func (c *PhysicalNetwork) ResourceLabels() (labels.Set, error) {
return nil, nil
}

// Paused implements the ReconcilePauser interface.
func (c *SecurityGroup) Paused() bool {
return c.Spec.Pause
}

// StatusConditionRead scans the status conditions for an existing condition whose type
// matches.
func (c *SecurityGroup) StatusConditionRead(t unikornv1core.ConditionType) (*unikornv1core.Condition, error) {
return unikornv1core.GetCondition(c.Status.Conditions, t)
}

// StatusConditionWrite either adds or updates a condition in the cluster manager status.
// If the condition, status and message match an existing condition the update is
// ignored.
func (c *SecurityGroup) StatusConditionWrite(t unikornv1core.ConditionType, status corev1.ConditionStatus, reason unikornv1core.ConditionReason, message string) {
unikornv1core.UpdateCondition(&c.Status.Conditions, t, status, reason, message)
}

// ResourceLabels generates a set of labels to uniquely identify the resource
// if it were to be placed in a single global namespace.
func (c *SecurityGroup) ResourceLabels() (labels.Set, error) {
return nil, nil
}
2 changes: 2 additions & 0 deletions pkg/apis/unikorn/v1alpha1/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ func init() {
SchemeBuilder.Register(&OpenstackIdentity{}, &OpenstackIdentityList{})
SchemeBuilder.Register(&OpenstackPhysicalNetwork{}, &OpenstackPhysicalNetworkList{})
SchemeBuilder.Register(&VLANAllocation{}, &VLANAllocationList{})
SchemeBuilder.Register(&SecurityGroup{}, &SecurityGroupList{})
SchemeBuilder.Register(&OpenstackSecurityGroup{}, &OpenstackSecurityGroupList{})
}

// Resource maps a resource type to a group resource.
Expand Down
101 changes: 101 additions & 0 deletions pkg/apis/unikorn/v1alpha1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ limitations under the License.
package v1alpha1

import (
"github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules"
unikornv1core "github.com/unikorn-cloud/core/pkg/apis/unikorn/v1alpha1"

"k8s.io/apimachinery/pkg/api/resource"
Expand Down Expand Up @@ -517,3 +518,103 @@ type FlavorQuota struct {

type QuotaStatus struct {
}

// SecurityGroupList is a typed list of security groups.
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
type SecurityGroupList struct {
metav1.TypeMeta `json:",inline"`
metav1.ListMeta `json:"metadata,omitempty"`
Items []SecurityGroup `json:"items"`
}

// SecurityGroup defines a security group beloning to an identity.
// +genclient
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
// +kubebuilder:resource:scope=Namespaced,categories=unikorn
// +kubebuilder:subresource:status
// +kubebuilder:printcolumn:name="status",type="string",JSONPath=".status.conditions[?(@.type==\"Available\")].reason"
// +kubebuilder:printcolumn:name="age",type="date",JSONPath=".metadata.creationTimestamp"
type SecurityGroup struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`
Spec SecurityGroupSpec `json:"spec"`
Status SecurityGroupStatus `json:"status,omitempty"`
}

type SecurityGroupSpec struct {
// Pause, if true, will inhibit reconciliation.
Pause bool `json:"pause,omitempty"`
// Tags are an abitrary list of key/value pairs that a client
// may populate to store metadata for the resource.
Tags TagList `json:"tags,omitempty"`
// Provider defines the provider type.
Provider Provider `json:"provider"`
// Ingress are the ingress rules.
Ingress []SecurityGroupRule `json:"ingress,omitempty"`
}

// +kubebuilder:validation:Enum=tcp;udp
type SecurityGroupRuleProtocol string

const (
TCP SecurityGroupRuleProtocol = "tcp"
UDP SecurityGroupRuleProtocol = "udp"
)

type SecurityGroupRulePortRange struct {
// Start is the start of the range.
// +kubebuilder:validation:Minimum=1
Start int `json:"start"`
// End is the end of the range.
// +kubebuilder:validation:Maximum=65535
End int `json:"end"`
}

type SecurityGroupRulePort struct {
// Number is the port number.
Number *int `json:"number"`
// Range is the port range.
Range *SecurityGroupRulePortRange `json:"range"`
}

type SecurityGroupRule struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: 22

The 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-9d3b69c9b120

I 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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Protocol is the protocol of the rule.
Protocol SecurityGroupRuleProtocol `json:"protocol"`
// Port is the port or range of ports.
Port SecurityGroupRulePort `json:"port"`
}

type SecurityGroupStatus struct {
// Current service state of a cluster manager.
Conditions []unikornv1core.Condition `json:"conditions,omitempty"`
}

// OpenstackSecurityGroupList is a typed list of security groups.
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
type OpenstackSecurityGroupList struct {
metav1.TypeMeta `json:",inline"`
metav1.ListMeta `json:"metadata,omitempty"`
Items []OpenstackSecurityGroup `json:"items"`
}

// OpenstackSecurityGroup has no controller, its a database record of state.
// +genclient
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
// +kubebuilder:resource:scope=Namespaced,categories=unikorn
// +kubebuilder:printcolumn:name="age",type="date",JSONPath=".metadata.creationTimestamp"
type OpenstackSecurityGroup struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`
Spec OpenstackSecurityGroupSpec `json:"spec"`
Status OpenstackSecurityGroupStatus `json:"status,omitempty"`
}

type OpenstackSecurityGroupSpec struct {
// SecurityGroupID is the security group ID.
SecurityGroupID *string `json:"securityGroupID,omitempty"`
// Rules are the security group rules.
Rules []rules.SecGroupRule `json:"rules,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be importing/exposing gophercloud types, there's jsut way too much unnecessary crap in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😮 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! ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

type OpenstackSecurityGroupStatus struct {
}
Loading
Loading