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

Cherry-pick #2024 protocol port-range constraints hotfix into main for future release #2071

Conversation

aj-stein-gsa
Copy link

@aj-stein-gsa aj-stein-gsa commented Nov 13, 2024

Committer Notes

This PR is very simple and only cherry-picks a commit. This PR's commit has already been reviewed, approved, and merged into the develop branch, as identified by @brian-ruf in #2024 (comment) to fix #2023, would need to be "promoted" with a variety changes that may not be read for a final release, and if so, a minor or major release, not a patch release (per the NIST OSCAL Team's semantic versioning guidelines in their wiki).

By accepting this cherry-picked commit, NIST OSCAL maintainers could later accept this PR, they could "re-play" after a patch release by "rebasing" the main or release-1.1 branch as prudent on develop with git fetch origin && git checkout --track origin/develop && git pull -r origin main (as suggested by the wiki procedures, just not develop branch). A cherry-pick detection should drop the commit as already included in that branch, and this PR will facilitate quick inclusion into a possible upcoming patch release.

/cc @david-waltermire for awareness

All Submissions:

By submitting a pull request, you are agreeing to provide this contribution under the CC0 1.0 Universal public domain dedication.

(For reviewers: The wiki has guidance on code review and overall issue review for completeness.)

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable? This repo does not contain model-based/instance-based testing; we will update the GSA FedRAMP Automation Team's test suite accordingly for public review and ongoing use.
  • Have you included examples of how to use your new feature(s)? Not in this repository, but will work with GSA FedRAMP Automation Team to update our constraints and examples accordingly.
  • Have you updated all OSCAL website and readme documentation affected by the changes you made? Changes to the OSCAL website can be made in the docs/content directory of your branch.

@aj-stein-gsa aj-stein-gsa requested a review from a team as a code owner November 13, 2024 01:38
@iMichaela
Copy link
Contributor

@aj-stein-gsa - Port range error has been fixed. The fix is already in the develop branch

@aj-stein-gsa
Copy link
Author

@aj-stein-gsa - Port range error has been fixed. The fix is already in the develop branch

Yes, by definition I took that commit and cherry-picked it. I would encourage you to re-read my explanation of why I took that commit and ask you apply it to main and rebase develop accordingly for a patch release, and not push changes from develop to main.

@aj-stein-gsa
Copy link
Author

Also, @iMichaela, no one has approved the workflow so I cannot check the tests and ensure that this PR will work.

@iMichaela
Copy link
Contributor

Also, @iMichaela, no one has approved the workflow so I cannot check the tests and ensure that this PR will work.

Please submit them against the develop branch. But the protocol range error is already fixed in develop.

@aj-stein-gsa
Copy link
Author

aj-stein-gsa commented Nov 13, 2024

Also, @iMichaela, no one has approved the workflow so I cannot check the tests and ensure that this PR will work.

Please submit them against the develop branch. But the protocol range error is already fixed in develop.

Did you read explicitly what I wrote in the PR message and the related mailing list discussion about hotfix and following NIST OSCAL release guidance? It may seem redundant, but it is important. If you are not willing to accept this PR and accept my proposed plan, please let me know. I will close this PR and plan accordingly.

@iMichaela
Copy link
Contributor

iMichaela commented Nov 13, 2024

Also, @iMichaela, no one has approved the workflow so I cannot check the tests and ensure that this PR will work.

Please submit them against the develop branch. But the protocol range error is already fixed in develop.

Did you read explicitly what I wrote in the PR message and the related mailing list discussion about hotfix and following NIST OSCAL release guidance? It may seem redundant, but it is important. If you are not willing to accept this PR and accept my proposed plan, please me know. I will close this PR and plan accordingly.

@aj-stein-gsa -- Sorry - missed that detail about cherry-picking the commit from NIST develop to main. We have other work in develop that is equally important. Why not including it all in the patch? There are many dependabot and other fixes we need.
Also the CI/CD pipeline will fail due to a link that I need to fix as soon as I am done with oscal-content updates.

@aj-stein-gsa
Copy link
Author

Also, @iMichaela, no one has approved the workflow so I cannot check the tests and ensure that this PR will work.

Please submit them against the develop branch. But the protocol range error is already fixed in develop.

Did you read explicitly what I wrote in the PR message and the related mailing list discussion about hotfix and following NIST OSCAL release guidance? It may seem redundant, but it is important. If you are not willing to accept this PR and accept my proposed plan, please me know. I will close this PR and plan accordingly.

@aj-stein-gsa -- Sorry - missed that detail about cherry-picking the commit from NIST develop to main. We have other work in develop that is equally important. Why not including it all in the patch? There are many dependabot and other fixes we need. Also the CI/CD pipeline will fail due to a link that I need to fix as soon as I am done with oscal-content updates.

There are elements to my recommendation, one general, one specific.

  1. Generally, I am following the exact guidance the team wrote. It specifically identies this scenario and the existence of the release-X.Y branch for such use in patch releases.
  2. Specifically, there are a series of changes to the XSLT that resolves entities for Metaschema modules (those on February 26, 2024) and updates to the underlying metaschema-xslt dependency submodule via dependabot. As we do not directly use those tools for profile resolutions and I do not have an efficient way to test them, I cannot be sure those bug fixes, for tools used for use cases FedRAMP does not directly support. This introduces some risk to your proposed release I cannot guarantee, test, or support them with a high-level of confidence. If there are issues with profile resolution in the oscal-content repository or other unforseen affects, I cannot easily volunteer to coordinate and support resolution with your team.

@iMichaela iMichaela added the Waiting for Action Waiting for an external action to be taken label Nov 14, 2024
@iMichaela
Copy link
Contributor

Also, @iMichaela, no one has approved the workflow so I cannot check the tests and ensure that this PR will work.

Please submit them against the develop branch. But the protocol range error is already fixed in develop.

Did you read explicitly what I wrote in the PR message and the related mailing list discussion about hotfix and following NIST OSCAL release guidance? It may seem redundant, but it is important. If you are not willing to accept this PR and accept my proposed plan, please me know. I will close this PR and plan accordingly.

@aj-stein-gsa -- Sorry - missed that detail about cherry-picking the commit from NIST develop to main. We have other work in develop that is equally important. Why not including it all in the patch? There are many dependabot and other fixes we need. Also the CI/CD pipeline will fail due to a link that I need to fix as soon as I am done with oscal-content updates.

There are elements to my recommendation, one general, one specific.

  1. Generally, I am following the exact guidance the team wrote. It specifically identies this scenario and the existence of the release-X.Y branch for such use in patch releases.
  2. Specifically, there are a series of changes to the XSLT that resolves entities for Metaschema modules (those on February 26, 2024) and updates to the underlying metaschema-xslt dependency submodule via dependabot. As we do not directly use those tools for profile resolutions and I do not have an efficient way to test them, I cannot be sure those bug fixes, for tools used for use cases FedRAMP does not directly support. This introduces some risk to your proposed release I cannot guarantee, test, or support them with a high-level of confidence. If there are issues with profile resolution in the oscal-content repository or other unforseen affects, I cannot easily volunteer to coordinate and support resolution with your team.

Those XSLT fixes you mention were implemented and tested by @wendellpiez and @galtm and are focussing on tests we included in the CI/CD pipeline.

Are you implying that FedRAMP will freez the OSCAL version used at v1.1.3, if I cherry pick from develop and release the rest right after as 1.1.4, because all fixes will be released regardless of the patch number? I am sure #2059 is of high interest as well... How come nobody cares of this bug at FedRAMP.

The profile resolution PR is very old and not approved and merged for the reason you stated above. It requires a lot of reviews and tests.

@aj-stein-gsa
Copy link
Author

aj-stein-gsa commented Nov 14, 2024

Those XSLT fixes you mention were implemented and tested by @wendellpiez and @galtm and are focussing on tests we included in the CI/CD pipeline.

Yes, and the underlying metaschema-xslt submodule change, in addition to tests, brings in other changes to profile resolution.

Are you implying that FedRAMP will freez the OSCAL version used at v1.1.3, if I cherry pick from develop and release the rest right after as 1.1.4, because all fixes will be released regardless of the patch number? I am sure #2059 is of high interest as well... How come nobody cares of this bug at FedRAMP.

I would prefer we keep comments objective and not subjective, is that OK? I am not trying to imply anything. I did not say, personally or representing FedRAMP anything about a "version freeze." We will evaluate a minimally required version as the need arises.

Subjectively, It is not an issue of who or who does not care about the bug at FedRAMP. I do not want to address questions from that perspective, it is personal and does not really support you or the community if I keep framing work items and discussions that way.

To directly answer your question about 1.1.3 and freezes in an objective way: if changes are useful in 1.1.3, we will evaluate, if there are separate changes in 1.1.4 we will evaluate, and determine the minimally required version for FedRAMP use from there. To not leave anything to inference and implication: I did not write the proposal nor do we plan on only using 1.1.3 and freezing the version if this proposal is followed through. We will evaluate new versions as needed to inform FedRAMP requirements.

Objectively, I commented on this #2059 issue just now, and I had not reviewed it before. I commented because I see there is no bug fix in a PR or elsewhere. Please correct me if I am wrong. It would seem it is not ready for this proposal.

To speak about the current state of work for the FedRAMP Automation Team, our focus is currently on SSP and then we may move onto other models. Speaking for myself and someone prioritizing that backlog, that is the only reason I have not looked upstream for issues in the AP/AR models. It is not for lack of caring or other subjective reasons. It is just not our focus right now.

The profile resolution PR is very old and not approved and merged for the reason you stated above. It requires a lot of reviews and tests.

I am not speaking of the unmerged PR, I am talking specifically about this submodule update.

ed90047

It may be minor, but I have said every time you asked, without implication, that FedRAMP does not directly use this part of the codebase and I cannot reliably test or review. That is the only reason I wanted to follow published release guidance (which has not changed and the requests, despite worthwhile explanation, deviate from it). As a courtesy, I have tried to make that clear, but I will stop.

@wendellpiez
Copy link
Contributor

wendellpiez commented Nov 14, 2024

Any updates or changes made to profile resolution XSLTs are not in metaschema-xslt submodule, but in oscal repository.

https://github.com/usnistgov/OSCAL/tree/main/src/utils/resolver-pipeline

(I doubt not there are tools to compare this in different branches?)

What @aj-stein-gsa is still true, however, only more so. Without regression testing for profile resolver (somewhere stuck behind develop in any case very old tech debt) you can only ensure its correctness by trusting the devs, or seeing for yourselves.

As one of the devs I strongly recommend both, but if you have to pick one, not trusting the devs and seeing for yourselves is better.

This goes for both profile resolution in this repo, and all metaschema-xslt dependents. It too has much more unit testing and regression testing than formerly, but is still a work in progress in this area.

@wendellpiez
Copy link
Contributor

Changes in the metaschema-xslt submodule are mostly not directly impactful (more tests), but there are meaningful changes as requested, including nominal support for choice in JSON Schemas (which had been skipped for understandable but no-longer-defensible reasons).

In a mature system we would already have regression testing in place to ensure, e.g., we were not breaking schema production with a release. Here we have the beginnings of that. However, all the regression testing in the world does not create trust. We also need transparency and multiple assurances at the various layers.

Accordingly, OSCAL also has its own nascent regression testing for schemas or some schemas (by showing me make, Nikita led the way opening probability-space for us to do this) ensuring if not that they are 'correct' at least they appear plausible and run without error. But again, lacking transparency as to how these tests are run and what they mean, this is not as helpful as it should be.

Additionally, there is also work in my demo site oscal-xproc3 framing up 'schema field testing'. Any schema in any branch or release could be targeted for testing.

So the deficit here (IMHO) is not in the testing as much as it is in the replication and "V&V" (validation/verification) of the testing we have. Until we can replicate and expose the testing (across multiple test sites), we can't actually know how much of a deficit there is, to say nothing of validating new tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Waiting for Action Waiting for an external action to be taken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error with port range definition in Component-def
3 participants