Skip to content

Commit

Permalink
fix issue with gg 1.103 with pod network being a slice (#177)
Browse files Browse the repository at this point in the history
  • Loading branch information
kon-angelo authored Sep 16, 2024
1 parent cfd8e9f commit f4d9ee3
Show file tree
Hide file tree
Showing 10 changed files with 104 additions and 50 deletions.
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ LEADER_ELECT := "true"
# If Integration Test Suite is to be run locally against clusters then export the below variable
# with MCM deployment name in the cluster
MACHINE_CONTROLLER_MANAGER_DEPLOYMENT_NAME := machine-controller-manager
PLATFORM ?= linux/amd64

#########################################
# Tools & Cleanup
Expand Down Expand Up @@ -116,7 +117,6 @@ test-integration:
.PHONY: release
release: docker-image docker-push

PLATFORM ?= linux/amd64
.PHONY: docker-image
docker-image:
@docker buildx build --platform $(PLATFORM) -t $(IMAGE_NAME):$(VERSION) -t $(IMAGE_NAME):latest .
Expand Down
32 changes: 30 additions & 2 deletions hack/api-reference/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,21 @@ string
</em>
</td>
<td>
<p>PodNetworkCidr is the CIDR range for the pods assigned to this instance.</p>
<em>(Optional)</em>
<p>PodNetworkCidr is the CIDR range for the pods assigned to this instance.
Deprecated: use PodNetworkCIDRs instead</p>
</td>
</tr>
<tr>
<td>
<code>podNetworkCIDRs</code></br>
<em>
[]string
</em>
</td>
<td>
<em>(Optional)</em>
<p>PodNetworkCIDRs is the CIDR ranges for the pods assigned to this instance.</p>
</td>
</tr>
<tr>
Expand Down Expand Up @@ -360,7 +374,21 @@ string
</em>
</td>
<td>
<p>PodNetworkCidr is the CIDR range for the pods assigned to this instance.</p>
<em>(Optional)</em>
<p>PodNetworkCidr is the CIDR range for the pods assigned to this instance.
Deprecated: use PodNetworkCIDRs instead</p>
</td>
</tr>
<tr>
<td>
<code>podNetworkCIDRs</code></br>
<em>
[]string
</em>
</td>
<td>
<em>(Optional)</em>
<p>PodNetworkCIDRs is the CIDR ranges for the pods assigned to this instance.</p>
</td>
</tr>
<tr>
Expand Down
3 changes: 3 additions & 0 deletions pkg/apis/openstack/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@ type MachineProviderConfigSpec struct {
// SubnetID is the ID of the subnet the instance should belong to. If SubnetID is not specified
SubnetID *string
// PodNetworkCidr is the CIDR range for the pods assigned to this instance.
// Deprecated - use `PodNetworkCIDRs` instead.
PodNetworkCidr string
// PodNetworkCidr is the CIDR ranges for the pods assigned to this instance.
PodNetworkCIDRs []string
// The size of the root disk used for the instance.
RootDiskSize int
// The type of the root disk type used for the instance
Expand Down
5 changes: 5 additions & 0 deletions pkg/apis/openstack/v1alpha1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,12 @@ type MachineProviderConfigSpec struct {
// +optional
SubnetID *string `json:"subnetID,omitempty"`
// PodNetworkCidr is the CIDR range for the pods assigned to this instance.
// Deprecated: use PodNetworkCIDRs instead
// +optional
PodNetworkCidr string `json:"podNetworkCidr"`
// PodNetworkCIDRs is the CIDR ranges for the pods assigned to this instance.
// +optional
PodNetworkCIDRs []string `json:"podNetworkCIDRs"`
// The size of the root disk used for the instance.
RootDiskSize int `json:"rootDiskSize,omitempty"` // in GB
// The type of the root disk used for the instance.
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/openstack/v1alpha1/zz_generated.conversion.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions pkg/apis/openstack/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions pkg/apis/openstack/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 5 additions & 5 deletions pkg/apis/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,20 +57,20 @@ func validateMachineProviderConfig(providerConfig *openstack.MachineProviderConf
if "" == providerConfig.Spec.NetworkID && len(providerConfig.Spec.Networks) == 0 {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("networkID"), "both \"networks\" and \"networkID\" should not be empty"))
}
if "" == providerConfig.Spec.PodNetworkCidr {
allErrs = append(allErrs, field.Required(fldPath.Child("podNetworkCidr"), "PodNetworkCidr is required"))
if len(providerConfig.Spec.PodNetworkCIDRs) == 0 && len(providerConfig.Spec.PodNetworkCidr) == 0 {
allErrs = append(allErrs, field.Required(fldPath.Child("PodNetworkCIDRs"), "PodNetworkCIDRs is required"))
}
if providerConfig.Spec.RootDiskSize < 0 {
allErrs = append(allErrs, field.Required(fldPath.Child("rootDiskSize"), "RootDiskSize can not be negative"))
}

allErrs = append(allErrs, validateNetworks(providerConfig.Spec.Networks, providerConfig.Spec.PodNetworkCidr, field.NewPath("spec.networks"))...)
allErrs = append(allErrs, validateNetworks(providerConfig.Spec.Networks, providerConfig.Spec.PodNetworkCidr, providerConfig.Spec.PodNetworkCIDRs, field.NewPath("spec.networks"))...)
allErrs = append(allErrs, validateClassSpecTags(providerConfig.Spec.Tags, field.NewPath("spec.tags"))...)

return allErrs
}

func validateNetworks(networks []openstack.OpenStackNetwork, podNetworkCidr string, fldPath *field.Path) field.ErrorList {
func validateNetworks(networks []openstack.OpenStackNetwork, podNetworkCidr string, podNetworkCIDRs []string, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

for index, network := range networks {
Expand All @@ -81,7 +81,7 @@ func validateNetworks(networks []openstack.OpenStackNetwork, podNetworkCidr stri
if "" != network.Id && "" != network.Name {
allErrs = append(allErrs, field.Forbidden(fldPath, "simultaneous use of network \"id\" and \"name\" is forbidden"))
}
if "" == podNetworkCidr && network.PodNetwork {
if len(podNetworkCIDRs) == 0 && len(podNetworkCidr) == 0 && network.PodNetwork {
allErrs = append(allErrs, field.Required(fldPath.Child("podNetwork"), "\"podNetwork\" switch should not be used in absence of \"spec.podNetworkCidr\""))
}
}
Expand Down
18 changes: 9 additions & 9 deletions pkg/apis/validation/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,13 @@ var _ = Describe("Validation", func() {
fmt.Sprintf("%s-foo", ServerTagRolePrefix): "1",
fmt.Sprintf("%s-foo", ServerTagClusterPrefix): "1",
},
NetworkID: "networkID",
SubnetID: nil,
PodNetworkCidr: "10.0.0.1/8",
RootDiskSize: 0,
UseConfigDrive: nil,
ServerGroupID: nil,
Networks: nil,
NetworkID: "networkID",
SubnetID: nil,
PodNetworkCIDRs: []string{"10.0.0.1/8"},
RootDiskSize: 0,
UseConfigDrive: nil,
ServerGroupID: nil,
Networks: nil,
},
}
})
Expand All @@ -59,7 +59,7 @@ var _ = Describe("Validation", func() {
spec.FlavorName = ""
spec.AvailabilityZone = ""
spec.KeyName = ""
spec.PodNetworkCidr = ""
spec.PodNetworkCIDRs = nil
err := validateMachineProviderConfig(machineProviderConfig)

Expect(err).To(ConsistOf(
Expand All @@ -81,7 +81,7 @@ var _ = Describe("Validation", func() {
})),
PointTo(MatchFields(IgnoreExtras, Fields{
"Type": BeEquivalentTo("FieldValueRequired"),
"Field": Equal("spec.podNetworkCidr"),
"Field": Equal("spec.PodNetworkCIDRs"),
})),
))
})
Expand Down
72 changes: 39 additions & 33 deletions pkg/driver/executor/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/wait"
"k8s.io/klog/v2"
"k8s.io/utils/pointer"
"k8s.io/utils/ptr"

api "github.com/gardener/machine-controller-manager-provider-openstack/pkg/apis/openstack"
"github.com/gardener/machine-controller-manager-provider-openstack/pkg/client"
Expand Down Expand Up @@ -134,7 +134,7 @@ func (ex *Executor) resolveServerNetworks(ctx context.Context, machineName strin
return serverNetworks, nil
}

if !isEmptyString(pointer.StringPtr(networkID)) {
if !isEmptyString(ptr.To(networkID)) {
klog.V(3).Infof("deploying in network [ID=%q]", networkID)
serverNetworks = append(serverNetworks, servers.Network{UUID: ex.Config.Spec.NetworkID})
return serverNetworks, nil
Expand All @@ -145,7 +145,7 @@ func (ex *Executor) resolveServerNetworks(ctx context.Context, machineName strin
resolvedNetworkID string
err error
)
if isEmptyString(pointer.StringPtr(network.Id)) {
if isEmptyString(ptr.To(network.Id)) {
resolvedNetworkID, err = ex.Network.NetworkIDFromName(network.Name)
if err != nil {
return nil, err
Expand Down Expand Up @@ -383,42 +383,49 @@ func (ex *Executor) patchServerPortsForPodNetwork(serverID string) error {
return fmt.Errorf("failed to resolve network IDs for the pod network %v", err)
}

// coalesce all pod network CIDRs into a single slice.
podCIDRs := sets.NewString(ex.Config.Spec.PodNetworkCIDRs...)
if ex.Config.Spec.PodNetworkCidr != "" {
podCIDRs.Insert(ex.Config.Spec.PodNetworkCidr)
}

for _, port := range allPorts {
if podNetworkIDs.Has(port.NetworkID) {
addressPairFound := false

for _, pair := range port.AllowedAddressPairs {
if pair.IPAddress == ex.Config.Spec.PodNetworkCidr {
klog.V(3).Infof("port [ID=%q] already allows pod network CIDR range. Skipping update...", port.ID)
addressPairFound = true
// break inner loop if target found
break
}
}
// continue outer loop if target found
if addressPairFound {
continue
}
// if the port is not part of the networks we care about, continue.
if !podNetworkIDs.Has(port.NetworkID) {
continue
}

if err := ex.Network.UpdatePort(port.ID, ports.UpdateOpts{
AllowedAddressPairs: &[]ports.AddressPair{{IPAddress: ex.Config.Spec.PodNetworkCidr}},
}); err != nil {
return fmt.Errorf("failed to update allowed address pair for port [ID=%q]: %v", port.ID, err)
for _, cidr := range podCIDRs.List() {
if err := func() error {
for _, pair := range port.AllowedAddressPairs {
if pair.IPAddress == cidr {
klog.V(3).Infof("port [ID=%q] already allows pod network CIDR range. Skipping update...", port.ID)
return nil
}
}
if err := ex.Network.UpdatePort(port.ID, ports.UpdateOpts{
AllowedAddressPairs: &[]ports.AddressPair{{IPAddress: cidr}},
}); err != nil {
return fmt.Errorf("failed to update allowed address pair for port [ID=%q]: %v", port.ID, err)
}
return nil
}(); err != nil {
return err
}
}
}
return nil
}

// resolveNetworkIDsForPodNetwork resolves the networks that accept traffic from the pod CIDR range.
func (ex *Executor) resolveNetworkIDsForPodNetwork() (sets.String, error) {
func (ex *Executor) resolveNetworkIDsForPodNetwork() (sets.Set[string], error) {
var (
networkID = ex.Config.Spec.NetworkID
networks = ex.Config.Spec.Networks
podNetworkIDs = sets.NewString()
podNetworkIDs = sets.New[string]()
)

if !isEmptyString(pointer.StringPtr(networkID)) {
if !isEmptyString(ptr.To(networkID)) {
podNetworkIDs.Insert(networkID)
return podNetworkIDs, nil
}
Expand All @@ -428,7 +435,7 @@ func (ex *Executor) resolveNetworkIDsForPodNetwork() (sets.String, error) {
resolvedNetworkID string
err error
)
if isEmptyString(pointer.StringPtr(network.Id)) {
if isEmptyString(ptr.To(network.Id)) {
resolvedNetworkID, err = ex.Network.NetworkIDFromName(network.Name)
if err != nil {
return nil, err
Expand All @@ -451,7 +458,7 @@ func (ex *Executor) DeleteMachine(ctx context.Context, machineName, providerID s
err error
)

if !isEmptyString(pointer.StringPtr(providerID)) {
if !isEmptyString(ptr.To(providerID)) {
serverID := decodeProviderID(providerID)
server, err = ex.getMachineByID(ctx, serverID)
} else {
Expand Down Expand Up @@ -514,11 +521,10 @@ func (ex *Executor) getOrCreatePort(_ context.Context, machineName string) (stri
}

port, err := ex.Network.CreatePort(&ports.CreateOpts{
Name: machineName,
NetworkID: ex.Config.Spec.NetworkID,
FixedIPs: []ports.IP{{SubnetID: *ex.Config.Spec.SubnetID}},
AllowedAddressPairs: []ports.AddressPair{{IPAddress: ex.Config.Spec.PodNetworkCidr}},
SecurityGroups: &securityGroupIDs,
Name: machineName,
NetworkID: ex.Config.Spec.NetworkID,
FixedIPs: []ports.IP{{SubnetID: *ex.Config.Spec.SubnetID}},
SecurityGroups: &securityGroupIDs,
})
if err != nil {
return "", err
Expand Down Expand Up @@ -695,5 +701,5 @@ func (ex *Executor) listServers(_ context.Context) ([]servers.Server, error) {

// isUserManagedNetwork returns true if the port used by the machine will be created and managed by MCM.
func (ex *Executor) isUserManagedNetwork() bool {
return !isEmptyString(pointer.StringPtr(ex.Config.Spec.NetworkID)) && !isEmptyString(ex.Config.Spec.SubnetID)
return !isEmptyString(ptr.To(ex.Config.Spec.NetworkID)) && !isEmptyString(ex.Config.Spec.SubnetID)
}

0 comments on commit f4d9ee3

Please sign in to comment.