OCPBUGS-114882: drop GCD health-check ranges in openshift-gcp-routes #6501
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| package template | ||
|
|
||
| // gcpPublicHealthCheckSourceRanges are the prober ranges for public GCP. | ||
| var gcpPublicHealthCheckSourceRanges = []string{ | ||
| "35.191.0.0/16", | ||
| "130.211.0.0/22", | ||
| } | ||
|
|
||
| // gcdHealthCheckSourceRanges maps a GCD region to its prober ranges. Sourced | ||
| // from the "Probe IP ranges" section of each region's | ||
| // load-balancing/docs/firewall-rules (u-france-east1: https://documentation.s3ns.fr). | ||
| var gcdHealthCheckSourceRanges = map[string][]string{ | ||
| "u-germany-northeast1": { | ||
| "34.3.144.0/23", | ||
| "34.3.151.0/26", | ||
| "34.3.151.64/26", | ||
| "136.124.104.0/22", | ||
| "136.124.108.0/22", | ||
| }, | ||
| "u-france-east1": { | ||
| "177.222.80.0/23", | ||
| "177.222.87.0/26", | ||
| "177.222.87.64/26", | ||
| "136.124.104.0/22", | ||
| "136.124.108.0/22", | ||
| }, | ||
| } | ||
|
|
||
| // gcpHealthCheckSourceRanges returns the health-check prober source ranges to | ||
| // drop for the cluster's region. Only GCD regions have specific ranges. | ||
| func gcpHealthCheckSourceRanges(cfg RenderConfig) []string { | ||
|
Contributor
There was a problem hiding this comment. Does https://github.com/openshift/api/blob/6733660e6ece5593234b36fd21327c4c8ec96db2/config/v1/types_infrastructure.go#L854 UniverseDomain intersect with this at all? Also I noticed that in the MCO vendored code, the featuregate for soverign cloud on GCP https://github.com/openshift/machine-config-operator/blob/main/vendor/github.com/openshift/api/features/features.go#L880 is still dev preview. Do you need a vendor bump?
Contributor
Author
There was a problem hiding this comment.
Yes, it does intersect, in the sense that these GCD sovereign cloud regions have alternate universe domains. Most of the implementation for this feature has been updating clients to use an alternate universe domain when talking to GCD. I'm not sure how strict the coupling is between GCD and alternate universe domains, it seems possible there will be uses of alternate universe domains separate from GCD.
Contributor
Author
There was a problem hiding this comment.
Maybe? I have not used feature gates in MCO before, and I wasn't aware it needed to be explicitly vendored in to be updated (the installer is like that too, but most components AFAIK do not need vendoring...) On the other hand, nothing in the code is currently gated and the gated API field is not being used in this case, so it doesn't seem strictly necessary. That said, it seems like a good idea to have it up to date, so I can include a bump here.
Contributor
Author
There was a problem hiding this comment. Bumped openshift/api for good measure |
||
| if ps := cfg.Infra.Status.PlatformStatus; ps != nil && ps.GCP != nil { | ||
| if ranges, ok := gcdHealthCheckSourceRanges[ps.GCP.Region]; ok { | ||
| return ranges | ||
| } | ||
| } | ||
| return gcpPublicHealthCheckSourceRanges | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| package template | ||
|
|
||
| import ( | ||
| "reflect" | ||
| "testing" | ||
|
|
||
| configv1 "github.com/openshift/api/config/v1" | ||
| mcfgv1 "github.com/openshift/api/machineconfiguration/v1" | ||
| ) | ||
|
|
||
| func TestGCPHealthCheckSourceRanges(t *testing.T) { | ||
| renderConfig := func(region string) RenderConfig { | ||
| return RenderConfig{ | ||
| ControllerConfigSpec: &mcfgv1.ControllerConfigSpec{ | ||
| Infra: &configv1.Infrastructure{ | ||
| Status: configv1.InfrastructureStatus{ | ||
| PlatformStatus: &configv1.PlatformStatus{ | ||
| GCP: &configv1.GCPPlatformStatus{Region: region}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| cfg RenderConfig | ||
| expected []string | ||
| }{ | ||
| { | ||
| name: "public region uses public GCP ranges", | ||
| cfg: renderConfig("us-central1"), | ||
| expected: []string{"35.191.0.0/16", "130.211.0.0/22"}, | ||
| }, | ||
| { | ||
| name: "gcd berlin uses its own region ranges", | ||
| cfg: renderConfig("u-germany-northeast1"), | ||
| expected: []string{"34.3.144.0/23", "34.3.151.0/26", "34.3.151.64/26", "136.124.104.0/22", "136.124.108.0/22"}, | ||
| }, | ||
| { | ||
| name: "gcd france uses its own region ranges", | ||
| cfg: renderConfig("u-france-east1"), | ||
| expected: []string{"177.222.80.0/23", "177.222.87.0/26", "177.222.87.64/26", "136.124.104.0/22", "136.124.108.0/22"}, | ||
| }, | ||
| { | ||
| name: "nil gcp platform status falls back to public ranges", | ||
| cfg: RenderConfig{ControllerConfigSpec: &mcfgv1.ControllerConfigSpec{Infra: &configv1.Infrastructure{}}}, | ||
| expected: []string{"35.191.0.0/16", "130.211.0.0/22"}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if got := gcpHealthCheckSourceRanges(tt.cfg); !reflect.DeepEqual(got, tt.expected) { | ||
| t.Errorf("gcpHealthCheckSourceRanges() = %v, want %v", got, tt.expected) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the following entry are listed for both germany and france, are they actually regional? Or global? (Doesn't affect the implementation I think but just curious)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm yes apparently both of these ranges are included in both S3NS france docs and the berlin region, the docs for which are not publicy available.
These are both for external passthrough load balancers so, yes they are global, although distinct from the gcp ranges.