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

Refactor Flux 2.3 update #172

Merged
merged 1 commit into from
Jun 24, 2024
Merged

Refactor Flux 2.3 update #172

merged 1 commit into from
Jun 24, 2024

Conversation

kvaps
Copy link
Member

@kvaps kvaps commented Jun 17, 2024

This PR:

@kvaps kvaps changed the base branch from main to upd-flux June 17, 2024 14:12
@kvaps kvaps changed the title flux 2.3 refactor refactor Flux 2.3 update Jun 17, 2024
@kvaps kvaps changed the title refactor Flux 2.3 update Refactor Flux 2.3 update Jun 17, 2024
@kvaps kvaps mentioned this pull request Jun 17, 2024
5 tasks
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
@kingdonb
Copy link
Contributor

I'm checking out this PR now. I see that flux-operator 0.6 is out, a few hours ago. I'll run make update and see what divergences exist between the upstream chart and the local clone. Ideally the changes having been merged upstream will allow us to remove any patches we've added out-of-band to the flux-operator chart.

I saw you merged the values files together, I don't know how to support this idea, but I was hoping we could allow users to select their own values somehow, maybe through a toggle. The idea being that the more fields we set in our FluxInstance the more difficult it will be for users to customize it.

In the earlier PR of the two that got merged, #166 I think I kept it minimal, and then added more detailed customization in #167 - in my initial idea, ideally the whole spec.kustomize block would be left unset, so users can 3-way merge in their own configuration there.

Then on second thought I guess it makes more sense if cozystack is creating the FluxInstance to expose a configuration opportunity for the users more directly, they shouldn't have to know about 3-way merge behavior just to configure Flux!

@kingdonb kingdonb mentioned this pull request Jun 22, 2024
@kingdonb
Copy link
Contributor

kingdonb commented Jun 22, 2024

The network policy appears a bit messed up.

allow-from-dashboard is present in cozy-fluxcd ns, but the usual flux networkpolicy resources aren't present (so the default deny rule that Flux installs is not the issue)

The network patch (alternative to setting cluster.domain) didn't work for me. It correctly identified the service to poll (source-controller) and it looks like coreDNS is directing us to the right address, but it's getting I/O timeout. The error:

cozy-cilium                      cilium                      6d    False   Could not load chart: GET http://source-controller.cozy-fluxcd.svc/helmchart/cozy-system/cozy-cilium-cilium/cozy-cilium-0.1.0.tgz giving up after 10 attempt(s): Get "http://source-controller.cozy-fluxcd.svc/helmchart/cozy-system/cozy-cilium-cilium/cozy-cilium-0.1.0.tgz": dial tcp 10.96.254.142:80: i/o timeout

This may have been the cilium issue you were talking about before?

@kingdonb
Copy link
Contributor

Removing the network policy that cozystack imposes (I can't find where, I did spot it earlier) allows flux pods to talk again:

cozy-fluxcd      allow-from-dashboard              <none>                                                                                                          40m

I suspect that flux would be talking fine if the default network policies weren't skipped. I'm not sure why they don't apply. I'll keep poking around, I have not yet mastered the build process for cozystack + all its parts

@kingdonb
Copy link
Contributor

I see that allow-from-dashboard policy has actually been removed in d44681b

The flux network policies weren't created by default because:

looks like a bug in the operator, it can be easily worked around by setting any of the values under cluster. so that networkPolicy: true default is set. (I hadn't seen it before because I've been setting cluster.domain)

What was the purpose of removing allow-from-dashboard ? (it may need to be re-added if this bug is fixed upstream.)

@kingdonb
Copy link
Contributor

remove policy to allow access fluxcd from dashboard because now fluxcd shipped without policies

I'm sure this is a bug, networkPolicy: true is documented as a default, but we're not getting that value because we haven't set any values under cluster. Cozystack isn't setting up any secrets in flux-system to protect for now, so maybe we can keep this off, but I don't think it's intentional that these network policies are not shipped by default anymore.

I opened:

| commonAnnotations | object | `{}` | Common annotations to add to all deployed objects including pods. |
| commonLabels | object | `{}` | Common labels to add to all deployed objects including pods. |
| fullnameOverride | string | `""` | |
+| hostNetwork | bool | `false` | If `true`, start flux-operator in hostNetwork mode. |

Choose a reason for hiding this comment

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

This is not in sync with the upstream chart, please copy https://github.com/controlplaneio-fluxcd/charts/tree/main/charts/flux-operator

Copy link
Contributor

@kingdonb kingdonb Jun 22, 2024

Choose a reason for hiding this comment

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

We have make update which pulls from OCI chart upstream, and for flux-operator 0.6 - #178 will bring it back in sync, before these changes are released. I already updated in that PR to fix fuzz in one of the patches here, and deleted the other one for hostNetwork that is no longer needed because it got merged upstream in controlplaneio-fluxcd/charts#10.

@kvaps kvaps merged commit b8e5309 into upd-flux Jun 24, 2024
@kvaps kvaps deleted the flux-2.3-refactor branch June 24, 2024 11:14
kvaps added a commit that referenced this pull request Jun 24, 2024
This PR upgrades to Flux-Operator 0.6 released this morning, also includes:

* #170
which is an aggregate PR, so #171 #172 etc. I think this PR now basically subsumes #170 and can replace it.

I have at least 80% confidence there are no errors in this PR. It also restores the networkPolicy default and the deleted cozy-dashboard network policy, which we will see fixed (restored to install NetworkPolicy resources by default) in the next `flux-operator` release.

Ref: controlplaneio-fluxcd/flux-operator#52
kvaps pushed a commit that referenced this pull request Jun 24, 2024
This PR upgrades to Flux-Operator 0.6 released this morning, also includes:

* #170
which is an aggregate PR, so #171 #172 etc. I think this PR now basically subsumes #170 and can replace it.

I have at least 80% confidence there are no errors in this PR. It also restores the networkPolicy default and the deleted cozy-dashboard network policy, which we will see fixed (restored to install NetworkPolicy resources by default) in the next `flux-operator` release.

Ref: controlplaneio-fluxcd/flux-operator#52
kvaps pushed a commit that referenced this pull request Jun 24, 2024
This PR upgrades to Flux-Operator 0.6 released this morning, also includes:

* #170
which is an aggregate PR, so #171 #172 etc. I think this PR now basically subsumes #170 and can replace it.

I have at least 80% confidence there are no errors in this PR. It also restores the networkPolicy default and the deleted cozy-dashboard network policy, which we will see fixed (restored to install NetworkPolicy resources by default) in the next `flux-operator` release.

Ref: controlplaneio-fluxcd/flux-operator#52
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
kvaps pushed a commit that referenced this pull request Jun 24, 2024
This PR upgrades to Flux-Operator 0.6 released this morning, also includes:

* #170
which is an aggregate PR, so #171 #172 etc. I think this PR now basically subsumes #170 and can replace it.

I have at least 80% confidence there are no errors in this PR. It also restores the networkPolicy default and the deleted cozy-dashboard network policy, which we will see fixed (restored to install NetworkPolicy resources by default) in the next `flux-operator` release.

Ref: controlplaneio-fluxcd/flux-operator#52
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
kvaps pushed a commit that referenced this pull request Jun 24, 2024
This PR upgrades to Flux-Operator 0.6 released this morning, also includes:

* #170
which is an aggregate PR, so #171 #172 etc. I think this PR now basically subsumes #170 and can replace it.

I have at least 80% confidence there are no errors in this PR. It also restores the networkPolicy default and the deleted cozy-dashboard network policy, which we will see fixed (restored to install NetworkPolicy resources by default) in the next `flux-operator` release.

Ref: controlplaneio-fluxcd/flux-operator#52
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
kvaps added a commit that referenced this pull request Jun 24, 2024
This cumulative PR includes the following changes:

- Migrate from fluxcd-community charts to Flux-Operator #166
- Upgrade to Flux 2.3.x #167
- Refactor Flux 2.3 update #172
- Update flux plugin for dashboard #171
- Flux Operator 0.6 #178
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants