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

azurerm_kubernetes_cluster - maintenance_window_* added #21760

Merged
merged 26 commits into from
Jun 28, 2023

Conversation

aristosvo
Copy link
Collaborator

@aristosvo aristosvo commented May 11, 2023

Fixes #21722

TODO

  • (Extra) Acceptance Tests
  • Decide on the Considerations (especially 2 and 3)
  • Docs

Considerations

1 or multiple blocks:

resource "azurerm_kubernetes_cluster" "example" {
  ...
  // option 1:
  maintenance_window {
    type = "default"
    ...
  }
  
  // option 2:
  // default
  maintenance_window {
    ...
  }
  // autoUpgrade
  maintenance_window_auto_upgrade {
    ...
  }
  // nodeOs
  maintenance_window_node_os {
    ...
  }
}

The default maintenance window type (the "legacy" type, according to the docs) has a different model compared to the other two types. Supporting these 2 different models in one block type is a bit messy from a user perspective, as we cannot validate it as extensively as when we differentiate (option 2). Option 1 will result in a bit more confusion and questions, option 2 is more clear and directing.

In the new maintenance window type there are multiple frequencies possible (Daily, Weekly, AbsoluteMonthly and RelativeMonthly), which in itself are already adding a lot of parameters and confusion. That adds up to the before mentioned preference for option 2.

This means that I've chosen option 2 for now 🙈

Frequency schedules as separate blocks vs flat structure

resource "azurerm_kubernetes_cluster" "test" {
  ...
  // option 1 (implemented)
  maintenance_window_node_os {
    frequency = "AbsoluteMonthly"
    interval  = 1
    duration  = 9

    day_of_month = 5
    start_time   = "07:00"
    utc_offset   = "+01:00"
    start_date   = "2023-11-26T00:00:00Z"

    not_allowed {
      end   = "2023-11-30T00:00:00Z"
      start = "2023-11-26T00:00:00Z"
    }
  }

  // option 2  (more like `azurerm_monitor_alert_processing_rule_suppression`)
  maintenance_window_node_os {
    // frequency options, only 1 allowed at a time
    daily {
      interval = 1
    }
    weekly {
      interval    = 1
      day_of_week = "Monday"
    }
    relative_monthly {
      interval    = 1    
      day_of_week = "Monday"
      week_index  = "First"
    }
    absolute_monthly {
      interval     = 1
      day_of_month = 5
    }

    duration   = 9
    start_time = "07:00"
    utc_offset = "+01:00"
    start_date = "2023-11-26T00:00:00Z"

    not_allowed {
      end   = "2023-11-30T00:00:00Z"
      start = "2023-11-26T00:00:00Z"
    }
  }
}

I'd prefer option 2, as is makes validation better and mimics the API more closely.

Date, time: what are the standards?

We do have standards for date time, but what to use in case of date and time? I feel like it is a bit mixed across the provider? I've modeled dates now as datetime, but that doesn't sound like a great solution in the long run

@aristosvo aristosvo marked this pull request as ready for review May 26, 2023 06:46
@stephybun stephybun self-assigned this Jun 1, 2023
Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @aristosvo. I agree with the design decisions you've made regarding separate blocks for each type as well as flattening the schedule properties within the block, definitely looks cleaner and reads easier within the code. I have some very minor suggestions about the log messages but overall this looks good!

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Tests look good - think just the docs are missing now.

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks for this @aristosvo LGTM 💾

@stephybun stephybun merged commit b915ee4 into hashicorp:main Jun 28, 2023
14 checks passed
@github-actions github-actions bot added this to the v3.63.0 milestone Jun 28, 2023
stephybun added a commit that referenced this pull request Jun 28, 2023
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for newly introduced planned maintenance configuration types for AKS cluster upgrades
2 participants