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: input qualification #1695

Merged

Conversation

ml019
Copy link
Contributor

@ml019 ml019 commented May 30, 2021

Intent of Change

  • Refactor (non-breaking change which improves the structure or operation of the implementation)

Description

Re-enable qualification as a step in input processing.

Fixes #1599

Motivation and Context

Support qualification across all forms of input. Reverses #1600

For backwards compatibility, ensure input qualification checks for ids and names of layers. Best practice should be to encourage the use of names and long form qualification but a number of existing cmdbs use ids and short form qualifiers.

This PR also adds some longer form alternatives for the info and warn macros, as well as an alternative fatal macro which always stops processing.

How Has This Been Tested?

Local template generation.

Related Changes

Prerequisite PRs:

  • None

Dependent PRs:

Consumer Actions:

  • None

@ml019 ml019 requested a review from a team May 30, 2021 06:53
@ml019 ml019 self-assigned this May 30, 2021
@ml019 ml019 marked this pull request as draft May 30, 2021 23:30
@ml019
Copy link
Contributor Author

ml019 commented May 30, 2021

@roleyfoley I started a separate PR (hamlet-io/engine-plugin-aws#325) to cleanup the use of qualifiers in the code and found one spot where input pipeline based qualification will fail, which is that the LB component currently includes the port in the qualifiers - https://github.com/hamlet-io/engine-plugin-aws/blob/0eb8c1163e3b53e0f825c26a58658e66c33bff26/aws/components/lb/setup.ftl#L275

I'm assuming that was so the certificate config could be at the component level but then varied per port.

A few alterative ways forward come to mind;

  1. Remove source port qualification assuming it hasn't actually been used anywhere. Are you able to advise examples where port qualification has been used?
  2. Assuming it is in use, include the certificate information in each port in the solution. This would be a breaking change. Input qualification would still allow the certificate to be varied per port e.g. based on environment
  3. input processing qualification leaves any qualifiers behind that it didn't recognise.

The last option is problematic as things like environment would have been removed.

My suggestion is that 2 is the way forward as it still allows per port certificate variation (and qualification).

Thoughts on how to proceed?

@roleyfoley
Copy link
Contributor

I think 2 makes the most sense.
I'm pretty sure I've only be defining certificate details on the port since its also used for the hostfiltering rule on each port so it makes more sense to define it at that level.

So yeah I'd say 2 and a check to see if a component level qualification with port configuration has been defined would cover off the migration for the change.

@ml019
Copy link
Contributor Author

ml019 commented Jun 1, 2021

The input pipeline will strip all the qualification info so there won't be anything to check. While the long form of qualifier would result in a validation error (if run with Ross' validation entrance), I think any current use would be the short form which is pretty hard to validate.

Any other ideas about how to detect? Its kind of a use case for being about to scan all cmdbs for all customers to look for existing usage.

@roleyfoley
Copy link
Contributor

Maybe we store the qualification details in a global variable as part of the qualifcation step?
Then it could be queried or included as part of state ( maybe under an input processing occurrence section )

That would let you see what caused your configuration change and give us access to that kind of thing without tainting the solution config

@ml019
Copy link
Contributor Author

ml019 commented Jun 1, 2021

I think the simplest is to

  • keep the unqualified data as well as the qualified data in the state
  • mark each qualifier in the unqualified data as ignored or applied as it is processed
  • expose the unqualified data so custom entrances can show where qualification was applied, or a flow could incorporate the qualifier info into the occurrence structure.

While this doesn't explicitly provide a warning re source ports, it would provide more general tooling to sort out why a qualifier wasn't applied.

Is that workable?

I think qualification anywhere will prove very useful but having tools to debug will be important if we increase usage of qualifiers in cmdbs and reduce the number of files needed as a result.

@ml019
Copy link
Contributor Author

ml019 commented Jun 1, 2021

Latest commit includes an annotated copy of the pre-qualified state showing which qualifiers matched and which ones didn't. I could optionally include this based on log level if we think it will chew up too much memory but my feeling is we leave that until memory starts to become an issue.

Re-enable qualification

Fixes hamlet-io#1599

Reverses PR hamlet-io#1600

For backwards compatability, ensure input qualification checks for
ids and names of layers. Best practice should be to encourage the use of
names and long form qualification but a number of existing cmdbs use
ids and short form qualifiers.

This PR also adds some longer form alternatives for the `info` and `warn`
macros, as well as an alternative `fatal` macro which always stops
processing.

A followup PR will remove the use of specific checks for qualification
via the segmentQualifiers global variable.
Add an annotated copy of the state to the inputState showing which
qualifiers were or weren't applied.

This is intended to form the basis of tooling to assist users determine
the effect of the provided qualifiers where the results produced are
unexpected.
@ml019 ml019 force-pushed the fix-dynamic-cmdb-domain-qualification branch from 8f9868b to be1f5c9 Compare June 9, 2021 22:34
@ml019 ml019 marked this pull request as ready for review June 9, 2021 22:37
@ml019
Copy link
Contributor Author

ml019 commented Jun 9, 2021

I will need this for upcoming deployments in HA.

I think the inclusion of the unqualified value in the state is enough for now and would let a future PR include this information where required, for instance in the occurrence structure.

@roleyfoley roleyfoley merged commit df57585 into hamlet-io:master Jun 10, 2021
@ml019 ml019 deleted the fix-dynamic-cmdb-domain-qualification branch June 10, 2021 05:06
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.

[BUG] Domain Qualificiation failing
2 participants