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

Add dynamic proxy configuration #150

Merged
merged 4 commits into from
Dec 7, 2020

Conversation

iffyio
Copy link
Collaborator

@iffyio iffyio commented Dec 3, 2020

Integrates the XDS client into the cluster manager

Work on #10

Integrates the XDS client into the cluster manager

Work on #10
Copy link
Member

@markmandel markmandel left a comment

Choose a reason for hiding this comment

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

Two small minor things, but otherwise looks really good!

@@ -0,0 +1,29 @@
#
# Copyright 2020 Google LLC All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Should we also add an entry to https://github.com/googleforgames/quilkin/blob/master/docs/proxy-configuration.md while we are at it in this PR?

(Also just noted this page is not referenced from the home page, but that's a separate issue)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah good catch, will fix!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated!

src/proxy/server/mod.rs Show resolved Hide resolved
properties:
filter:
'$ref': '#/definitions/filterchain'
endpoints:
'$ref': '#/definitions/endpoints'
required:
- endpoints
dynamic:
Copy link
Member

@markmandel markmandel Dec 7, 2020

Choose a reason for hiding this comment

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

Should this be a oneOf with children of dynamic and static ? https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/#oneof

Or would that be too complicated for the documentation? (Not for this PR, but maybe we should split this up into separate docs)

Actually, this would be the link of oneOf, yes? http://json-schema.org/draft-04/json-schema-validation.html#rfc.section.5.5 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah ideally it'll be oneof but oneof is a bit weird at the top level, instead of being able to say oneof static or dynamic at the top level, we can only say oneof or which might be more complex

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good - just wanted to check 👍

@markmandel markmandel merged commit 3654bea into master Dec 7, 2020
@markmandel markmandel added kind/feature New feature or request area/operations Installation, updating, metrics etc labels Dec 7, 2020
@markmandel markmandel deleted the iu/dynamic-config-xds-clustermanager branch December 8, 2020 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/operations Installation, updating, metrics etc cla: yes kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants