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

fix issue with gg 1.103 with pod network being a slice #177

Merged
merged 1 commit into from
Sep 16, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
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)
}
Loading