Skip to content

Support configuration of security list management modes as LB service annotation #225

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Aug 2, 2018
Merged

Support configuration of security list management modes as LB service annotation #225

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
3056f4a
Support seclist management as lb service annotation
Jul 30, 2018
63e384e
Support configuration of security list management modes by specifying…
Jul 31, 2018
3aeb662
Support security list managment modes through annotation
Jul 31, 2018
7a753ae
modifying unit tests
Jul 31, 2018
2973f06
Modifying comments and typos
Aug 1, 2018
4d5e784
Improvements to deploy and rollback functions. (#224)
templecloud Aug 2, 2018
a1e8632
Support seclist management as lb service annotation
Jul 30, 2018
97727e2
Support configuration of security list management modes by specifying…
Jul 31, 2018
08542f0
Support security list managment modes through annotation
Jul 31, 2018
ddacd25
modifying unit tests
Jul 31, 2018
8ea9b48
Modifying comments and typos
Aug 1, 2018
feadb79
PR comments and documentation
Aug 2, 2018
47efdfd
PR comments and documentation
Aug 2, 2018
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
29 changes: 13 additions & 16 deletions pkg/oci/ccm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import (
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
wait "k8s.io/apimachinery/pkg/util/wait"
informers "k8s.io/client-go/informers"
informersv1 "k8s.io/client-go/informers/core/v1"
clientset "k8s.io/client-go/kubernetes"
listersv1 "k8s.io/client-go/listers/core/v1"
cache "k8s.io/client-go/tools/cache"
Expand Down Expand Up @@ -59,8 +58,8 @@ type CloudProvider struct {
client client.Interface
kubeclient clientset.Interface

securityListManager securityListManager
config *Config
securityListManagerFactory securityListManagerFactory
config *Config
}

// Compile time check that CloudProvider implements the cloudprovider.Interface
Expand Down Expand Up @@ -130,24 +129,22 @@ func (cp *CloudProvider) Initialize(clientBuilder controller.ControllerClientBui

nodeInformer := factory.Core().V1().Nodes()
go nodeInformer.Informer().Run(wait.NeverStop)
serviceInformer := factory.Core().V1().Services()
go serviceInformer.Informer().Run(wait.NeverStop)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to stick a \n under here to separate out the setup vs the cache sync

glog.Info("Waiting for node informer cache to sync")
if !cache.WaitForCacheSync(wait.NeverStop, nodeInformer.Informer().HasSynced) {
utilruntime.HandleError(fmt.Errorf("Timed out waiting for node informer to sync"))
if !cache.WaitForCacheSync(wait.NeverStop, nodeInformer.Informer().HasSynced, serviceInformer.Informer().HasSynced) {
utilruntime.HandleError(fmt.Errorf("Timed out waiting for informers to sync"))
}
cp.NodeLister = nodeInformer.Lister()

if !cp.config.LoadBalancer.Disabled {
var serviceInformer informersv1.ServiceInformer
if cp.config.LoadBalancer.SecurityListManagementMode != ManagementModeNone {
serviceInformer = factory.Core().V1().Services()
go serviceInformer.Informer().Run(wait.NeverStop)
glog.Info("Waiting for service informer cache to sync")
if !cache.WaitForCacheSync(wait.NeverStop, serviceInformer.Informer().HasSynced) {
utilruntime.HandleError(fmt.Errorf("Timed out waiting for service informer to sync"))
}
cp.securityListManagerFactory = func(mode string) securityListManager {
if cp.config.LoadBalancer.Disabled {
return newSecurityListManagerNOOP()
}

cp.securityListManager = newSecurityListManager(cp.client, serviceInformer, cp.config.LoadBalancer.SecurityLists, cp.config.LoadBalancer.SecurityListManagementMode)
if len(mode) == 0 {
mode = cp.config.LoadBalancer.SecurityListManagementMode
}
return newSecurityListManager(cp.client, serviceInformer, cp.config.LoadBalancer.SecurityLists, mode)
}
}

Expand Down
35 changes: 21 additions & 14 deletions pkg/oci/load_balancer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,18 @@ const (
ServiceAnnotationLoadBalancerSSLPorts = "service.beta.kubernetes.io/oci-load-balancer-ssl-ports"

// ServiceAnnotationLoadBalancerTLSSecret is a Service annotation for
// specifying the TLS secret ti install on the load balancer listeners which
// specifying the TLS secret to install on the load balancer listeners which
// have SSL enabled.
// See: https://kubernetes.io/docs/concepts/services-networking/ingress/#tls
ServiceAnnotationLoadBalancerTLSSecret = "service.beta.kubernetes.io/oci-load-balancer-tls-secret"

// ServiceAnnotationLoadBalancerConnectionIdleTimeout is the annotation used
// on the service to specify the idle connection timeout.
ServiceAnnotationLoadBalancerConnectionIdleTimeout = "service.beta.kubernetes.io/oci-load-balancer-connection-idle-timeout"

//ServiceAnnotaionLoadBalancerSecurityListManagementMode is a Service annotation for
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after //

//specifying the security list managment mode ("All","Frontend","None") that configures how security lists are managed by the CCM
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after // and ("All", "Frontend", "None")

ServiceAnnotaionLoadBalancerSecurityListManagementMode = "service.beta.kubernetes.io/oci-load-balancer-security-list-management-mode"
)

// DefaultLoadBalancerPolicy defines the default traffic policy for load
Expand Down Expand Up @@ -242,7 +246,7 @@ func (cp *CloudProvider) createLoadBalancer(ctx context.Context, spec *LBSpec) (
}

for _, ports := range spec.Ports {
if err = cp.securityListManager.Update(ctx, lbSubnets, nodeSubnets, spec.SourceCIDRs, nil, ports); err != nil {
if err = spec.SecurityListManager.Update(ctx, lbSubnets, nodeSubnets, spec.SourceCIDRs, nil, ports); err != nil {
return nil, err
}
}
Expand Down Expand Up @@ -305,7 +309,7 @@ func (cp *CloudProvider) EnsureLoadBalancer(ctx context.Context, clusterName str
ssl = NewSSLConfig(lbName, ports, cp)
}
subnets := []string{cp.config.LoadBalancer.Subnet1, cp.config.LoadBalancer.Subnet2}
spec, err := NewLBSpec(service, nodes, subnets, ssl)
spec, err := NewLBSpec(service, nodes, subnets, ssl, cp.securityListManagerFactory)
if err != nil {
glog.Errorf("Failed to derive LBSpec: %+v", err)
return nil, err
Expand Down Expand Up @@ -364,7 +368,7 @@ func (cp *CloudProvider) updateLoadBalancer(ctx context.Context, lb *loadbalance
for _, action := range actions {
switch a := action.(type) {
case *BackendSetAction:
err := cp.updateBackendSet(ctx, lbID, a, lbSubnets, nodeSubnets)
err := cp.updateBackendSet(ctx, lbID, a, lbSubnets, nodeSubnets, spec.SecurityListManager)
if err != nil {
return errors.Wrap(err, "updating BackendSet")
}
Expand All @@ -381,7 +385,7 @@ func (cp *CloudProvider) updateLoadBalancer(ctx context.Context, lb *loadbalance
ports = spec.Ports[backendSetName]
}

err := cp.updateListener(ctx, lbID, a, ports, lbSubnets, nodeSubnets, spec.SourceCIDRs)
err := cp.updateListener(ctx, lbID, a, ports, lbSubnets, nodeSubnets, spec.SourceCIDRs, spec.SecurityListManager)
if err != nil {
return errors.Wrap(err, "updating listener")
}
Expand All @@ -390,7 +394,7 @@ func (cp *CloudProvider) updateLoadBalancer(ctx context.Context, lb *loadbalance
return nil
}

func (cp *CloudProvider) updateBackendSet(ctx context.Context, lbID string, action *BackendSetAction, lbSubnets, nodeSubnets []*core.Subnet) error {
func (cp *CloudProvider) updateBackendSet(ctx context.Context, lbID string, action *BackendSetAction, lbSubnets, nodeSubnets []*core.Subnet, secListManager securityListManager) error {
var (
sourceCIDRs = []string{}
workRequestID string
Expand All @@ -403,19 +407,19 @@ func (cp *CloudProvider) updateBackendSet(ctx context.Context, lbID string, acti

switch action.Type() {
case Create:
err = cp.securityListManager.Update(ctx, lbSubnets, nodeSubnets, sourceCIDRs, nil, ports)
err = secListManager.Update(ctx, lbSubnets, nodeSubnets, sourceCIDRs, nil, ports)
if err != nil {
return err
}

workRequestID, err = cp.client.LoadBalancer().CreateBackendSet(ctx, lbID, action.Name(), bs)
case Update:
if err = cp.securityListManager.Update(ctx, lbSubnets, nodeSubnets, sourceCIDRs, action.OldPorts, ports); err != nil {
if err = secListManager.Update(ctx, lbSubnets, nodeSubnets, sourceCIDRs, action.OldPorts, ports); err != nil {
return err
}
workRequestID, err = cp.client.LoadBalancer().UpdateBackendSet(ctx, lbID, action.Name(), bs)
case Delete:
err = cp.securityListManager.Delete(ctx, lbSubnets, nodeSubnets, ports)
err = secListManager.Delete(ctx, lbSubnets, nodeSubnets, ports)
if err != nil {
return err
}
Expand All @@ -435,7 +439,7 @@ func (cp *CloudProvider) updateBackendSet(ctx context.Context, lbID string, acti
return nil
}

func (cp *CloudProvider) updateListener(ctx context.Context, lbID string, action *ListenerAction, ports portSpec, lbSubnets, nodeSubnets []*core.Subnet, sourceCIDRs []string) error {
func (cp *CloudProvider) updateListener(ctx context.Context, lbID string, action *ListenerAction, ports portSpec, lbSubnets, nodeSubnets []*core.Subnet, sourceCIDRs []string, secListManager securityListManager) error {
var workRequestID string
var err error
listener := action.Listener
Expand All @@ -445,21 +449,21 @@ func (cp *CloudProvider) updateListener(ctx context.Context, lbID string, action

switch action.Type() {
case Create:
err = cp.securityListManager.Update(ctx, lbSubnets, nodeSubnets, sourceCIDRs, nil, ports)
err = secListManager.Update(ctx, lbSubnets, nodeSubnets, sourceCIDRs, nil, ports)
if err != nil {
return err
}

workRequestID, err = cp.client.LoadBalancer().CreateListener(ctx, lbID, action.Name(), listener)
case Update:
err = cp.securityListManager.Update(ctx, lbSubnets, nodeSubnets, sourceCIDRs, nil, ports)
err = secListManager.Update(ctx, lbSubnets, nodeSubnets, sourceCIDRs, nil, ports)
if err != nil {
return err
}

workRequestID, err = cp.client.LoadBalancer().UpdateListener(ctx, lbID, action.Name(), listener)
case Delete:
err = cp.securityListManager.Delete(ctx, lbSubnets, nodeSubnets, ports)
err = secListManager.Delete(ctx, lbSubnets, nodeSubnets, ports)
if err != nil {
return err
}
Expand Down Expand Up @@ -547,6 +551,9 @@ func (cp *CloudProvider) EnsureLoadBalancerDeleted(ctx context.Context, clusterN
return errors.Wrap(err, "getting subnets for load balancers")
}

securityListManager := cp.securityListManagerFactory(
service.Annotations[ServiceAnnotaionLoadBalancerSecurityListManagementMode])

for listenerName, listener := range lb.Listeners {
backendSetName := *listener.DefaultBackendSetName
bs, ok := lb.BackendSets[backendSetName]
Expand All @@ -559,7 +566,7 @@ func (cp *CloudProvider) EnsureLoadBalancerDeleted(ctx context.Context, clusterN

glog.V(4).Infof("Deleting security rules for listener %q for load balancer %q ports=%+v", listenerName, id, ports)

if err := cp.securityListManager.Delete(ctx, lbSubnets, nodeSubnets, ports); err != nil {
if err := securityListManager.Delete(ctx, lbSubnets, nodeSubnets, ports); err != nil {
return errors.Wrapf(err, "delete security rules for listener %q on load balancer %q", listenerName, name)
}
}
Expand Down
2 changes: 2 additions & 0 deletions pkg/oci/load_balancer_security_lists.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ type baseSecurityListManager struct {
securityLists map[string]string
}

type securityListManagerFactory func(mode string) securityListManager

func newSecurityListManager(client client.Interface, serviceInformer informersv1.ServiceInformer, securityLists map[string]string, mode string) securityListManager {
if securityLists == nil {
securityLists = make(map[string]string)
Expand Down
18 changes: 12 additions & 6 deletions pkg/oci/load_balancer_spec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,16 +74,17 @@ type LBSpec struct {
Listeners map[string]loadbalancer.ListenerDetails
BackendSets map[string]loadbalancer.BackendSetDetails

Ports map[string]portSpec
SourceCIDRs []string
SSLConfig *SSLConfig
Ports map[string]portSpec
SourceCIDRs []string
SSLConfig *SSLConfig
SecurityListManager securityListManager
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't export this field so securityListManager


service *v1.Service
nodes []*v1.Node
}

// NewLBSpec creates a LB Spec from a Kubernetes service and a slice of nodes.
func NewLBSpec(svc *v1.Service, nodes []*v1.Node, defaultSubnets []string, sslCfg *SSLConfig) (*LBSpec, error) {
func NewLBSpec(svc *v1.Service, nodes []*v1.Node, defaultSubnets []string, sslCfg *SSLConfig, secListFactory securityListManagerFactory) (*LBSpec, error) {
if len(defaultSubnets) != 2 {
return nil, errors.New("default subnets incorrectly configured")
}
Expand Down Expand Up @@ -134,6 +135,10 @@ func NewLBSpec(svc *v1.Service, nodes []*v1.Node, defaultSubnets []string, sslCf
return nil, err
}

//A security list manager will be configured based on the annotation specified when creating the service,
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after //

//if an annotation is not specified, then the mode specified in cloud provider config file is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after //

slManagerSpec := secListFactory(svc.Annotations[ServiceAnnotaionLoadBalancerSecurityListManagementMode])

return &LBSpec{
Name: GetLoadBalancerName(svc),
Shape: shape,
Expand All @@ -146,8 +151,9 @@ func NewLBSpec(svc *v1.Service, nodes []*v1.Node, defaultSubnets []string, sslCf
SSLConfig: sslCfg,
SourceCIDRs: sourceCIDRs,

service: svc,
nodes: nodes,
service: svc,
nodes: nodes,
SecurityListManager: slManagerSpec,
Copy link
Contributor

Choose a reason for hiding this comment

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

SecurityListManager: secListFactory(
	svc.Annotations[ServiceAnnotaionLoadBalancerSecurityListManagementMode],
)

}, nil
}

Expand Down
21 changes: 18 additions & 3 deletions pkg/oci/load_balancer_spec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ func TestNewLBSpecSuccess(t *testing.T) {
HealthCheckerPort: 10256,
},
},
SecurityListManager: newSecurityListManagerNOOP(),
},
},
"internal": {
Expand Down Expand Up @@ -137,6 +138,7 @@ func TestNewLBSpecSuccess(t *testing.T) {
HealthCheckerPort: 10256,
},
},
SecurityListManager: newSecurityListManagerNOOP(),
},
},
"subnet annotations": {
Expand Down Expand Up @@ -192,8 +194,10 @@ func TestNewLBSpecSuccess(t *testing.T) {
HealthCheckerPort: 10256,
},
},
SecurityListManager: newSecurityListManagerNOOP(),
},
},
//"security list manager annotation":
"custom shape": {
defaultSubnetOne: "one",
defaultSubnetTwo: "two",
Expand Down Expand Up @@ -246,6 +250,7 @@ func TestNewLBSpecSuccess(t *testing.T) {
HealthCheckerPort: 10256,
},
},
SecurityListManager: newSecurityListManagerNOOP(),
},
},
"custom idle connection timeout": {
Expand Down Expand Up @@ -303,6 +308,7 @@ func TestNewLBSpecSuccess(t *testing.T) {
HealthCheckerPort: 10256,
},
},
SecurityListManager: newSecurityListManagerNOOP(),
},
},
}
Expand All @@ -312,7 +318,11 @@ func TestNewLBSpecSuccess(t *testing.T) {
// we expect the service to be unchanged
tc.expected.service = tc.service
subnets := []string{tc.defaultSubnetOne, tc.defaultSubnetTwo}
result, err := NewLBSpec(tc.service, tc.nodes, subnets, nil)
//reference default cp added
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after //

slManagerFactory := func(mode string) securityListManager {
return newSecurityListManagerNOOP()
}
result, err := NewLBSpec(tc.service, tc.nodes, subnets, nil, slManagerFactory)
if err != nil {
t.Error(err)
}
Expand All @@ -330,7 +340,8 @@ func TestNewLBSpecFailure(t *testing.T) {
defaultSubnetTwo string
nodes []*v1.Node
service *v1.Service
expectedErrMsg string
//add cp or cp security list
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after //

expectedErrMsg string
}{
"unsupported udp protocol": {
service: &v1.Service{
Expand Down Expand Up @@ -415,6 +426,7 @@ func TestNewLBSpecFailure(t *testing.T) {
Spec: v1.ServiceSpec{
SessionAffinity: v1.ServiceAffinityNone,
Ports: []v1.ServicePort{},
//add security list mananger in spec
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after //

},
},
expectedErrMsg: "a configuration for subnet1 must be specified for an internal load balancer",
Expand All @@ -424,7 +436,10 @@ func TestNewLBSpecFailure(t *testing.T) {
for name, tc := range testCases {
t.Run(name, func(t *testing.T) {
subnets := []string{tc.defaultSubnetOne, tc.defaultSubnetTwo}
_, err := NewLBSpec(tc.service, tc.nodes, subnets, nil)
slManagerFactory := func(mode string) securityListManager {
return newSecurityListManagerNOOP()
}
_, err := NewLBSpec(tc.service, tc.nodes, subnets, nil, slManagerFactory)
if err == nil || err.Error() != tc.expectedErrMsg {
t.Errorf("Expected error with message %q but got %q", tc.expectedErrMsg, err)
}
Expand Down