Skip to content

Add sshdconfig get #1004

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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Add sshdconfig get #1004

wants to merge 21 commits into from

Conversation

tgauth
Copy link
Collaborator

@tgauth tgauth commented Jul 24, 2025

PR Summary

  • add get support to sshdconfig resource:
    • if no input is provided, get returns all settings from SSHD -T including defaults
    • if input is provided, get returns the subset of settings from SSHD -T included in the input
    • if includeDefaults: false is passed in via _metadata, get returns the subset of settings from SSHD -T that are not defaults
    • if filepath: <filepath> is passed in via _metadata, get runs SSHD -T -f <filepath>
  • add pester tests for sshdconfig get and export
  • update resource schema to allow additional properties

@tgauth tgauth requested review from SteveL-MSFT and Copilot July 25, 2025 19:50
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds comprehensive get functionality to the sshdconfig resource, enabling retrieval of SSH server configuration settings with flexible filtering options.

  • Add get command support to retrieve SSH server settings from sshd -T output
  • Implement metadata-based filtering to exclude defaults and specify custom config files
  • Add comprehensive Pester tests for the new get and export functionality

Reviewed Changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
sshd_config.dsc.resource.json Adds get/set command definitions and enables additional properties in schema
src/util.rs Adds command argument structures and utility functions for metadata extraction and default filtering
src/main.rs Updates get command to accept input parameter
src/get.rs Implements complete get functionality with filtering and metadata support
src/export.rs Refactors export to support reusable map generation
src/error.rs Adds IO and persist error types, removes unused NotImplemented error
src/args.rs Adds input parameter to get command
locales/en-us.toml Updates localization strings for new functionality
Cargo.toml Adds tempfile dependency
dsc/tests/dsc_sshdconfig.tests.ps1 Adds comprehensive Pester tests for get and export operations

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@michaeltlombardi
Copy link
Collaborator

It's a bit late, since the implementation is already so far along, but I want to address a few things from the design perspective:

  • if no input is provided, get returns all settings from SSHD -T including defaults
  • if input is provided, get returns the subset of settings from SSHD -T included in the input
  • if defaults: false is passed in via _metadata, get returns the subset of settings from SSHD -T that are not defaults
  • if filepath: <filepath> is passed in via _metadata, get runs SSHD -T -f <filepath>

The differing behavior for input/no-input in the get operation here is a break from prior design, user expectation, and integration with higher order tooling. Typically, get operations return the full configurable state for a resource instance. This filtering makes more sense to me for the export operation. This design also prevents me from retrieving the full configuration but only verifying or enforcing a specific subset.

Similarly, here we seem to be defining write-only properties (behavior-modifying parameters) to be passed through _metadata. Arguably, filepath is a non-required key property - I would expect a configuration document containing any two instances of this resource with the same filepath (or filepath not being defined) to raise a conflict in the future.

@SteveL-MSFT
Copy link
Member

@michaeltlombardi seems like we should have a meeting to discuss this before we commit this change

@tgauth
Copy link
Collaborator Author

tgauth commented Jul 29, 2025

@michaeltlombardi seems like we should have a meeting to discuss this before we commit this change

I'll set it up!

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.

3 participants