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

FlowForge helm: 1. Editors: service account. 2. Broker: propagate ingress. 3. README #148

Merged
merged 8 commits into from
Jul 31, 2023
Merged

FlowForge helm: 1. Editors: service account. 2. Broker: propagate ingress. 3. README #148

merged 8 commits into from
Jul 31, 2023

Conversation

elenaviter
Copy link
Contributor

Multiple changes in FlowForge helm, all are optional.

Description

FlowForge helm edits:

  1. Editors: optional service account provisioning*.
  2. Broker: propagate ingress definitions to broker helm.
  3. values.yaml: remove secrets from referent values.yaml, adding ref for Editors default service account definition.
  4. README.md: update with IAM section.

*Note that by "default" here meant that all Editor instances will be provisioned with this service account linked.

We have a need and the plan for multiple IAM support, per Project, as a part of Project Configuration with the ability to change IAM binding (likewise the stack can be changed).

Related Issue(s)

FlowForge helm: option to provision default service account for editors. broker: derive the ingress definitions from values.yaml. cluster role-related: resources names should be release-dependent

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on flowforge/helm to update ConfigMap Template
    • Issue/PR raised on flowforge/CloudProject to update values for Staging/Production

Labels

  • Backport needed? -> add the backport label
  • Includes a DB migration? -> add the area:migration label

… broker: propagate ingress definitions to broker helm. 3. remove secrets from referent values.yml, adding ref for service account definition. 4. Update README.md with IAM section
Copy link
Contributor

@hardillb hardillb left a comment

Choose a reason for hiding this comment

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

Thank you for this, I've added some quick notes, but will have to come back and review properly later.

One small thing, the issue says the changes can be applied independently, but that's not possible if they are all in the same PR.

And it would also be useful to raise the issue with the suggestions before doing the work for the PR so we have time to discuss the requirements/changes first to ensure they align with our future plans. If you can update the issue with details about each change that would be useful

helm/flowforge/values.yaml Outdated Show resolved Hide resolved
helm/flowforge/values.yaml Outdated Show resolved Hide resolved
helm/flowforge/values.yaml Show resolved Hide resolved
@elenaviter
Copy link
Contributor Author

Thank you for this, I've added some quick notes, but will have to come back and review properly later.

One small thing, the issue says the changes can be applied independently, but that's not possible if they are all in the same PR.

And it would also be useful to raise the issue with the suggestions before doing the work for the PR so we have time to discuss the requirements/changes first to ensure they align with our future plans. If you can update the issue with details about each change that would be useful

@hardillb Thank you, sorry for making some mess here. You are right about several aspects of this PR, which is clearly wrong and I will take this into account.

Regarding aspects:

  1. Default service account provisioning for Editors:
    Since the Editors are the "dev env, the battlefield for collaboration", and solutions developed often contain integrations with cloud services, the solution host (Editor) might need the web identity to be linked to it. The Editor instances deployments are provisioned with FlowForge core, so that's the only way to link the web identity to deployments is through FlowForge core and it's k8s package. It's important for us to allow the we identity (at least single, as of now) to be mounted to Editor instances deployments, so that Editor instance can leverage this IAM to unlock cloud services and resources usage as far as it is allowed by the role.

    • Just mounting IAM to Editors deployments according to this PR and the follow-up PR opened in @flowforge/kubernetes lib - is not enough.
    • This also requires the environment variables that are mounted by IAM OIDC Identity Provider to the [Editor] pods - to be propagated to Node-RED, along with environment variables that are configured for the project (in its settings). This is the subject of next issue we will open (if it does not exist yet).
  2. Broker: propagate ingress definitions to broker helm:
    Broker already has the support for ingress in it's helm just that it's not filled from values.yaml. Thus the only change is the effective propagation of ingress annotations if there are any defined in values.yaml, to the Broker helm. This allows the Broker to link to cluster balancer.

Please let me know if this makes more sense and thank you!

@hardillb
Copy link
Contributor

hardillb commented Jul 7, 2023

The broker Ingress annotations definitely

The editor AWS IAM stuff I want to have a longer think about, I think it would be best to try and build a generic way to add kubernetes annotations that can be applied across the board so we don't end up adding values for each specific task and also don't add things that end up being AWS specific if possible so we can support all the different Cloud environment equally.

I will try and come up with something over the weekend.

@elenaviter
Copy link
Contributor Author

Just wanted to apologize for the misunderstanding which I might introduce with "IAM".
By that I mean generic "Identity and Access Management services" area which apply to every cloud vendor.

Regarding the ServiceAccount - since it's k8s-level resource, it can be used with the different annotations, depending on the vendor, but still serve the purpose of identity binding.

  • For AWS it's going to be

    eks.amazonaws.com/role-arn: arn:aws:iam::${ACCOUNT_ID}:role/${ROLE_NAME}
    
  • For Azure there are multiple ways to solve AAD, among them Azure pod identity and Azure Workload.

    • And what's great about Azure Workload, ServiceAccount is back on stage again.
      • annotations (of the service account) are going to be
        azure.workload.identity/client-id: ********-****-****-****-************
        
      • pod labels are going to be
        azure.workload.identity/use: "true"
        

Still this does not solve the problem of "multiple roles support per project" since it's going to be "identity for any project running on this FlowForge" - but this is the food for thinking! 🙂

@hardillb
Copy link
Contributor

Sorry it's been a while, just looking at this again.

Can you please add the new helm fields to the values.schema.json file as a well

@elenaviter
Copy link
Contributor Author

Hey @hardillb, sorry for delay.
I've just added the following missing pieces to the schema:

  • forge.projectDeploymentTolerations (optional array-based attribute)
  • editors.serviceAccount (optional definition of service account for Projects)

@hardillb
Copy link
Contributor

Will need to patch the broker annotations in #156

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.

None yet

2 participants