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.

Sign up for GitHub

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

Jump to bottom

Run the TSB in a separate pod #15672

Merged
merged 2 commits into from
Aug 9, 2017
Merged

Run the TSB in a separate pod #15672

merged 2 commits into from
Aug 9, 2017

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Aug 8, 2017

This completes the work necessary to run the TSB in a pod. We should continue to remove the cruft from the old path, but I've included a template which creates a running TSB server in the openshift-template-service-broker namespace.

@bparees @jim-minter

@openshift-merge-robot openshift-merge-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 8, 2017
@deads2k deads2k mentioned this pull request Aug 8, 2017
Merged
@deads2k
Copy link
Contributor Author

deads2k commented Aug 8, 2017

Build your images and do:

oc adm new-project  openshift-template-service-broker
oc process -f test/testdata/templateservicebroker/templateservicebroker-template.yaml | oc create -f -

@sdodson What does it take to do the equivalent in the installer?

@deads2k deads2k mentioned this pull request Aug 8, 2017
Closed
@sdodson
Copy link
Member

sdodson commented Aug 8, 2017

@sdodson What does it take to do the equivalent in the installer?

Whomever owns TSB should work with @ewolinetz to contribute TSB installation automation to openshift-ansible. Is this optional? Is this installed by default when Service Catalog is enabled? Fully supported in 3.7? need to answer those sort of questions too.

@deads2k deads2k force-pushed the tsb-06-separate branch 3 times, most recently from 1b1fe15 to 4ae0bef Compare August 8, 2017 15:55
@openshift-merge-robot openshift-merge-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 8, 2017
@jim-minter
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 8, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, jim-minter

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@deads2k
Copy link
Contributor Author

deads2k commented Aug 8, 2017

This won't make non-conflicting conflicts

@deads2k
Copy link
Contributor Author

deads2k commented Aug 8, 2017

/test extended_conformance_gce

@deads2k
Copy link
Contributor Author

deads2k commented Aug 8, 2017

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Aug 8, 2017

/test extended_networking_minimal

bparees
bparees reviewed Aug 8, 2017
View reviewed changes
pkg/openservicebroker/server/apiserver.go
} else {
// we're running in cluster
// in this case we actually want to construct our own template informer
// eventually the config value drops out as it isn't supported server side
Copy link
Contributor

Choose a reason for hiding this comment

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

"config value" refers to c.TemplateNamespaces ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"config value" refers to c.TemplateNamespaces ?

tempateinformers

Copy link
Contributor

Choose a reason for hiding this comment

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

so where does the template informer get its config from at that point? and is that something one of your other pulls changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so where does the template informer get its config from at that point? and is that something one of your other pulls changes?

5 lines down in this pull.

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm lost.

In the case where this code is running in a pod (not in the master), you're currently standing up an informer using a client that's created using the in cluster config.

the comment says "eventually the config value drops out". What is meant by eventually, are you referring to a future evolution of this code(my original interpretation), or are you referring to some downstream codepath that dumps the value? And if the latter, what is meant by "drops out"?

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 comment says "eventually the config value drops out". What is meant by eventually, are you referring to a future evolution of this code(my original interpretation),

Future evolution removes this code. See e27e5d5#diff-e73d057d1e253024c42b0fec37e12420L59

bparees
bparees reviewed Aug 8, 2017
View reviewed changes
pkg/template/servicebroker/servicebroker.go
if err != nil {
return nil, err
}
extrouteclientset, err := extrouteclientset.NewForConfig(saKubeClientConfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

externalRouteClient (align w/ other names+case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

externalRouteClient (align w/ other names+case)

pre-existing. happy to do it in a follow on.

@deads2k
Copy link
Contributor Author

deads2k commented Aug 8, 2017 

Extended.[Conformance][networking][router] The HAProxy router should support reencrypt to services backed by a serving certificate automatically
Extended.[Conformance][networking][router] router headers The HAProxy router should set Forwarded headers appropriately
Extended.[Conformance][networking][router] openshift router metrics The HAProxy router should expose a health check on the metrics port

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Aug 8, 2017

Router again. I don't see the relationship. I'll try once more

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Aug 9, 2017

/retest

@soltysh
Copy link
Contributor

soltysh commented Aug 9, 2017

#8571

/test extended_conformance_install_update

soltysh
soltysh reviewed Aug 9, 2017
View reviewed changes
pkg/openservicebroker/cmd/server/start.go
SecureServing *genericoptions.SecureServingOptions
Authentication *genericoptions.DelegatingAuthenticationOptions
Authorization *genericoptions.DelegatingAuthorizationOptions
Audit *genericoptions.AuditOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing this is making our audit harder, of course. Although the advanced audit is designed to work with multiple api server I'm not 100% sure if that was tested ;) Btw. do we care about upstream audit options here. We haven't decided yet which parts of the advanced audit we will enable and this is literally taking what upstream has.

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue

@openshift-merge-robot openshift-merge-robot merged commit 07ec3d2 into openshift:master Aug 9, 2017
@deads2k deads2k deleted the tsb-06-separate branch January 24, 2018 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bparees bparees bparees left review comments

@soltysh soltysh soltysh approved these changes

Assignees

@soltysh soltysh

@jim-minter jim-minter

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. retest-not-required size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@deads2k @sdodson @jim-minter @openshift-merge-robot @soltysh @bparees @openshift-ci-robot