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

feat(kuma-cp) server for managing provided ca #473

Merged
merged 10 commits into from
Dec 11, 2019

Conversation

jakubdyszkiewicz
Copy link
Contributor

Summary

Server for managing Provided CA.

Open questions

  1. Name
    How do we name common server for Dataplane Token Server and CA Manager Server? For now I introduced name "Admin Server".
    Because of the lack of the name I did not introduce new configs for now and just hardcoded the port.
    Maybe after all it's easier to have a separate server?

  2. Error Handling
    For now I just used functions from api-server package, but it's a bit weird to have dependency inside ca/provided/rest on that. Should we extract error handler and error types to something like core/rest/errors?

@jakubdyszkiewicz jakubdyszkiewicz requested a review from a team December 6, 2019 11:28
Copy link
Contributor

@yskopets yskopets left a comment

Choose a reason for hiding this comment

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

Maybe after all it's easier to have a separate server?

If I understand correctly, the requirement is to have a single server.

Should we extract error handler and error types to something like core/rest/errors?

Makes sense 👍

@jakubdyszkiewicz jakubdyszkiewicz force-pushed the feature/provided-ca-server branch from 4115adc to b93b9d0 Compare December 11, 2019 11:48
@jakubdyszkiewicz
Copy link
Contributor Author

Merged two servers together and provided autoconfig with migration form old one.

case config_core.UniversalEnvironment:
generator, err := builtin.NewDataplaneTokenIssuer(rt)
if err != nil {
return nil, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a swallowed error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

// Admin Server configuration
type AdminServerConfig struct {
// Configuration for Dataplane Token Webservice
DataplaneTokenWs *DataplaneTokenWsConfig `yaml:"dataplaneTokenWs"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Name dataplaneTokenWs that will appear in YAML and env variables is too implementation-specific.

Let's use the same approach as in Catalog.

E.g.,

type AdminServerConfig struct {
   Apis *ApisConfg
}

type ApisConfig struct {
  DataplaneToken *DataplaneTokenApiConfig
}

type DataplaneTokenApiConfig struct {}

@jakubdyszkiewicz jakubdyszkiewicz merged commit ae3d687 into master Dec 11, 2019
@jakubdyszkiewicz jakubdyszkiewicz deleted the feature/provided-ca-server branch December 23, 2019 11:52
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.

2 participants