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

Do not write YAML tags to instance and bindings #73

Closed
wants to merge 1 commit into from

Conversation

felixzieger
Copy link
Contributor

@felixzieger felixzieger commented Feb 21, 2022

I try to solve #69. However, the the YAMLGenerator config option to disable types tagging inlines tags. For originatingIdentity, we end up with two platform keys, which makes the resulting the YAML invalid. Therefore this PR is marked as draft.

Copy link
Member

@JohannesRudolph JohannesRudolph left a comment

Choose a reason for hiding this comment

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

for originatingIdentity, we end up with two platform keys,

Not sure what you want to say with that. From the code in ServiceBinding.kt and ServiceInstance.kt it looks like unipipe broker simply "echos" whatever values it receives from the platform (marketplace) without performing any further validation.

According to the OSB spec https://github.com/openservicebrokerapi/servicebroker/blob/master/profile.md#originating-identity-header the platform value should be a proper platform identifier string like cloudfoundry or meshmarketplace (ref:

our testdata which uses null and http://localhost:8080/serviceRegistry/location/1 is wrong here - maybe that's the problem?

val yamlMapper = ObjectMapper(YAMLFactory())
.registerKotlinModule()
val yamlMapper = ObjectMapper(YAMLFactory()
.configure(YAMLGenerator.Feature.USE_NATIVE_TYPE_ID,false) // Set to false, so that types are tagged inline
Copy link
Member

Choose a reason for hiding this comment

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

s: extract var for YamlFactory - having complex logic in method calls makes code hard to read (and extra tough to debug if you ever need to step through it)

@DorukAkinci
Copy link
Contributor

DorukAkinci commented Feb 25, 2022

I believe we should wait this PR to release our next version. This is also important for the new credentials handling if users choose the terraform handlers for their service instances and bindings. Our most of users use terraform on their OSB. There is a hidden prerequisite(replacing these inline tags with bash script) at the moment and if we cannot release this PR soon probably we should document that this is not a mandatory value and they can remove it safely from their yaml files to process them.

@felixzieger
Copy link
Contributor Author

felixzieger commented Feb 25, 2022

for originatingIdentity, we end up with two platform keys,

Not sure what you want to say with that. From the code in ServiceBinding.kt and ServiceInstance.kt it looks like unipipe broker simply "echos" whatever values it receives from the platform (marketplace) without performing any further validation.

According to the OSB spec https://github.com/openservicebrokerapi/servicebroker/blob/master/profile.md#originating-identity-header the platform value should be a proper platform identifier string like cloudfoundry or meshmarketplace (ref:

our testdata which uses null and http://localhost:8080/serviceRegistry/location/1 is wrong here - maybe that's the problem?

The problem is, that the platform key is shown twice in the output with the YAMLGenerator config I tried out. (When I run the test createServiceInstance creates expected yaml)

image

@DorukAkinci
Copy link
Contributor

DorukAkinci commented Feb 28, 2022

I have figured out this logic. Basically the model automatically puts the first parameter of the originatingIdentity header into the base64 json model as platform key also you can see in the Context constructor function. If you duplicate the value in the base64 format then you see 2 lines for the same record. you can also set 2 different values if you pass different parameters in the header itself and base64. We should check our test cases. I replicated this with our current latest release.

image

In your case, this is possible if you use the following header
-H 'X-Broker-API-Originating-Identity: PlatformContext somenicebase64encoding\ ( the base64 part's platform variable is null )

image

@JohannesRudolph JohannesRudolph deleted the feature/no-yaml-type-tags branch June 24, 2024 09:21
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