-
Notifications
You must be signed in to change notification settings - Fork 41
Remote agent refactor cert provider new #810
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
base: remote-agent
Are you sure you want to change the base?
Changes from 15 commits
67e711d
ff5b3bf
f5d79a4
f463629
cc10d56
6cce19e
e4f778c
9c50fba
2390cd0
a350525
e44cdf3
ace1d5d
127b54e
dd5b7a1
f186d06
d7d0d92
bf49c08
5714575
8644d5f
29d8d73
3f92994
c0aaea1
1341c66
4bb39a5
2d85e4c
dd4abb7
27b7948
3b263a9
fa20723
935df65
c2e69ca
c2c987e
a6042ec
61c62bb
b5e91c0
2b95a6e
f7621db
86297ca
b4efd62
14ca85f
f97352c
71bbfc7
fc7e1e7
2895e9d
fa66dd4
0e6f580
3d0a757
f8fc0b6
06f1319
fd518b9
214f646
3642797
830ad3a
189109c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ import ( | |
| "github.com/eclipse-symphony/symphony/api/pkg/apis/v1alpha1/managers/solution/metrics" | ||
| "github.com/eclipse-symphony/symphony/api/pkg/apis/v1alpha1/model" | ||
| sp "github.com/eclipse-symphony/symphony/api/pkg/apis/v1alpha1/providers" | ||
| certProvider "github.com/eclipse-symphony/symphony/api/pkg/apis/v1alpha1/providers/cert" | ||
| tgt "github.com/eclipse-symphony/symphony/api/pkg/apis/v1alpha1/providers/target" | ||
| api_utils "github.com/eclipse-symphony/symphony/api/pkg/apis/v1alpha1/utils" | ||
| "github.com/eclipse-symphony/symphony/coa/pkg/apis/v1alpha2" | ||
|
|
@@ -68,6 +69,8 @@ const ( | |
| type SolutionManager struct { | ||
| SummaryManager | ||
| TargetProviders map[string]tgt.ITargetProvider | ||
| CertProvider certProvider.ICertProvider | ||
| certProviderConfig map[string]interface{} | ||
| ConfigProvider config.IExtConfigProvider | ||
| SecretProvider secret.ISecretProvider | ||
| KeyLockProvider keylock.IKeyLockProvider | ||
|
|
@@ -118,6 +121,21 @@ func (s *SolutionManager) Init(context *contexts.VendorContext, config managers. | |
| return err | ||
| } | ||
|
|
||
| // Initialize cert provider | ||
| if certProviderInstance, exists := providers["working-cert"]; exists { | ||
| if cp, ok := certProviderInstance.(certProvider.ICertProvider); ok { | ||
| s.CertProvider = cp | ||
| // Try to get config from provider instance if possible | ||
| if providerCfg, ok := certProviderInstance.(interface{ Config() map[string]interface{} }); ok { | ||
| s.certProviderConfig = providerCfg.Config() | ||
| } else if providerCfg, ok := certProviderInstance.(interface{ GetConfig() map[string]interface{} }); ok { | ||
| s.certProviderConfig = providerCfg.GetConfig() | ||
| } | ||
| } else { | ||
| return fmt.Errorf("working-cert provider does not implement ICertProvider interface") | ||
| } | ||
| } | ||
|
|
||
| if v, ok := config.Properties["isTarget"]; ok { | ||
| b, err := strconv.ParseBool(v) | ||
| if err == nil || b { | ||
|
|
@@ -159,6 +177,77 @@ func (s *SolutionManager) Init(context *contexts.VendorContext, config managers. | |
|
|
||
| return nil | ||
| } | ||
|
|
||
| // GetCertProvider returns the cert provider instance for certificate management operations | ||
| func (s *SolutionManager) GetCertProvider() certProvider.ICertProvider { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where did you use this function? |
||
| return s.CertProvider | ||
| } | ||
|
|
||
| // 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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why we name it as SafeXXX?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fix it now |
||
| if s.CertProvider == nil { | ||
| return fmt.Errorf("cert provider not initialized") | ||
| } | ||
|
|
||
| // Pre-creation validation: check if certificate already exists | ||
| log.InfofCtx(ctx, " M (Solution): validating certificate %s doesn't exist before creation", certID) | ||
| _, err := s.CertProvider.GetCert(ctx, certID, request.Namespace) | ||
| if err == nil { | ||
| log.InfofCtx(ctx, " M (Solution): certificate %s already exists, skipping creation", certID) | ||
| return nil | ||
| } | ||
|
|
||
| // Create the certificate | ||
| log.InfofCtx(ctx, " M (Solution): creating working certificate %s", certID) | ||
| err = s.CertProvider.CreateCert(ctx, request) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create certificate %s: %v", certID, err) | ||
| } | ||
|
|
||
| // Post-creation validation: verify certificate was created successfully | ||
| log.InfofCtx(ctx, " M (Solution): validating certificate %s was created successfully", certID) | ||
| _, err = s.CertProvider.GetCert(ctx, certID, request.Namespace) | ||
| if err != nil { | ||
| return fmt.Errorf("certificate %s creation validation failed, certificate not found after creation: %v", certID, err) | ||
| } | ||
|
|
||
| log.InfofCtx(ctx, " M (Solution): working certificate %s created and validated successfully", certID) | ||
| return nil | ||
| } | ||
|
|
||
| // 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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why we name it as SafeXXX?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fix it now |
||
| if s.CertProvider == nil { | ||
| return fmt.Errorf("cert provider not initialized") | ||
| } | ||
|
|
||
| // Pre-deletion validation: check if certificate exists | ||
| log.InfofCtx(ctx, " M (Solution): validating certificate %s exists before deletion", certID) | ||
| _, err := s.CertProvider.GetCert(ctx, certID, namespace) | ||
| if err != nil { | ||
| return fmt.Errorf("certificate %s not found, cannot delete: %v", certID, err) | ||
| } | ||
|
|
||
| // Delete the certificate | ||
| log.InfofCtx(ctx, " M (Solution): deleting working certificate %s", certID) | ||
| err = s.CertProvider.DeleteCert(ctx, certID, namespace) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to delete certificate %s: %v", certID, err) | ||
| } | ||
|
|
||
| // Post-deletion validation: verify certificate was deleted successfully | ||
| log.InfofCtx(ctx, " M (Solution): validating certificate %s was deleted successfully", certID) | ||
| _, err = s.CertProvider.GetCert(ctx, certID, namespace) | ||
| if err == nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check is strange. In this case, you should get the NotFound error and the first cert object should be nil.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this check is for delete failed |
||
| return fmt.Errorf("certificate %s deletion validation failed, certificate still exists after deletion", certID) | ||
| } | ||
|
|
||
| log.InfofCtx(ctx, " M (Solution): working certificate %s deleted and validated successfully", certID) | ||
| return nil | ||
| } | ||
|
|
||
| func (s *SolutionManager) AsyncReconcile(ctx context.Context, deployment model.DeploymentSpec, remove bool, namespace string, targetName string) (model.SummarySpec, error) { | ||
| lockName := api_utils.GenerateKeyLockName(namespace, deployment.Instance.ObjectMeta.Name) | ||
| s.KeyLockProvider.Lock(lockName) | ||
|
|
@@ -1895,3 +1984,16 @@ func (s *SolutionManager) getOperationState(ctx context.Context, operationId str | |
| } | ||
| return ret, err | ||
| } | ||
|
|
||
| // CreateCertRequest creates a certificate request with required fields, letting the cert provider use its configured defaults for Duration and RenewBefore | ||
| func (s *SolutionManager) CreateCertRequest(targetName string, namespace string) certProvider.CertRequest { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why you need to expose this function? can you just integrate with SafeCreateWorkingCert?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we don't need it, fix it now |
||
| // Create request with required fields - provider will use its configured defaults for Duration and RenewBefore only | ||
| return certProvider.CertRequest{ | ||
| TargetName: targetName, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be cert name rather than targetname. |
||
| Namespace: namespace, | ||
| CommonName: "symphony-service", // Required field | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Define the const value in the front of the file. |
||
| IssuerName: "symphony-ca-issuer", // Required field | ||
| DNSNames: []string{targetName, fmt.Sprintf("%s.%s", targetName, namespace)}, | ||
| // Duration and RenewBefore will use provider defaults | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ type DeploymentSpec struct { | |
| Hash string `json:"hash,omitempty"` | ||
| IsDryRun bool `json:"isDryRun,omitempty"` | ||
| IsInActive bool `json:"isInActive,omitempty"` | ||
| RemoteTargetName string `json:"remoteTargetName,omitempty"` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why we need additional field? Can you use targets field? |
||
| } | ||
|
|
||
| func (d DeploymentSpec) GetComponentSlice() []ComponentSpec { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| /* | ||
| * Copyright (c) Microsoft Corporation. | ||
| * Licensed under the MIT license. | ||
| * SPDX-License-Identifier: MIT | ||
| */ | ||
|
|
||
| package cert | ||
|
|
||
| import ( | ||
| "context" | ||
| "time" | ||
| ) | ||
|
|
||
| // ICertProvider defines the interface for certificate management | ||
| type ICertProvider interface { | ||
| // CreateCert creates a certificate for the specified target | ||
| CreateCert(ctx context.Context, req CertRequest) error | ||
|
|
||
| // DeleteCert deletes the certificate for the specified target | ||
| DeleteCert(ctx context.Context, targetName, namespace string) error | ||
|
|
||
| // GetCert retrieves the certificate for the specified target (read-only) | ||
| GetCert(ctx context.Context, targetName, namespace string) (*CertResponse, error) | ||
|
|
||
| // CheckCertStatus checks if the certificate is ready and valid | ||
| CheckCertStatus(ctx context.Context, targetName, namespace string) (*CertStatus, error) | ||
| } | ||
|
|
||
| // CertRequest represents a certificate creation request | ||
| type CertRequest struct { | ||
| TargetName string `json:"targetName"` | ||
| Namespace string `json:"namespace"` | ||
| Duration time.Duration `json:"duration"` | ||
| RenewBefore time.Duration `json:"renewBefore"` | ||
| CommonName string `json:"commonName"` | ||
| DNSNames []string `json:"dnsNames"` | ||
| IssuerName string `json:"issuerName"` | ||
| ServiceName string `json:"serviceName"` | ||
| } | ||
|
|
||
| // CertResponse represents the certificate data | ||
| type CertResponse struct { | ||
| PublicKey string `json:"publicKey"` | ||
| PrivateKey string `json:"privateKey"` | ||
| ExpiresAt time.Time `json:"expiresAt"` | ||
| SerialNumber string `json:"serialNumber"` | ||
| } | ||
|
|
||
| // CertStatus represents the certificate status | ||
| type CertStatus struct { | ||
| Ready bool `json:"ready"` | ||
| Reason string `json:"reason"` | ||
| Message string `json:"message"` | ||
| LastUpdate time.Time `json:"lastUpdate"` | ||
| NextRenewal time.Time `json:"nextRenewal"` | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why you need to keep certProviderConfig here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I fix it now.