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

Update nodeagent proto message for Google CA plugin #20560

Merged
merged 10 commits into from Jan 29, 2020

Conversation

@myidpt
Copy link
Contributor

myidpt commented Jan 27, 2020

[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[ ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ X ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

@myidpt myidpt requested review from JimmyCYJ and lei-tang Jan 27, 2020
@myidpt myidpt requested a review from istio/wg-security-maintainers as a code owner Jan 27, 2020
@googlebot googlebot added the cla: yes label Jan 27, 2020
@myidpt myidpt changed the title Modify proto message to call Google CA API Modify nodeagent proto message to call Google CA API Jan 27, 2020
@myidpt myidpt changed the title Modify nodeagent proto message to call Google CA API Modify nodeagent proto message for Google CA plugin Jan 27, 2020
@myidpt myidpt removed the request for review from JimmyCYJ Jan 28, 2020
@myidpt myidpt changed the title Modify nodeagent proto message for Google CA plugin Update nodeagent proto message for Google CA plugin Jan 28, 2020
@myidpt myidpt requested a review from JimmyCYJ Jan 28, 2020
Copy link
Member

JimmyCYJ left a comment

LGTM

// Keep trying until no error or timeout.
for {
var httpRespCode int
if isCSR {
requestErrorString = fmt.Sprintf("%s CSR", conIDresourceNamePrefix)
certChainPEM, err = sc.fetcher.CaClient.CSRSign(
ctx, csrPEM, exchangedToken, int64(sc.configOptions.SecretTTL.Seconds()))
ctx, reqID, csrPEM, exchangedToken, int64(sc.configOptions.SecretTTL.Seconds()))

This comment has been minimized.

Copy link
@JimmyCYJ

JimmyCYJ Jan 28, 2020

Member

Do we need to print this reqID in existing logs so that we can trace a specific CSR request?

This comment has been minimized.

Copy link
@myidpt

myidpt Jan 28, 2020

Author Contributor

Good point. I updated the log.

@@ -49,7 +49,7 @@ func NewMockCAClient(mockCertChain1st, mockCertChainRemain []string, failureRate
return &cl
}

func (c *CAClient) CSRSign(ctx context.Context, csrPEM []byte, exchangedToken string,
func (c *CAClient) CSRSign(ctx context.Context, reqID string, csrPEM []byte, exchangedToken string,
certValidTTLInSec int64) ([]string /*PEM-encoded certificate chain*/, error) {

This comment has been minimized.

Copy link
@JimmyCYJ

JimmyCYJ Jan 28, 2020

Member

We could use a struct to wrap some parameters, but I'm okay with current function parameter list.

This comment has been minimized.

Copy link
@myidpt

myidpt Jan 28, 2020

Author Contributor

Sure. I think it's OK for now.

@@ -75,6 +75,12 @@ func isRetryableErr(c codes.Code, httpRespCode int, isGrpc bool) bool {

// cacheLogPrefix returns a unified log prefix.
func cacheLogPrefix(conID, resourceName string) string {
lPrefix := fmt.Sprintf("node:%s resource:%s", conID, resourceName)
lPrefix := fmt.Sprintf("[Node:%s Res:%s]", conID, resourceName)

This comment has been minimized.

Copy link
@howardjohn

howardjohn Jan 28, 2020

Member

why are we inventing new logging formats?

This comment has been minimized.

Copy link
@myidpt

myidpt Jan 28, 2020

Author Contributor

Good question. I think this makes the logging line shorter (more concise). But if this pattern aligns with other components, I can change it back.

p := sc.configOptions.Plugins[0]
exchangedToken, _, httpRespCode, err = p.ExchangeToken(ctx, sc.configOptions.TrustDomain, exchangedToken)
}
cacheLog.Debugf("%s", requestErrorString)

This comment has been minimized.

Copy link
@howardjohn

howardjohn Jan 28, 2020

Member

remove? we are already logging it

This comment has been minimized.

Copy link
@myidpt

myidpt Jan 28, 2020

Author Contributor

But for other parts, we are only logging it when an error happens. I think this is OK since it's Debugf :)


// Certificate request message.
message MeshCertificateRequest {
// The request ID must be a valid UUID with the exception that zero UUID is
// not supported (00000000-0000-0000-0000-000000000000).
string request_id = 1;

This comment has been minimized.

Copy link
@howardjohn

howardjohn Jan 28, 2020

Member

Seems a bit weird to change the proto #, isn't it supposed to be immutable

This comment has been minimized.

Copy link
@myidpt

myidpt Jan 28, 2020

Author Contributor

Yes, but since the package is changed from v1beta1 to v1, I think it's OK. We are changing the package during this transition.

@howardjohn

This comment has been minimized.

Copy link
Member

howardjohn commented Jan 28, 2020

/retest

@istio-testing istio-testing merged commit 8ecb735 into istio:master Jan 29, 2020
23 checks passed
23 checks passed
cla/google All necessary CLAs are signed
e2e-bookInfoTests-envoyv2-v1alpha3_istio Job succeeded.
Details
e2e-dashboard_istio Job succeeded.
Details
e2e-mixer-no_auth_istio Job succeeded.
Details
gencheck_istio Job succeeded.
Details
integ-conformance-k8s-tests_istio Job succeeded.
Details
integ-conformance-local-tests_istio Job succeeded.
Details
integ-distroless-k8s-tests_istio Job succeeded.
Details
integ-galley-k8s-tests_istio Job succeeded.
Details
integ-galley-local-tests_istio Job succeeded.
Details
integ-istioio-k8s-tests_istio Job succeeded.
Details
integ-mixer-k8s-tests_istio Job succeeded.
Details
integ-pilot-k8s-tests_istio Job succeeded.
Details
integ-pilot-local-tests_istio Job succeeded.
Details
integ-security-k8s-tests_istio Job succeeded.
Details
integ-security-local-tests_istio Job succeeded.
Details
integ-telemetry-k8s-tests_istio Job succeeded.
Details
lint_istio Job succeeded.
Details
pilot-e2e-envoyv2-v1alpha3_istio Job succeeded.
Details
pilot-multicluster-e2e_istio Job succeeded.
Details
release-test_istio Job succeeded.
Details
tide In merge pool.
Details
unit-tests_istio Job succeeded.
Details
@istio-testing

This comment has been minimized.

Copy link
Collaborator

istio-testing commented Jan 29, 2020

In response to a cherrypick label: #20560 failed to apply on top of branch "release-1.4":

Applying: Small fix.
Applying: Comment out the last field in the proto message.
Applying: Small fix.
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	security/pkg/nodeagent/cache/helper.go
M	security/pkg/nodeagent/cache/secretcache.go
Falling back to patching base and 3-way merge...
Auto-merging security/pkg/nodeagent/cache/secretcache.go
Auto-merging security/pkg/nodeagent/cache/helper.go
CONFLICT (content): Merge conflict in security/pkg/nodeagent/cache/helper.go
Patch failed at 0004 Small fix.

@istio-testing

This comment has been minimized.

Copy link
Collaborator

istio-testing commented Jan 29, 2020

In response to a cherrypick label: new pull request created: #20628

@myidpt myidpt deleted the myidpt:googlecafix branch Jan 29, 2020
selmanj added a commit to selmanj/istio that referenced this pull request Jan 29, 2020
* Modify the proto message to work with Google CA.

* Small fix.

* Comment out the last field in the proto message.

* Small fix.

* Small fix.

* Small fix on logging.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.