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

Sps-Service Header Proposal #99

Merged
merged 5 commits into from
Jan 13, 2025
Merged

Sps-Service Header Proposal #99

merged 5 commits into from
Jan 13, 2025

Conversation

nicholas-s-perkins
Copy link
Contributor

@nicholas-s-perkins nicholas-s-perkins commented Dec 12, 2024

@nicholas-s-perkins nicholas-s-perkins requested a review from a team as a code owner December 12, 2024 00:22
@travisgosselin travisgosselin changed the title Sps-User-Agent Header Proposal Sps-Service Header Proposal Dec 19, 2024
Copy link
Contributor Author

@nicholas-s-perkins nicholas-s-perkins left a comment

Choose a reason for hiding this comment

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

I approve the changes, I don't get a button for that 😸

Copy link

@jimthedev jimthedev left a comment

Choose a reason for hiding this comment

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

Hello. Thanks for working on this.

Just a few (perhaps naïve) questions:

  • Would it be fair to assume that the id & name portion of the Sps-Service header should be registered in prod tech registry? If so, would it be worth mentioning in the notes that this as a place where people should go to get that service id and name to at least try to ensure they match?
  • Would the environment of the service making the request be something that might be added into the custom attributes given that it isn't part of the key itself? Perhaps that is actually somewhere else in the spec or maybe is being considered in other specs. I am asking because it at least is possible that in the case of a public endpoint it could get a request from another environment than the one the receiving service is running in. This kind of thing would be tricky to track down but would result in prod data being returned to a test UI or vice versa. Maybe not something for the spec but something to think about when considering what we could log given the header.

Again thanks for doing this.

@travisgosselin
Copy link
Member

Hello. Thanks for working on this.

Just a few (perhaps naïve) questions:

Great feedback @jimthedev - appreciate you taking the time to read through it.

Would it be fair to assume that the id & name portion of the Sps-Service header should be registered in prod tech registry? If so, would it be worth mentioning in the notes that this as a place where people should go to get that service id and name to at least try to ensure they match?

Our API Standards ideally are created agnostic of the implementation and subsystem that sits on top of it and references it. They are also public (like this conversation). So with that in mind, it is recognized that there needs to be an authority for the service ID (in our case at SPS that is tech registry). This line was intending to cover those use cases: Requests **MAY** validate the Sps-Serviceheaderid value against the associated authority or service registry. Validation failures **MUST** return a 400 indicating the reason.

Does that make sense or should we add more / reword?

Would the environment of the service making the request be something that might be added into the custom attributes given that it isn't part of the key itself? Perhaps that is actually somewhere else in the spec or maybe is being considered in other specs. I am asking because it at least is possible that in the case of a public endpoint it could get a request from another environment than the one the receiving service is running in. This kind of thing would be tricky to track down but would result in prod data being returned to a test UI or vice versa. Maybe not something for the spec but something to think about when considering what we could log given the header.

I think of this header as environment agnostic, and specific to the service entity itself. With that in mind, the implementation architecture matters to ensure it is understood what environment the service is from. That being said, the state in the SPS ecosystem is that we have one PROD asset registry used in all environments, our runtime in our integration environment makes use of PRODUCTION service ids as a single source of truth for a lot of reasons I won't enumerate here. Please advise me if you think this is incorrect.

Copy link

@acchristensen acchristensen left a comment

Choose a reason for hiding this comment

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

Looking forward to having this in and folks adopting it!

@jimthedev
Copy link

@travisgosselin Ok I think that makes sense. Thanks for clarifying. I think that rewording makes a lot of sense.

@travisgosselin
Copy link
Member

travisgosselin commented Jan 10, 2025

@travisgosselin Ok I think that makes sense. Thanks for clarifying. I think that rewording makes a lot of sense.

Any suggestions on how to reword it? What terminology you'd like to see to make it clear? I put it through AI to try and get some alternatives, but mostly just a few grammatical changes:

Requests MAY validate the Sps-Service Header Id value against the corresponding authority or service registry. If validation fails, a 400 Bad Request response MUST be returned with an explanation of the error.

Thoughts @jimthedev ?

@travisgosselin travisgosselin merged commit fe8815a into main Jan 13, 2025
4 checks passed
@travisgosselin travisgosselin deleted the sps-user-agent branch January 13, 2025 15:26
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.

5 participants