[Refactor] DCR Monitor Refactoring (2/N) #19
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
privacywill
reviewed
Nov 5, 2024
| hlog.Infof("[KanikoJobMonitor]job name: %v, job status: %v", k8sJob.Name, k8sJob.Status.Conditions[0].Type) | ||
|
|
||
| if k8sJob.Status.Conditions[0].Type == batchv1.JobComplete { | ||
| image, digest, err := b.getImageDigestAndDeleteJob(k8sJob.Name) |
There was a problem hiding this comment.
How about devide the getImageDigestAndDeleteJob function into two functions. Something like getImageDigest and deleteJob
privacywill
reviewed
Nov 5, 2024
| OIDC OIDC `json:"oidc"` | ||
| } | ||
|
|
||
| // FIXME: duplicated from pkg/cloud |
There was a problem hiding this comment.
Does it mean we should remove the duplicated struct in the pkg/cloud
There was a problem hiding this comment.
I have completely deduplicated it by removing PrepareResourcesForUser.
privacywill
reviewed
Nov 6, 2024
| return req | ||
| } | ||
|
|
||
| func (c *TEEProviderGCPConfidentialSpace) createWorkloadIdentityPoolProvider(name string, digest string) error { |
There was a problem hiding this comment.
workload identity pool is used for allowing the confidential space instance to use the kms key. Since the kms key has been totally removed, we should think whether the workload identity is required any more.
privacywill
approved these changes
Nov 6, 2024
dayeol
added a commit
that referenced
this pull request
Nov 13, 2024
2/3 PR for a major refactoring of DCR Monitor. See #15. 1/3: #16 2/3: #19 This pretty much concludes the major refactoring, resolving #15 # Changes * Added `Created` job status, because the reconciler needs to handle newly created jobs. * Move KanikoService functionality into the monitor (=reconciler) * Remove unused config `Cluster.PodServiceAccount` * Clean up ARG/ENV of TEE dockerfile * In-memory Dockerfile injection in API. See `addDockerfileToTarGz`. This resolves #3. Also Kaniko can just refer to the remote object path. * Removed most of shallow functions in `Config` * Completely removed `CloudProvider` * Removed shallow / no-longer used modules in `pkg`: `pkg/utils/constant.go`, `pkg/utils/file.go`, and `pkg/utils/k8s.go`. # Testing Manual end-to-end testing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
2/N PR for a major refactoring of DCR Monitor. See #15.
1/N: #16
Changes
imagebuilderinterface, which currently has one image builder implementation which is KanikoRunJoblogic into the reconciler. Now TEE is launched by the reconciler viaLaunchInstancemethod inTEEProvider.Config. Removed bunch of unnecessary global configs, and made them local.PrepareResourcesForUser. Any logic needed for TEE should go toTEEProvider.project_numberfrom config, because it is not needed anymore.Caveat
Testing
Added unit tests, and also did manual end-to-end testing.