Skip to content

Define CRDs for HTTPAPI and Quota spec and distribute to proxies#1845

Merged
ayj merged 6 commits intoistio:masterfrom
ayj:distribute-mixerclient-spec-quota
Nov 27, 2017
Merged

Define CRDs for HTTPAPI and Quota spec and distribute to proxies#1845
ayj merged 6 commits intoistio:masterfrom
ayj:distribute-mixerclient-spec-quota

Conversation

@ayj
Copy link
Copy Markdown
Contributor

@ayj ayj commented Nov 23, 2017

  1. create corresponding CRD for HTTPAPISpec, HTTPAPISpecBinding, QuotaSpec, and QuotaSpecBinding (see https://github.com/istio/api/blob/master/mixer/v1/config/client/api_spec.proto)

  2. add corresponding validation code for new model types

  3. update istioConfigStore to select HTTPAPISpec and QuotaSpec config associated with destination service instances using the corresponding HTTPAPISpec and QuotaSpec bindings.

  4. distribute the HTTPAPI and Quota spec to proxy as part of mixerclient filter configuration.

@istio-testing
Copy link
Copy Markdown
Collaborator

@ayj: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

Details

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

@istio-merge-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: rshriram

Assign the PR to them by writing /assign @rshriram in a comment when ready.

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

Details 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

@ayj
Copy link
Copy Markdown
Contributor Author

ayj commented Nov 23, 2017

That that this is a continuation of istio/old_pilot_repo#1644 which was rebased from istio/pilot onto istio/istio.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 23, 2017

Codecov Report

Merging #1845 into master will decrease coverage by 0.55%.
The diff coverage is 57.98%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1845      +/-   ##
==========================================
- Coverage   81.68%   81.12%   -0.56%     
==========================================
  Files         135      184      +49     
  Lines       11537    18650    +7113     
==========================================
+ Hits         9424    15130    +5706     
- Misses       1920     3101    +1181     
- Partials      193      419     +226
Flag Coverage Δ
#broker 44.44% <ø> (-1.07%) ⬇️
#mixer 82.46% <ø> (-0.03%) ⬇️
#pilot 80.27% <57.98%> (?)
#security 90.39% <ø> (ø) ⬆️
Impacted Files Coverage Δ
broker/pkg/model/config/mock_store.go 18.57% <ø> (ø) ⬆️
pilot/proxy/envoy/discovery.go 73.98% <0%> (ø)
pilot/proxy/envoy/ingress.go 75.55% <100%> (ø)
pilot/proxy/envoy/config.go 92.06% <100%> (ø)
pilot/model/conversion.go 85.36% <100%> (ø)
pilot/model/config.go 71.61% <11.11%> (ø)
pilot/adapter/config/crd/types.go 58.07% <52.83%> (ø)
pilot/model/validation.go 92.15% <70.39%> (ø)
pilot/proxy/envoy/mixer.go 87.2% <83.33%> (ø)
pilot/adapter/config/crd/conversion.go 96.11% <85.71%> (ø)
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1903f78...9373e33. Read the comment docs.

@ayj ayj requested a review from kyessenov November 27, 2017 17:45
// `istio.mixer.v1.config.client.IstioService` and
// `istio.proxy.v1.config.IstioService` are logically
// equivalent. Convert from mixer-to-proxy representation so we can
// use ResolveHostname below.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

note: IstioService is going away in pilot (see @louiscryan and @rshriram latest gateway proposal)

pbt := proto.MessageType(ps.MessageName)
if pbt == nil {
return nil, fmt.Errorf("unknown type %q", ps.MessageName)
pbt = gogoproto.MessageType(ps.MessageName)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clarify where gogoproto comes in from?

}

// ValidateHTTPAPISpecBinding checks that HTTPAPISpecBinding is well-formed.
func ValidateHTTPAPISpecBinding(msg proto.Message) error {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

note: we're entertaining the idea of using protoc-validate annotations from lyft to automate these functions. this is fine for now.

Copy link
Copy Markdown
Contributor

@kyessenov kyessenov left a comment

Choose a reason for hiding this comment

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

Code looks fine.

@ayj
Copy link
Copy Markdown
Contributor Author

ayj commented Nov 27, 2017

Agreed 100% on consistent codegen for our api-machinery. cc @gjwestlife

@ayj ayj merged commit 85983c2 into istio:master Nov 27, 2017
@ayj ayj deleted the distribute-mixerclient-spec-quota branch November 27, 2017 21:14
vadimeisenbergibm pushed a commit to vadimeisenbergibm/istio that referenced this pull request Nov 28, 2017
…io#1845)

1. create corresponding CRD for HTTPAPISpec, HTTPAPISpecBinding, QuotaSpec, and QuotaSpecBinding (see https://github.com/istio/api/blob/master/mixer/v1/config/client/api_spec.proto)

2. add corresponding validation code for new model types

3. update istioConfigStore to select HTTPAPISpec and QuotaSpec config associated with destination service instances using the corresponding HTTPAPISpec and QuotaSpec bindings.

4. distribute the HTTPAPI and Quota spec to proxy as part of mixerclient filter configura
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.

6 participants