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

Add support for port aggregation #182

Merged
merged 4 commits into from
Mar 8, 2023
Merged

Add support for port aggregation #182

merged 4 commits into from
Mar 8, 2023

Conversation

paultyng
Copy link
Owner

Fixes #142

@paultyng
Copy link
Owner Author

Unsure how to test this, need to see if its possible without adopting a device or if it needs one to run against.

joshuaspence
joshuaspence previously approved these changes Sep 14, 2021
Copy link
Collaborator

@joshuaspence joshuaspence left a comment

Choose a reason for hiding this comment

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

It doesn't need to be a part of this PR, but I think we should add acceptance test coverage that is opt-in with an environment variable to run against real hardware. Something like UNIFI_SWITCH=<MAC_ADDRESS>.

ValidateFunc: validation.StringInSlice([]string{"switch", "mirror", "aggregate"}, false),
// Default: "switch",
},
"aggregate_num_ports": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add validation to make sure this is only set if op_mode is set to aggregate?

@dekarl
Copy link

dekarl commented Dec 12, 2021

Is there an easy way to test this in combination with gitlab-terraform images?

I built the plugin and simply replaced the cached one in .terraform/providers/registry.terraform.io/paultyng/unifi/0.34.0/linux_amd64/terraform-provider-unifi_v0.34.0 but on the next run of gitlab-terraform plan it gets replaced with a pristine copy from the registry...

The change itself looks good when I compare it to the JSON returned when configuring a LAG.

Maybe add some validation, like aggregate_num_ports must be set to use op_mode=aggregate. Or that there is no port_override for port 12 if there is a 2-port-LAG on port 11.

@dekarl
Copy link

dekarl commented Dec 14, 2021

I managed to test it locally, appears to be working.
A small nit, the port override to have just one native VLAN on switching port 3 detects a drift with nothing to apply every time I run terraform plan. Looks like aggregate_num_ports should be omitted when op_mode=switching or aggregate_num_ports = 0

Terraform detected the following changes made outside of Terraform since the last "terraform apply":

  # unifi_device.switch-sued-2 has been changed
  ~ resource "unifi_device" "switch-sued-2" {
        id       = "5fde1f005ab20c014e3f7d19"
        name     = "sued-switch-2"
        # (3 unchanged attributes hidden)

      + port_override {
          + aggregate_num_ports = 0
          + name                = "switch-maas"
          + number              = 3
          + port_profile_id     = "61a80d3d5ab20c0144a62015"
        }
      - port_override {
          - name            = "switch-maas" -> null
          - number          = 3 -> null
          - port_profile_id = "61a80d3d5ab20c0144a62015" -> null
        }
        # (1 unchanged block hidden)
    }

@dekarl
Copy link

dekarl commented Mar 7, 2022

I'm using this branch successfully in homelab-production. But I'd love to return to the released version. What is still needed before merging it?

@CarbonCollins
Copy link

👋 Is planned to be merged at any point? Would very much like to be able to configure this :D

@szinn
Copy link

szinn commented Mar 6, 2023

@paultyng Anything else blocking this from being merged?

Note that only switches with a Broadcom, Microsemi or Nephos chipset support both port mirroring and port aggregation.

```java
package com.ubnt.data;

public class Device extends X implements Sanitizable
{

  // ...

  public int getMaxMirrorSession() {
    int n = 0;
    if (this.getModel() == Model.\u00f8\u00f50000) {
      n = 2;
    }
    else if (this.isBroadcomSwitch() || this.isMicrosemiSwitch() || this.isMediaTekSwitch() || this.isNephosSwitch()) {
      n = 1;
    }
    return this.thisforObject().getInt("max_mirror_sessions", n);
  }

  public int getMaxAggregation() {
    int n = 0;
    if (this.isBroadcomSwitch() || this.isMicrosemiSwitch() || this.isNephosSwitch()) {
      n = 6;
    }
    return this.thisforObject().getInt("max_aggregate_sessions", n);
  }

  // ...

  public boolean isBroadcomSwitch() {
    final Model model = this.getModel();
    return model.getChipset().typeOf(Chipset.\u00f400000) && model.getType() == DeviceType.if;
  }

  public boolean isMicrosemiSwitch() {
    final Model model = this.getModel();
    return model.getChipset().typeOf(Chipset.o00000) && model.getType() == DeviceType.if;
  }

  public boolean isMediaTekSwitch() {
    final Model model = this.getModel();
    return model.getChipset().typeOf(Chipset.\u00d3O0000) && model.getType() == DeviceType.if;
  }

  public boolean isNephosSwitch() {
    final Model model = this.getModel();
    return model.getChipset().typeOf(Chipset.\u00d5O0000) && model.getType() == DeviceType.if;
  }

  // ...

}
```

To extract the list of models that use one of these chipsets I used the following script (executed as `java --class-path path/to/ace.jar main.java`):

```java
import com.ubnt.data.Model;

class UniFiModels {
  public static void main(String[] args) {
    /*
    for (Model model : Model.values()) {
      System.out.printf(
          "Model = %s\nSKU = %s\nType = %s\nFeatures = %s\nChipset = %s\nSysId = %s\nPortNum = %s\n\n",
          model,
          model.getSku(),
          model.getType(),
          model.getFeatures(),
          model.getChipset(),
          model.getSysId(),
          model.getPortNum());
    }
    */

    for (Model model : Model.values()) {
      System.out.printf("%s: %s (%s)\n", model.getChipset(), model, model.getSku());
    }
  }
}

```
@joshuaspence joshuaspence merged commit bf08789 into main Mar 8, 2023
@joshuaspence joshuaspence deleted the port-aggregation branch March 8, 2023 07:02
@joshuaspence
Copy link
Collaborator

@dekarl @szinn @CarbonCollins I've added test coverage and merged this now. Please feel free to test it out and report any issues you find. I will test it myself and make a new release in the next few days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add switch port aggregation support
5 participants