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

Feat(eos_cli_config_gen): Add key_type for ntp.authentication_keys #2258

Merged
merged 7 commits into from Nov 10, 2022

Conversation

philippebureau
Copy link
Contributor

@philippebureau philippebureau commented Nov 3, 2022

Change Summary

adding key_type under ntp in eos_cli_config_gen
To prevent impact on existing implementations, the current behaviour will be preserved until AVD 4.0.0 (see release notes)
Starting with AVD 4.0.0, if no key type is defined, type 7 will be default.

Related Issue(s)

Fixes #2211

Component(s) name

arista.avd.eos_cli_config_gen

Proposed changes

Add a variable to define ntp encryption key type and define with type 7, 8a and no key for ntp authentication keys

example ( key 1 will use no key (old behaviour), key 2 type 7 and key 3 type 8a)

  authentication_keys:
    - id: 1
      hash_algorithm: "md5"
      key: "7 044F0E151B"
    - id: 2
      hash_algorithm: "md5"
      key: "044F0E151B"
      key_type: 7
    - id: 3
      hash_algorithm: "sha1"
      key: "$BYk2Sjahe+D9T7uDgIItSA==$JTw5JOAPcYEo0O2hsvsxFQ==$C7wmpXOo"
      key_type: 8a

How to test

tested without key_type set to confirm type 7 is used and test with other values

output in molecule:

ntp authentication-key 1 md5 7 044F0E151B
ntp authentication-key 2 md5 7 044F0E151B
ntp authentication-key 3 sha1 8a $BYk2Sjahe+D9T7uDgIItSA==$JTw5JOAPcYEo0O2hsvsxFQ==$C7wmpXOo

Checklist

User Checklist

  • N/A

Repository Checklist

  • My code has been rebased from devel before I start
  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation and documentation have been updated accordingly.
  • I have updated molecule CI testing accordingly. (check the box if not applicable)

@philippebureau philippebureau requested a review from a team as a code owner November 3, 2022 15:37
@github-actions github-actions bot added role: eos_cli_config_gen issue related to eos_cli_config_gen role state: CI Updated CI scenario have been updated in the PR labels Nov 3, 2022
@carlbuchmann carlbuchmann changed the title adding ntp key_type Fix: ntp authentication_keys not applied with the right type Nov 4, 2022
Copy link
Contributor

@ClausHolbechArista ClausHolbechArista left a comment

Choose a reason for hiding this comment

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

While I agree this is a fix since the documentation is misleading, this is also a breaking change for ppl who have worked around the issue by using a clear-text password or by inserting 7 before their password.
@carlbuchmann
Maybe worth postponing to 4.x or at the very least call it out in more detail in release-notes?
Please add a description to draft release-notes as part of this PR.
Alternatively we could remove the default 7 in this PR, and only add the type if it is specifically set. Then the behavior would be 1:1 for existing deployments. We can then add the default in 4.0.

@github-actions github-actions bot added the type: documentation Improvements or additions to documentation label Nov 7, 2022
@philippebureau
Copy link
Contributor Author

updated the ntp.j2 template with @ClausHolbechArista inputs

Co-authored-by: Carl Buchmann <carl.buchmann@arista.com>
@ClausHolbechArista ClausHolbechArista requested a review from a team November 10, 2022 08:32
@ClausHolbechArista ClausHolbechArista changed the title Fix: ntp authentication_keys not applied with the right type Feat(eos_cli_config_gen): Add key_type for ntp.authentication_keys Nov 10, 2022
Copy link
Contributor

@gmuloc gmuloc left a comment

Choose a reason for hiding this comment

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

some small comment that can be ignored if you feel that's enough.

LGTM


- **NTP authentication keys variables:**

- `authentication_key.key_type` introduced in 3.8.0 allow you to define the authentication key type. If the key type is not defined, the previous behavior will be preserved. The key type will be set to 7 as default starting in 4.0.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth stating what the previous behavior is in this otherwise as a common user I don't know where to look for it.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's enough for this context - we will capture more info on this in 4.0 when behavior changes.

@carlbuchmann carlbuchmann merged commit a43cd97 into devel Nov 10, 2022
@carlbuchmann carlbuchmann deleted the NTP_KEY_TYPE branch November 10, 2022 13:52
mayurgs34 pushed a commit to Shivani-chourasiya/ansible-avd that referenced this pull request Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rn: Feat(eos_cli_config_gen) role: eos_cli_config_gen issue related to eos_cli_config_gen role state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated type: documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ntp authentication_keys not applied with the right type
4 participants