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

Blog: Connecting to a service without SBO #5915

Merged
merged 12 commits into from
Jul 7, 2022

Conversation

valaparthvi
Copy link
Contributor

@valaparthvi valaparthvi commented Jul 4, 2022

Signed-off-by: Parthvi Vala [email protected]

What type of PR is this:
/kind documentation

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #5771

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:
https://deploy-preview-5915--odo-docusaurus-preview.netlify.app/blog/binding-database-service-without-sbo

@netlify
Copy link

netlify bot commented Jul 4, 2022

Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit d5715c8
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/62c680cdf08eae000971e524
😎 Deploy Preview https://deploy-preview-5915--odo-docusaurus-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@odo-robot
Copy link

odo-robot bot commented Jul 4, 2022

Unit Tests on commit 78b8795 finished successfully.
View logs: TXT HTML

@dharmit
Copy link
Member

dharmit commented Jul 4, 2022

The blog's not showing a single instance of odo add binding. How is it justified as a blog that uses odo to connect to a service without SBO? It's using the service discovery mechanism used since a long time - manually populating the environment variables.

@odo-robot
Copy link

odo-robot bot commented Jul 4, 2022

Windows Tests (OCP) on commit 78b8795 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Jul 4, 2022

Kubernetes Tests on commit 78b8795 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Jul 4, 2022

OpenShift Tests on commit 78b8795 finished with errors.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Jul 4, 2022

Validate Tests on commit 78b8795 finished successfully.
View logs: TXT HTML

@valaparthvi
Copy link
Contributor Author

valaparthvi commented Jul 4, 2022

The blog's not showing a single instance of odo add binding. How is it justified as a blog that uses odo to connect to a service without SBO? It's using the service discovery mechanism used since a long time - manually populating the environment variables.

It is not supposed to use SBO to connect, hence there is no odo add binding. The purpose of this blog is to show that you can connect to a service without the help of SBO. You can read the attached issue #5771 for more details.

The importance of this is to remind users that they can use their regular flow with "hardcoding" connection information to env variables in devfile.yaml.

- name: username
value: userAdmin
- name: password
value: userAdmin123456
Copy link
Contributor

@feloy feloy Jul 4, 2022

Choose a reason for hiding this comment

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

This will make a secret written into the Devfile, most probably pushed to some git repository.

I'm not sure we want to document this method, as it is not encouraged at all

We could use some kind of envFrom to point to the secret, but Devfile does not support it for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does devfile support dynamically obtaining values? I mean if we can put kubectl get secrets minimal-secret -ojsonpath='{.data.username}' | base64 -d inside the devfile, we can avoid having to hard code password.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could add a note acknowledging that it's a bad practice, but this is for example purpose only, or I could find a way to fetch password from the code itself to avoid this practice altogether.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you could start with this solution, by indicating from the very beginning this is not a secure solution, and then continue with more secure solutions (and probaly more difficult ones). That would show the interest of using SBO.

  • The simplest solution would be to have an envFrom field in the Devfile's container, but this field does not exist. Is it worth asking Devfile team to add it? I'm not sure
  • You could pass the secret name and the secret keys as environment variables, and the program would be responsible for getting the values from the secret. That means the program needs access to the Kubernetes API, which opens security breaches

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The simplest solution would be to have an envFrom field in the Devfile's container, but this field does not exist. Is it worth asking Devfile team to add it? I'm not sure

When you say envFrom, do you mean devfile can evaluate the value from the environment variable exposed in your local system?

env:
- name: password
  value:
    envFrom: $MONGODB_PASSWORD

Something like this? Or more like this https://raw.githubusercontent.com/kubernetes/website/main/content/en/examples/pods/inject/pod-secret-envFrom.yaml?

You could pass the secret name and the secret keys as environment variables, and the program would be responsible for getting the values from the secret. That means the program needs access to the Kubernetes API, which opens security breaches

I think asking the program to be responsible for fetching everything would defeat the purpose of this blog. I think I'll go with a note for now, and it's like you said, it might encourage users to try SBO.

Copy link
Contributor

@feloy feloy Jul 5, 2022

Choose a reason for hiding this comment

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

Copy link
Member

@rm3l rm3l Jul 5, 2022

Choose a reason for hiding this comment

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

This will make a secret written into the Devfile, most probably pushed to some git repository.

Would it be possible to leverage the variable substitution feature here? I think this would solve the issue while showing this other feature of odo.

Not tested, but calling odo dev with something like this:

# Given that the variables have already been exported
odo dev --var MY_MONGODB_USERNAME --var MY_MONGODB_PASSWORD

What do you think?

Copy link
Contributor Author

@valaparthvi valaparthvi Jul 5, 2022

Choose a reason for hiding this comment

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

But then devfile would have to interact with the k8s API and that does not make a lot of sense to me from the design standpoint. I don't think devfile is dependent on the K8s API right now, and I'm not sure if they'd be willing to create a dependency now. Can we take this up in cabal?

@dharmit
Copy link
Member

dharmit commented Jul 4, 2022

It is not supposed to use SBO to connect, hence there is no odo add binding. The purpose of this blog is to show that you can connect to a service without the help of SBO. You can read the attached issue #5771 for more details.

Yeah, I read that issue right after commenting. It seems like we only want to show that odo can create a Deployment, and a Pod of that Deployment can talk to a Service using env vars. Anyway, it seems like that's what @kadel wants to showcase.

However, the flow here is not the same as:

$ odo init
$ odo dev

# Now create a service (using helm or operators)
# Next modify the existing devfile to hardcodde service info, and let odo dev do the magic

@dharmit
Copy link
Member

dharmit commented Jul 4, 2022

However, the flow here is not the same as:

$ odo init
$ odo dev

# Now create a service (using helm or operators)
# Next modify the existing devfile to hardcodde service info, and let odo dev do the magic

@valaparthvi please make sure that your blog complies with this. At the moment, odo init is on Step 6. I would prefer it be Step 1 right after cloning the repo and creating a namespace (which should be mentioned as "optional", IMO.)

@dharmit
Copy link
Member

dharmit commented Jul 5, 2022

@valaparthvi thanks for the fixes.

/approve

@openshift-ci
Copy link

openshift-ci bot commented Jul 5, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dharmit

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jul 5, 2022
Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

@valaparthvi Great blog post 👍🏿

I added a few comments below.

Another small comment. The SBO acronym is mentioned all over the place, but I saw nowhere where it was defined. It is clear to us, but might not for readers.
Maybe it could be worth changing the title to something more explicit, like "Binding to a database service without the Service Binding Operator (SBO)".

My 2 cents...

- name: username
value: userAdmin
- name: password
value: userAdmin123456
Copy link
Member

@rm3l rm3l Jul 5, 2022

Choose a reason for hiding this comment

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

This will make a secret written into the Devfile, most probably pushed to some git repository.

Would it be possible to leverage the variable substitution feature here? I think this would solve the issue while showing this other feature of odo.

Not tested, but calling odo dev with something like this:

# Given that the variables have already been exported
odo dev --var MY_MONGODB_USERNAME --var MY_MONGODB_PASSWORD

What do you think?

@valaparthvi valaparthvi requested review from rm3l, dharmit and feloy July 5, 2022 11:41


## Export environment variables
2. Export the necessary environment variables:
Copy link
Contributor

Choose a reason for hiding this comment

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

I find that exporting these variables at this point is premature. We cannot understand what they are for. I think it would be better to introduce the helm chart before to export them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but then if we want to maintain the order of odo init > odo dev > deploy charts > wait for odo dev to detect changes, it becomes necessary to export these variables at this point

Copy link
Contributor

Choose a reason for hiding this comment

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

The order

  • odo init
  • odo dev
  • deploy chart
    is just an example provided by Tomas, I'm not sure it must be respected at all price.

By first deploying the chart (including exportint variables), then running odo init and odo dev, the flow seems more understandable to me

Copy link
Member

Choose a reason for hiding this comment

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

is just an example provided by Tomas, I'm not sure it must be respected at all price.

By first deploying the chart (including exportint variables), then running odo init and odo dev, the flow seems more understandable to me

I like the current approach of enabling Helm repo first. But I don't agree with the idea of spinning up Helm chart using it before doing odo init. If it were me, I would first show that your Go application is deployed using odo dev, but curl request made to it is throwing 404 or some error. Thereafter, we spin up the MongoDB database required by the application.

Or maybe, do an odo dev without all the variables which would help one connect to the MongoDB database. Make a curl call that returns 404 or some error. Now add information about connection to MongoDB and see how the application returns expected output.

I'm saying this because that's how I think an application developer's flow would be like. But then I have not a lot of application development myself, so correct me if I'm mistaken.

Copy link
Contributor Author

@valaparthvi valaparthvi Jul 6, 2022

Choose a reason for hiding this comment

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

Or maybe, do an odo dev without all the variables which would help one connect to the MongoDB database. Make a curl call that returns 404 or some error. Now add information about connection to MongoDB and see how the application returns expected output.

I'm saying this because that's how I think an application developer's flow would be like. But then I have not a lot of application development myself, so correct me if I'm mistaken.

I agree with you, but I can't find a flow that works for everyone. For now, I have added a note stating that odo dev will fail if run before adding connection information(bf76584).

Copy link
Member

Choose a reason for hiding this comment

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

Works for me. I discussed this with @feloy and agree with the flow we have now.

## Deploy the application
4. Run `odo dev` to deploy the application on the cluster.
```sh
odo dev --var PASSWORD=$MY_MONGODB_ROOT_PASSWORD --var USERNAME=$MY_MONGODB_ROOT_USERNAME
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, I think it is premature to pass these variables, as they are not existing in the Devfile yet and we are not using them. I think it would be better to wait for steps 7.x before to do this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I can simply not do odo dev first and put it after deploying the microservice.

@valaparthvi
Copy link
Contributor Author

One last comment. I think it would also be great to include odo list after deploying the Bitnami Helm Chart, just to show that odo can list Helm-managed resources as well. What do you think?

❯ odo list
 ✓  Listing components from namespace 'blog-helm-wo-sbo-5915' [2s]
 NAME       PROJECT TYPE  RUNNING IN  MANAGED 
 * restapi  go            Dev         odo     
 mongodb    Unknown       None        Helm    

I think it might be stirring a little away from the topic at hand. I would rather do that in a separate blog if necessary.

@openshift-ci
Copy link

openshift-ci bot commented Jul 6, 2022

@valaparthvi: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/unit-and-validate-test bf76584 link true /test unit-and-validate-test

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

- DELETE `/api/places/<id>` - Delete place with id `<id>`

## Conclusion
To conclude this blog, it is possible to connect your application with another microservice without the Service Binding Operator if you have the correct connection information. Using the Service Binding Operator with a [Bindable Operator](https://github.com/redhat-developer/service-binding-operator#known-bindable-operators) makes it easy for you to not care about finding the connection information and ease the binding.
Copy link
Contributor

Choose a reason for hiding this comment

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

You could add to the conclusion that it is up to the reader to manage the secrets stored in the environment variables MY_MONGODB_ROOT_PASSWORD and MY_MONGODB_ROOT_USERNAME, so they are accessible every time the following command is executed:

odo dev --var PASSWORD=$MY_MONGODB_ROOT_PASSWORD --var USERNAME=$MY_MONGODB_ROOT_USERNAME --var HOST="mongodb"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added this note in the section where we introduce these variables for the first time, I think it makes more sense to put them there than in the conclusion.

@feloy
Copy link
Contributor

feloy commented Jul 6, 2022

/lgtm

/hold

Feel free to unhold if other reviewers are ok

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. lgtm Indicates that a PR is ready to be merged. Required by Prow. labels Jul 6, 2022
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 7, 2022
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

No Coverage information No Coverage information
0.3% 0.3% Duplication

Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

/hold cancel

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. Required by Prow. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. labels Jul 7, 2022
@valaparthvi
Copy link
Contributor Author

/override OpenShift Integration tests/OpenShift Integration tests

1 similar comment
@feloy
Copy link
Contributor

feloy commented Jul 7, 2022

/override OpenShift Integration tests/OpenShift Integration tests

@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2022

@feloy: Overrode contexts on behalf of feloy: OpenShift Integration tests/OpenShift Integration tests

In response to this:

/override OpenShift Integration tests/OpenShift Integration tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2022

@valaparthvi: Overrode contexts on behalf of valaparthvi: OpenShift Integration tests/OpenShift Integration tests

In response to this:

/override OpenShift Integration tests/OpenShift Integration tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot merged commit 771644e into redhat-developer:main Jul 7, 2022
cdrage pushed a commit to cdrage/odo that referenced this pull request Aug 31, 2022
* Blog: Connecting to a service without SBO

Signed-off-by: Parthvi Vala <[email protected]>

* Dharmit's review

* Dharmit/Philippe's review

* Use devfile variable substitution, and bitnami charts for deploying mongodb service

* More review changes

* Fix netlify failures

* FIx broken links

* Philippe's review

* Show that odo dev will fail before adding connnection information

Signed-off-by: Parthvi Vala <[email protected]>

* Update docs/website/blog/2022-06-30-binding-database-service-without-sbo.md

Co-authored-by: Armel Soro <[email protected]>

* Fix port in the devfile, add note about env vars

* Change port

Co-authored-by: Armel Soro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: add documentation on how to connect to a service without using SBO
5 participants