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.

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

Jump to bottom

Allow to overwrite default ExternalTrafficPolicy for the service #1136

Merged
merged 6 commits into from
Sep 15, 2020
Merged

Allow to overwrite default ExternalTrafficPolicy for the service #1136

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
65a2add
Allow to overwrite default ExternalTrafficPolicy for the service
yanchenko-igor Sep 10, 2020
0c7bf4f
zz_generated.deepcopy.go updated
yanchenko-igor Sep 10, 2020
6543b12
test added
yanchenko-igor Sep 10, 2020
00cbaa4
external_traffic_policy if defined in Operator configuration instead …
yanchenko-igor Sep 10, 2020
59ba0bf
cleanup
yanchenko-igor Sep 10, 2020
07b3094
review fixes
yanchenko-igor Sep 10, 2020
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
2 changes: 0 additions & 2 deletions docs/administrator.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,8 +527,6 @@ lead to K8s removing this field from the manifest due to its
[handling of null fields](https://kubernetes.io/docs/concepts/overview/object-management-kubectl/declarative-config/#how-apply-calculates-differences-and-merges-changes).
Then the resultant manifest will not contain the necessary change, and the
operator will respectively do nothing with the existing source ranges.
To define external traffic policy for the load balancer set the option
externalTrafficPolicy, it will default to `Cluster` if undefined.

## Running periodic 'autorepair' scans of K8s objects

Expand Down
3 changes: 3 additions & 0 deletions docs/reference/operator_parameters.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,9 @@ In the CRD-based configuration they are grouped under the `load_balancer` key.
replaced with the hosted zone (the value of the `db_hosted_zone` parameter).
No other placeholders are allowed.
* **external_traffic_policy** define external traffic policy for the load
balancer, it will default to `Cluster` if undefined.
## AWS or GCP interaction
The options in this group configure operator interactions with non-Kubernetes
Expand Down
11 changes: 11 additions & 0 deletions pkg/apis/acid.zalan.do/v1/crds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1129,6 +1129,17 @@ var OperatorConfigCRDResourceValidation = apiextv1beta1.CustomResourceValidation
"replica_dns_name_format": {
Type: "string",
},
"external_traffic_policy": {
Type: "string",
Enum: []apiextv1beta1.JSON{
{
Raw: []byte(`"Cluster"`),
},
{
Raw: []byte(`"Local"`),
},
},
},
},
},
"aws_or_gcp": {
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/acid.zalan.do/v1/operator_configuration_type.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ type LoadBalancerConfiguration struct {
CustomServiceAnnotations map[string]string `json:"custom_service_annotations,omitempty"`
MasterDNSNameFormat config.StringTemplate `json:"master_dns_name_format,omitempty"`
ReplicaDNSNameFormat config.StringTemplate `json:"replica_dns_name_format,omitempty"`
ExternalTrafficPolicy string `json:"external_traffic_policy" default:"Cluster"`
}

// AWSGCPConfiguration defines the configuration for AWS
Expand Down
8 changes: 3 additions & 5 deletions pkg/cluster/k8sres.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1619,12 +1619,10 @@ func (c *Cluster) generateService(role PostgresRole, spec *acidv1.PostgresSpec)
}

c.logger.Debugf("final load balancer source ranges as seen in a service spec (not necessarily applied): %q", serviceSpec.LoadBalancerSourceRanges)
// spec.externalTrafficPolicy can define externalTrafficPolicy for the service
// if spec.externalTrafficPolicy is not defined default value is "Cluster"
if spec.ExternalTrafficPolicy != nil {
serviceSpec.ExternalTrafficPolicy = *spec.ExternalTrafficPolicy
if c.OpConfig.ExternalTrafficPolicy == "" {
Copy link
Member

Choose a reason for hiding this comment

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

With the absence of any kind of error handling just dealing with the empty case does not make sense or? it is already set to default to "Cluster" if not set. so just line 1625 is good enough.

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 am not sure if I am doing anything wrong, but I don't get default values in the test.

Copy link
Member

Choose a reason for hiding this comment

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

Yes but in no case is c.opconfig.externaltrafficpolicy "", which is fine. I would just argue this whole if statement can just be

serviceSpec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyType(c.OpConfig.ExternalTrafficPolicy)

You have the default properly set, so it should be cluster for everyone.

Everyone else switches it to Local.

And everyone else using something invalid gets an error.

So why deal/cover """ case extra?

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 mean, if I run tests and I don't define c.opConfig.ExternalTrafficPolicy, then I get "" here. Not the default "Cluster" that I expect.

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 guess it's just because the way how the config is defined in tests.

serviceSpec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyType("Cluster")
} else {
serviceSpec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyTypeCluster
serviceSpec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyType(c.OpConfig.ExternalTrafficPolicy)
}
serviceSpec.Type = v1.ServiceTypeLoadBalancer
} else if role == Replica {
Expand Down
9 changes: 3 additions & 6 deletions pkg/cluster/k8sres_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1746,7 +1746,6 @@ func TestSidecars(t *testing.T) {
func TestGenerateService(t *testing.T) {
var spec acidv1.PostgresSpec
var cluster *Cluster
var policy v1.ServiceExternalTrafficPolicyType = "Local"
var enableLB bool = true
spec = acidv1.PostgresSpec{
TeamID: "myapp", NumberOfInstances: 1,
Expand All @@ -1773,7 +1772,6 @@ func TestGenerateService(t *testing.T) {
DockerImage: "overwrite-image",
},
},
ExternalTrafficPolicy: &policy,
EnableMasterLoadBalancer: &enableLB,
}

Expand Down Expand Up @@ -1817,10 +1815,9 @@ func TestGenerateService(t *testing.T) {
}, k8sutil.KubernetesClient{}, acidv1.Postgresql{}, logger, eventRecorder)

service := cluster.generateService(Master, &spec)
fmt.Printf("%+v\n", service)
assert.Equal(t, v1.ServiceExternalTrafficPolicyTypeLocal, service.Spec.ExternalTrafficPolicy)
spec.ExternalTrafficPolicy = nil
service = cluster.generateService(Master, &spec)
assert.Equal(t, v1.ServiceExternalTrafficPolicyTypeCluster, service.Spec.ExternalTrafficPolicy)
cluster.OpConfig.ExternalTrafficPolicy = "Local"
service = cluster.generateService(Master, &spec)
assert.Equal(t, v1.ServiceExternalTrafficPolicyTypeLocal, service.Spec.ExternalTrafficPolicy)

}
1 change: 1 addition & 0 deletions pkg/controller/operator_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ func (c *Controller) importConfigurationFromCRD(fromCRD *acidv1.OperatorConfigur
result.CustomServiceAnnotations = fromCRD.LoadBalancer.CustomServiceAnnotations
result.MasterDNSNameFormat = fromCRD.LoadBalancer.MasterDNSNameFormat
result.ReplicaDNSNameFormat = fromCRD.LoadBalancer.ReplicaDNSNameFormat
result.ExternalTrafficPolicy = util.Coalesce(fromCRD.LoadBalancer.ExternalTrafficPolicy, "Cluster")

// AWS or GCP config
result.WALES3Bucket = fromCRD.AWSGCP.WALES3Bucket
Expand Down
2 changes: 2 additions & 0 deletions pkg/util/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,8 @@ type Config struct {
EnablePodAntiAffinity bool `name:"enable_pod_antiaffinity" default:"false"`
PodAntiAffinityTopologyKey string `name:"pod_antiaffinity_topology_key" default:"kubernetes.io/hostname"`
StorageResizeMode string `name:"storage_resize_mode" default:"ebs"`
// ExternalTrafficPolicy for load balancer
ExternalTrafficPolicy string `name:"external_traffic_policy" default:"Cluster"`
// deprecated and kept for backward compatibility
EnableLoadBalancer *bool `name:"enable_load_balancer"`
MasterDNSNameFormat StringTemplate `name:"master_dns_name_format" default:"{cluster}.{team}.{hostedzone}"`
Expand Down