Skip to content

feat(): layer2 support for CAPP #787

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

Closed
wants to merge 28 commits into from

Conversation

rahulii
Copy link

@rahulii rahulii commented Aug 22, 2024

✨ API and CRD changes for L2 support

This draft PR implements the API changes for design described in design doc (L2 design). This branch is being used to work through the design, build intuition, and gain early insights and feedback.

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #613

Signed-off-by: rahulii <r.sawra@gmail.com>
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 22, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rahulii
Once this PR has been reviewed and has the lgtm label, please assign vincepri for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 22, 2024
Signed-off-by: rahulii <r.sawra@gmail.com>
@rahulii rahulii marked this pull request as ready for review August 27, 2024 12:48
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 27, 2024
@k8s-ci-robot k8s-ci-robot requested a review from detiber August 27, 2024 12:48
Copy link

@niravparikh05 niravparikh05 left a comment

Choose a reason for hiding this comment

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

lgtm

Signed-off-by: rahulii <r.sawra@gmail.com>
@displague
Copy link
Member

This draft PR implements the API changes for design described in #786 (L2 design). This branch is being used to work through the design, build intuition, and gain early insights and feedback.

Converting to draft. This can't be merged until the controllers can meaningfully reconcile the proposed CRD changes and provision Equinix Metal devices to fit the desired spec.

@displague displague marked this pull request as draft August 29, 2024 15:21
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 29, 2024
Signed-off-by: rahulii <r.sawra@gmail.com>
Signed-off-by: rahulii <r.sawra@gmail.com>
@rahulii rahulii changed the title API and CRD changes for adding layer2 support feat(): layer2 support for CAPP Sep 24, 2024
Signed-off-by: rahulii <r.sawra@gmail.com>
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 25, 2024
Signed-off-by: rahulii <r.sawra@gmail.com>
Copy link

@nirav-rafay nirav-rafay left a comment

Choose a reason for hiding this comment

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

few minor comments

// If not specified, the first available IP address from the IP address range will be assigned.
// This is useful when you want to reserve some IP addresses for other purposes for eg Gateways, DNS etc.
// +optional
AssignmentRange string `json:"assignmentRange,omitempty"`

Choose a reason for hiding this comment

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

can you specify the expected format here ? Also add a validation if the format in valid.

Also can a user configure multiple address ranges as below ?
e.g. 10.60.10.2-10.60.10.9, 10.60.10.11-10.60.10.20

}

func (c *Config) GetTemplate() (string, error) {
tmpl, err := template.New("layer-2-user-data").Parse(configTemplate)

Choose a reason for hiding this comment

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

probably mode this under NewConfig(), we need to initialize and parse the template only once.

And move the tmpl.Execute as a separate utility method.

// GenerateInitDocument generates a multipart document with cloud-config data
func GenerateInitDocument(rawUserData, layer2UserData string) (string, error) {
var buf bytes.Buffer
mpWriter := multipart.NewWriter(&buf)

Choose a reason for hiding this comment

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

Suggested change
mpWriter := multipart.NewWriter(&buf)
mpWriter := multipart.NewWriter(&buf)
defer mpWriter.Close()


// execute the template and save the output to a buffer
var output bytes.Buffer
if err := tmpl.Execute(&output, c); err != nil {

Choose a reason for hiding this comment

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

support for adding route configuration to user data script

)

// GenerateInitDocument generates a multipart document with cloud-config data
func GenerateInitDocument(rawUserData, layer2UserData string) (string, error) {

Choose a reason for hiding this comment

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

let's make this more generic, something like append input user data and multiple clould config could be added as deemed necessary

@niravparikh05
Copy link

niravparikh05 commented Sep 26, 2024

@rahuli few noted todo's

Move the layer2 user data template to the default flavor PacketMachineTemplate spec found in templates/cluster-template.yaml
Opportunity to use generic ipam provider instead of writing it on our own

Signed-off-by: rahulii <r.sawra@gmail.com>
Copy link

@nirav-rafay nirav-rafay left a comment

Choose a reason for hiding this comment

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

@rahulii looks fine overall, few comments on making this approach more generic.

@@ -8,5 +8,5 @@ spec:
spec:
containers:
# Change the value of image field below to your controller image URL
- image: quay.io/equinix-oss/cluster-api-provider-packet:dev
- image: docker.io/rahulsawra/cluster-api-provider-packet-amd64:dev

Choose a reason for hiding this comment

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

keep this for your local testing only

@@ -8,4 +8,4 @@ spec:
spec:
containers:
- name: manager
imagePullPolicy: IfNotPresent
imagePullPolicy: Always

Choose a reason for hiding this comment

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

local testing only

Comment on lines +118 to +132
if bootstrapStripped != "" {
if err := yaml.Unmarshal([]byte(bootstrapStripped), &bootstrapConfig); err != nil {
return "", fmt.Errorf("error parsing bootstrap YAML: %v", err)
}
} else {
bootstrapConfig = make(map[string]interface{})
}

if layer2Stripped != "" {
if err := yaml.Unmarshal([]byte(layer2Stripped), &layer2UserDataConfig); err != nil {
return "", fmt.Errorf("error parsing layer2 YAML: %v", err)
}
} else {
layer2UserDataConfig = make(map[string]interface{})
}

Choose a reason for hiding this comment

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

bootstrapConfig := make(map[string]interface{})

if err := yaml.Unmarshal([]byte(layer2Stripped), &layer2UserDataConfig); err != nil {
	return "", fmt.Errorf("error parsing layer2 YAML: %v", err)
}

layer2UserDataConfig := make(map[string]interface{})

if err := yaml.Unmarshal([]byte(layer2Stripped), &layer2UserDataConfig); err != nil {
	return "", fmt.Errorf("error parsing layer2 YAML: %v", err)
}


for i, line := range lines {
trimmedLine := strings.TrimSpace(line)
switch trimmedLine {

Choose a reason for hiding this comment

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

this assumes there will not be any other header !

Can we modify this to be more generic - is there a way to identify headers apart from string equals

}

// MergeConfigs combines bootstrap data with layer2 config
func (m *CloudConfigMerger) MergeConfigs(bootstrapData string, layer2UserData string) (string, error) {

Choose a reason for hiding this comment

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

can we make this generic as well ?

CloudConfigMerger.WithBootstrapData(bdata).
WithLayer2Data(l2data).
WithAddlUserDatas([]userdatas). // this can be optional and can be extensible
MergeConfigs()

Signed-off-by: rahulii <r.sawra@gmail.com>
Signed-off-by: rahulii <r.sawra@gmail.com>
Signed-off-by: rahulii <r.sawra@gmail.com>
Copy link

linux-foundation-easycla bot commented Oct 11, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 11, 2024
Signed-off-by: rahulii <r.sawra@gmail.com>
// assignVXLAN assigns a VXLAN to a port
func (r *PacketMachineReconciler) assignVXLAN(ctx context.Context, portID, vxlanStr string) error {
_, resp, err := r.PacketClient.PortsApi.AssignPort(ctx, portID).PortAssignInput(metal.PortAssignInput{
Vnid: &vxlanStr,

Choose a reason for hiding this comment

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

let's check if this is anyway to fetch vxlan id from vlan value - for ease of configuration / usability

Copy link
Member

Choose a reason for hiding this comment

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

There are examples here: https://github.com/equinix/terraform-provider-equinix/blob/343c8fc6a8d3674f3067629e146855c1d3d96dbe/internal/resources/metal/port/helpers.go#L145
Either vlan_ids or vxlan_ids are accepted in the Terraform equinix_metal_port resource. The batch port assignment API accepts either format.

https://deploy.equinix.com/developers/api/metal/#tag/Ports/operation/createPortVlanAssignmentBatch

Copy link
Member

@displague displague Oct 16, 2024

Choose a reason for hiding this comment

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

Vnid in the PortAssignInput will also take either format: https://deploy.equinix.com/developers/api/metal/#tag/Ports/operation/assignPort

In Terraform, we had to be careful in some usage so that Terraform wouldn't get into a drift-detected state in places where an Equinix Metal API VLAN ID (UUID) could be returned in response despite a VXLAN ID (1234) value being supplied in the request.

Signed-off-by: rahulii <r.sawra@gmail.com>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 14, 2024
rahulii and others added 9 commits October 14, 2024 13:45
Signed-off-by: rahulii <r.sawra@gmail.com>
Signed-off-by: rahulii <r.sawra@gmail.com>
Signed-off-by: rahulii <r.sawra@gmail.com>
Signed-off-by: rahulii <r.sawra@gmail.com>
Signed-off-by: rahulii <r.sawra@gmail.com>
Signed-off-by: rahulii <r.sawra@gmail.com>
@displague displague marked this pull request as ready for review October 29, 2024 13:03
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 29, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 27, 2025
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle rotten
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 26, 2025
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants