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.

Sign up for GitHub

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

Jump to bottom

Add native Azure Blob support to BucketController #513

Closed
wants to merge 56 commits into from
Closed

Add native Azure Blob support to BucketController #513

wants to merge 56 commits into from

Conversation

laozc
Copy link
Contributor

@laozc laozc commented Dec 4, 2021 •
edited
Loading

This PR adds native Azure Blob support to the BucketController
which enables the integration in Azure environment.

---
apiVersion: source.toolkit.fluxcd.io/v1beta1
kind: Bucket
metadata:
  name: azure-blob
 namespace: flux-system
spec:
  interval: 1m
  provider: azure
  bucketName: flux
  endpoint: https://STORAGE_ACCOUNT_NAME.blob.core.windows.net/
  secretRef:
    name: blob-credentials

To provide the key the user may choose to use account key or resourceId.

kubectl create secret generic blob-credentials --namespace flux-system --from-literal secretkey=[STORAGE_ACCOUNT_KEY]
or
kubectl create secret generic blob-credentials --namespace flux-system --from-literal accesskey=[STORAGE_ACCOUNT_RESOURCE_ID]

When using resourceId as accesskey, the source-controller deployment is required to have Azure cloud-config (/etc/kubernetes/azure.json) mounted.

@laozc laozc force-pushed the azblob branch 4 times, most recently from 658faf0 to 77bf538 Compare December 4, 2021 04:18
laozc added 3 commits December 4, 2021 12:25
@laozc
Add native support to Azure Blob
04219b8 
Signed-off-by: Zhongcheng Lao <[email protected]>
@laozc
Remove calling deprecated MSI function
774cf9a 
Signed-off-by: Zhongcheng Lao <[email protected]>
@laozc
Update CRD
e89ef08 
Signed-off-by: Zhongcheng Lao <[email protected]>
hiddeco and others added 7 commits December 6, 2021 13:49
@hiddeco @darkowlzz
Introduce more explicit Condition types
8e86b57 
This commit introduces new Condition types to the v1beta1 API,
facilitating easier observation of (potentially) problematic state for
end-users.

- `ArtifactUnavailableCondition`: indicates there is no artifact
  available for the resource. This Condition should be set by the
  reconciler as soon as it observes the absence of an artifact for a
  source.
- `CheckoutFailedCondition`: indicates a transient or persistent
  checkout failure. This Condition should be set by the reconciler as
  soon as it observes a Git checkout failure, including any
  prerequisites like the unavailability of the referenced Secret used
  for authentication. It should be deleted as soon as a successful
  checkout has been observed again.
- `SourceVerifiedCondition`: indicates the integrity of the source has
  been verified. The Condition should be set to True or False by the
  reconciler based on the result of the integrity check.
  If there is no verification mode and/or secret configured, the
  Condition should be removed.
- `IncludeUnavailableCondition`: indicates one of the referenced
  includes is not available. This Condition should for example be set
  by the reconciler when the include does not exist, or does not have
  an artifact. If the includes become available, it should be deleted.
- `ArtifactOutdatedCondition`: indicates the current artifact of the
  source is outdated. This Condition should for example be set by the
  reconciler when it notices there is a newer revision for an artifact,
  or the previously included artifacts differ from the current available
  ones. The Condition should be removed after writing a new artifact
  to the storage.

Signed-off-by: Hidde Beydals <[email protected]>
@hiddeco @darkowlzz
Implement new runtime interfaces, prepare testenv
c50a170 
This commit ensures all API objects implement the interfaces used by
the runtime package to work with conditions, etc., and prepares the
test suite to work with the `pkg/runtime/testenv` wrapper.

Changes are made in a backwards compatible way (that being: the
existing code can still be build and works as expected), but without
proper dependency boundaries. The result of this is that the API
package temporary depends on the runtime package, which is resolved
when all reconcilers have been refactored and the API package does
no longer contain condition modifying functions.

Signed-off-by: Hidde Beydals <[email protected]>
@hiddeco @darkowlzz
Introduce artifactSet to replace hasArtifactUpdated
65dd61e 
NOTE: Remove `hasArtifactUpdated` in the future once it's no longer
used.

Signed-off-by: Hidde Beydals <[email protected]>
@darkowlzz @hiddeco
source: Add GetRequeueAfter
c278501 
The problem with `GetInterval()` was that the returned type was of
`metav1.Duration`, while almost anywhere it was used, a type of
`time.Duration` was requested. The result of this was that we had to
call `GetInterval().Duration` all the time, which would become a bit
cumbersome after awhile.

To prevent this, we introduce a new `GetRequeueAfter() time.Duration`
method, which both results the right type, and bears a name that is
easier to remember where the value is used most; while setting the
`Result.RequeueAfter` during reconcile operations.

The introduction of this method deprecates `GetInterval()`, which
should be removed in a future MINOR release.

Signed-off-by: Sunny <[email protected]>

Co-authored-by: Hidde Beydals <[email protected]>
@darkowlzz
Use new events and metrics helpers in main.go
2d88469 
Signed-off-by: Sunny <[email protected]>
@darkowlzz @hiddeco
Move Artifact conditions to conditions
e7f0406 
Also, introduce FetchFailedCondition for generic fetch failures.

Signed-off-by: Sunny <[email protected]>

Co-authored-by: Hidde Beydals <[email protected]>
@darkowlzz @hiddeco
Add gomega matcher for artifact
a722ec7 
Signed-off-by: Sunny <[email protected]>

Co-authored-by: Hidde Beydals <[email protected]>
@stefanprodan
Copy link
Member

When using resourceId as accesskey, the source-controller deployment is required to have Azure cloud-config (/etc/kubernetes/azure.json) mounted.

Please remove this option, mounting files inside a Flux controller breaks multi-tenancy.

phillebaba
phillebaba previously approved these changes Dec 6, 2021
View reviewed changes
controllers/bucket_controller.go Outdated Show resolved Hide resolved
controllers/bucket_controller.go Outdated Show resolved Hide resolved
controllers/bucket_controller.go Outdated Show resolved Hide resolved
controllers/bucket_controller.go Outdated Show resolved Hide resolved
@phillebaba
Copy link
Member

@laozc looks good, left a few smaller comments 🌹

hiddeco
hiddeco requested changes Dec 6, 2021
View reviewed changes
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

Generally supportive of adding support for Azure Blob, but I think the changes need to be carefully aligned with the work in #455 and additional pending work in https://github.com/fluxcd/source-controller/tree/reconcilers-dev-bucket.

Which means it may take some time before the prerequisites for this PR are in place, and I can provide a precise insight into what needs further addressing.

@phillebaba phillebaba self-requested a review December 6, 2021 14:36
@phillebaba phillebaba dismissed their stale review December 6, 2021 14:37

Remove Approval

@laozc
Fixes PR comments
0ae7758 
Signed-off-by: Zhongcheng Lao <[email protected]>
@laozc
Copy link
Contributor Author

laozc commented Dec 7, 2021

I'll hold on this PR for a bit until the #455 is merged.

@laozc
Copy link
Contributor Author

laozc commented Dec 7, 2021

When using resourceId as accesskey, the source-controller deployment is required to have Azure cloud-config (/etc/kubernetes/azure.json) mounted.

Please remove this option, mounting files inside a Flux controller breaks multi-tenancy.

Could you elaborate more details on your concern?
This Azure cloud config file is a cluster-wide configuration which contains the Service Principle or Managed Identity used by the cluster.
Services in the cluster may use the identity to communicate with Azure Management APIs.
It does not contain specific information to any resource linked to a Bucket object.
When mouting the cloud config in Flux controller, the controller gains the same permission as the cluster on operating Azure resources.
The user may provide only Azure Resource ID without touching the actual storage access key, which enhances the security in Azure environment.

darkowlzz and others added 6 commits December 7, 2021 21:51
@darkowlzz
bucket: Replace GetInterval() with GetRequeueAfter()
f1302d8 
Signed-off-by: Sunny <[email protected]>
@hiddeco @darkowlzz
Rewrite BucketReconciler to new standards
795b83d 
This commit rewrites the `BucketReconciler` to new standards, while
implementing the newly introduced Condition types, and trying to
adhere better to Kubernetes API conventions.

More specifically it introduces:

- Implementation of more explicit Condition types to highlight
  abnormalities.
- Extensive usage of the `conditions` subpackage from `runtime`.
- Better and more conflict-resilient (status)patching of reconciled
  objects using the `patch` subpackage from runtime.
- Proper implementation of kstatus' `Reconciling` and `Stalled`
  conditions.
- Refactor of reconciler logic, including more efficient detection of
  changes to bucket objects by making use of the etag data available,
  and downloading of object files in parallel with a limited number of
  workers (4).
- Integration tests that solely rely on `testenv` and do not
  use Ginkgo.

There are a couple of TODOs marked in-code, these are suggestions for
the future and should be non-blocking.
In addition to the TODOs, more complex and/or edge-case test scenarios
may be added as well.

Signed-off-by: Hidde Beydals <[email protected]>
@hiddeco @darkowlzz
Consolidate condition types into FetchFailed
f82a906 
This commit consolidates the `DownloadFailed` and `CheckoutFailed`
Condition types into a new more generic `FetchFailed` type to simplify
the API and observations by consumers.

Signed-off-by: Hidde Beydals <[email protected]>
@darkowlzz
BucketReconciler: Add reconcileArtifact tests
6f34ea8 
Add `BucketReconciler.reconcileArtifact` tests based on
`GitRepositoryReconciler.reconcileArtifact` test cases.

Signed-off-by: Sunny <[email protected]>
@darkowlzz
Add more reconcileMinioSource test cases
317cd44 
Signed-off-by: Sunny <[email protected]>
@darkowlzz
Add bucket controller tests for reconcileGCPSource
4814abc 
- Introduce mock GCP Server to test the gcp bucket client against mocked
  gcp server results.
- Add tests for reconcileGCPSource().
- Patch GCPClient.BucketExists() to return no error when the bucket
  doesn't exists. This keeps the GCP client compatible with the minio
  client.

Signed-off-by: Sunny <[email protected]>
hiddeco and others added 15 commits January 20, 2022 20:53
@hiddeco @darkowlzz
Introduce artifactSet to replace hasArtifactUpdated
7d752b7 
NOTE: Remove `hasArtifactUpdated` in the future once it's no longer
used.

Signed-off-by: Hidde Beydals <[email protected]>
@darkowlzz @hiddeco
source: Add GetRequeueAfter
d9ad873 
The problem with `GetInterval()` was that the returned type was of
`metav1.Duration`, while almost anywhere it was used, a type of
`time.Duration` was requested. The result of this was that we had to
call `GetInterval().Duration` all the time, which would become a bit
cumbersome after awhile.

To prevent this, we introduce a new `GetRequeueAfter() time.Duration`
method, which both results the right type, and bears a name that is
easier to remember where the value is used most; while setting the
`Result.RequeueAfter` during reconcile operations.

The introduction of this method deprecates `GetInterval()`, which
should be removed in a future MINOR release.

Signed-off-by: Sunny <[email protected]>

Co-authored-by: Hidde Beydals <[email protected]>
@darkowlzz
Use new events and metrics helpers in main.go
1c25c8c 
Signed-off-by: Sunny <[email protected]>
@darkowlzz @hiddeco
Move Artifact conditions to conditions
f927c76 
Also, introduce FetchFailedCondition for generic fetch failures.

Signed-off-by: Sunny <[email protected]>

Co-authored-by: Hidde Beydals <[email protected]>
@darkowlzz @hiddeco
Add gomega matcher for artifact
34b5dcf 
Signed-off-by: Sunny <[email protected]>

Co-authored-by: Hidde Beydals <[email protected]>
@darkowlzz
api: Embed runtime.Object in Source interface
45484ef 
Embedding runtime.Object in Source interface makes the Source type more
useful to interact with k8s API machinery.

Signed-off-by: Sunny <[email protected]>
@darkowlzz
Add internal packages error and reconcile
c3e2087 
- internal/error - Contains internal error type used across the
  source-controller reconcilers.
- internal/reconcile - Contains helper abstractions for the
  controller-runtime reconcile Result type and functions to
  interact with the abstractions.

Signed-off-by: Sunny <[email protected]>
@darkowlzz
Use runtime-events-fixes branch temporarily
8908d80 
Signed-off-by: Sunny <[email protected]>
@darkowlzz
bucket: Replace GetInterval() with GetRequeueAfter()
991692f 
Signed-off-by: Sunny <[email protected]>
@hiddeco @darkowlzz
Rewrite BucketReconciler to new standards
34b27a0 
This commit rewrites the `BucketReconciler` to new standards, while
implementing the newly introduced Condition types, and trying to
adhere better to Kubernetes API conventions.

More specifically it introduces:

- Implementation of more explicit Condition types to highlight
  abnormalities.
- Extensive usage of the `conditions` subpackage from `runtime`.
- Better and more conflict-resilient (status)patching of reconciled
  objects using the `patch` subpackage from runtime.
- Proper implementation of kstatus' `Reconciling` and `Stalled`
  conditions.
- Refactor of reconciler logic, including more efficient detection of
  changes to bucket objects by making use of the etag data available,
  and downloading of object files in parallel with a limited number of
  workers (4).
- Integration tests that solely rely on `testenv` and do not
  use Ginkgo.

There are a couple of TODOs marked in-code, these are suggestions for
the future and should be non-blocking.
In addition to the TODOs, more complex and/or edge-case test scenarios
may be added as well.

Signed-off-by: Hidde Beydals <[email protected]>
@hiddeco @darkowlzz
Consolidate condition types into FetchFailed
3105ba9 
This commit consolidates the `DownloadFailed` and `CheckoutFailed`
Condition types into a new more generic `FetchFailed` type to simplify
the API and observations by consumers.

Signed-off-by: Hidde Beydals <[email protected]>
@darkowlzz
BucketReconciler: Add reconcileArtifact tests
cf7dca0 
Add `BucketReconciler.reconcileArtifact` tests based on
`GitRepositoryReconciler.reconcileArtifact` test cases.

Signed-off-by: Sunny <[email protected]>
@darkowlzz
Add more reconcileMinioSource test cases
af946e7 
Signed-off-by: Sunny <[email protected]>
@darkowlzz
Add bucket controller tests for reconcileGCPSource
8dd51ab 
- Introduce mock GCP Server to test the gcp bucket client against mocked
  gcp server results.
- Add tests for reconcileGCPSource().
- Patch GCPClient.BucketExists() to return no error when the bucket
  doesn't exists. This keeps the GCP client compatible with the minio
  client.

Signed-off-by: Sunny <[email protected]>
@darkowlzz
bucket: Ignore patch error not found on delete
4a1818d 
Ignore "not found" error while patching when the delete timestamp is
set.

Signed-off-by: Sunny <[email protected]>
@darkowlzz darkowlzz force-pushed the reconcilers-dev-bucket branch from f4aded7 to ceec2b0 Compare January 20, 2022 22:05
@darkowlzz
bucket: Add more reconciler design improvements
a1e5067 
- Remove ArtifactUnavailable condition and use Reconciling condition to
  convey the same.
- Make Reconciling condition affect the ready condition.
- Introduce summarizeAndPatch() to calculate the final status conditions
  and patch them.
- Introduce reconcile() to iterate through the sub-reconcilers and
  execute them.

Signed-off-by: Sunny <[email protected]>
@darkowlzz darkowlzz force-pushed the reconcilers-dev-bucket branch from ceec2b0 to a1e5067 Compare January 20, 2022 23:46
@darkowlzz darkowlzz force-pushed the reconcilers-dev-bucket branch 4 times, most recently from 55311a5 to 4b939ee Compare January 27, 2022 16:26
@laozc
Copy link
Contributor Author

laozc commented Feb 19, 2022

Ping.
We have some customers waiting for the support to land in flux.
Could you advise how to proceed?

@hiddeco
Copy link
Member

hiddeco commented Feb 21, 2022

We are working on getting the work of #586 in main. Once that's done, I will revise this PR (and #455), resolve the conflicts, and ensure it gets included in the same release as the one that introduces the v1beta2 API.

Sorry about the wait, changes in that PR are so broad that fitting additional things into it is a bit like playing Tetris 😅

@hiddeco hiddeco mentioned this pull request Feb 21, 2022
Closed
5 tasks
@hiddeco hiddeco deleted the branch fluxcd:reconcilers-dev-bucket February 23, 2022 12:37
@hiddeco hiddeco closed this Feb 23, 2022
@hiddeco
Copy link
Member

hiddeco commented Feb 23, 2022

@laozc do not worry about the close. Feel either free to re-open a new one targeted against main, or wait for me to do it once I am done with #455.

@hiddeco hiddeco mentioned this pull request Mar 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hiddeco hiddeco hiddeco requested changes

@phillebaba phillebaba Awaiting requested review from phillebaba

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@laozc @stefanprodan @phillebaba @jonathan-innis @csand-msft @kingdonb @hiddeco @darkowlzz