Remote agent refactor cert provider new#810
Remote agent refactor cert provider new#810yanjiaxin534 wants to merge 54 commits intoeclipse-symphony:remote-agentfrom
Conversation
| sLog.InfofCtx(ctx, "V (Solution): handleWorkingCertManagement for remote target: %s, remove: %t", deployment.RemoteTargetName, remove) | ||
|
|
||
| if c.SolutionManager.GetCertProvider() == nil { | ||
| return fmt.Errorf("cert provider is not available") |
There was a problem hiding this comment.
we don't usually check manager's provider before we invoke the manager in vendor.
The check should be in the manager side.
There was a problem hiding this comment.
fix it now,thanks
| }) | ||
| } | ||
| // Check if targets manager is available for cert provider access | ||
| if c.TargetsManager == nil { |
There was a problem hiding this comment.
we don't usually check manager here. We should make sure in the init method.
There was a problem hiding this comment.
I fix it now, thanks
| "inCluster": true | ||
| } | ||
| }, | ||
| "working-cert": { |
There was a problem hiding this comment.
we can name it as cm-cert or k8s-cert
There was a problem hiding this comment.
I fix it now, thanks
| "type": "providers.secret.mock", | ||
| "config": {} | ||
| }, | ||
| "working-cert": { |
There was a problem hiding this comment.
fix it now, thanks
| SummaryManager | ||
| TargetProviders map[string]tgt.ITargetProvider | ||
| CertProvider certProvider.ICertProvider | ||
| certProviderConfig map[string]interface{} |
There was a problem hiding this comment.
why you need to keep certProviderConfig here?
There was a problem hiding this comment.
Thanks, I fix it now.
|
|
||
| // Initialize cert provider using unified approach | ||
| certProviderInstance, err := managers.GetCertProvider(config, providers) | ||
| if err == nil { |
There was a problem hiding this comment.
why we have special case for cert provider initialization?
There was a problem hiding this comment.
I don't need special case, fix it now.
| } | ||
|
|
||
| // GetCertProvider returns the cert provider instance for certificate management operations | ||
| func (s *SolutionManager) GetCertProvider() certProvider.ICertProvider { |
There was a problem hiding this comment.
where did you use this function?
|
|
||
| // SafeCreateWorkingCert creates a working certificate with validation checks | ||
| // It validates that the certificate doesn't exist before creation and verifies creation success after | ||
| func (s *SolutionManager) SafeCreateWorkingCert(ctx context.Context, certID string, request certProvider.CertRequest) error { |
There was a problem hiding this comment.
why we name it as SafeXXX?
|
|
||
| // SafeDeleteWorkingCert deletes a working certificate with validation checks | ||
| // It validates that the certificate exists before deletion and verifies deletion success after | ||
| func (s *SolutionManager) SafeDeleteWorkingCert(ctx context.Context, certID string, namespace string) error { |
There was a problem hiding this comment.
why we name it as SafeXXX?
| } | ||
|
|
||
| // Create the certificate | ||
| _, err := p.DynamicClient.Resource(certificateGVR).Namespace(req.Namespace).Create( |
There was a problem hiding this comment.
do we need to check the cert object ready status?
There was a problem hiding this comment.
Or I think the target create is not successfully.
| // Retry logic: retry up to 10 times, 2 seconds interval | ||
| var certificate *unstructured.Unstructured | ||
| var err error | ||
| for i := 0; i < 10; i++ { |
There was a problem hiding this comment.
can you define retryTimes:=10, i < (retryTimes-1) for time.sleep, using magic number is a bad coding style.
| time.Sleep(2 * time.Second) | ||
| } | ||
| } | ||
| if err != nil { |
There was a problem hiding this comment.
did you handle not found error? I saw you will use GetCert again for delete case.
There was a problem hiding this comment.
Yes， in delete case, when not find after delete, it means sucessfully delete the working cert.
| "k8s-cert": { | ||
| "type": "providers.cert.k8scert", | ||
| "config": { | ||
| "inCluster": true, |
There was a problem hiding this comment.
you don't need this option, did you have scenarios other than "inCluster"?
remote-agent/bootstrap/bootstrap.ps1
Outdated
| $WebRequestParams.GetEnumerator() | ForEach-Object { Write-Host (" {0}: {1}" -f $_.Key, $_.Value) } | ||
| $response = Invoke-WebRequest @WebRequestParams -Verbose | ||
| $jsonResponse = $response.Content | ConvertFrom-Json | ||
| if ($jsonResponse.public -and $jsonResponse.private -and $jsonResponse.public -ne "null" -and $jsonResponse.private -ne "null") { |
There was a problem hiding this comment.
what does "null" string compare mean? powershell have [string]::IsNullOrEmpty() utility.
There was a problem hiding this comment.
It is a bug, fix now, thanks
yanjiaxin534
force-pushed
the
remote-agent-refactor-cert-provider-new
branch
2 times, most recently
from
f25212a to
fa66dd4
Compare
yanjiaxin534
force-pushed
the
remote-agent-refactor-cert-provider-new
branch
from
f8f6fcb to
3642797
Compare
| func (s *SolutionManager) createCertRequest(targetName string, namespace string) certProvider.CertRequest { | ||
| // Create request with required fields - provider will use its configured defaults for Duration and RenewBefore only | ||
| return certProvider.CertRequest{ | ||
| TargetName: targetName, |
There was a problem hiding this comment.
It should be cert name rather than targetname.
The cert request does not only work for remote agent cert scenarios
| PublicKey string `json:"publicKey"` | ||
| PrivateKey string `json:"privateKey"` | ||
| ExpiresAt time.Time `json:"expiresAt"` | ||
| SerialNumber string `json:"serialNumber"` |
There was a problem hiding this comment.
It's called thumbprint in cert context
| } | ||
|
|
||
| func (p *K8SCertProvider) ID() string { | ||
| return "k8s-cert" |
| return cert.SerialNumber.String(), cert.NotAfter, nil | ||
| } | ||
|
|
||
| func (p *K8SCertProvider) CreateCert(ctx context.Context, req cert.CertRequest) error { |
There was a problem hiding this comment.
I think CreateCert interface should return the cert
3 participants
No description provided.