Add labels to kube-system and components namespaces #646

ipochi · 2020-06-19T14:24:58Z

This PR adds label lokomotive.kinvolk.io/name: <namespace_name> to Namespace object for kube-system and all components.

This is done so allow NetworkPolicies be written using the namespaceSelector.

UPDATE:
To go along with network policies we also need customized namespace labels such as metrics labels for prometheus operator, this was not possible in the current incarnation.

To mitigate that:

Add new method to the component interface GetNamespacelabels()
Each component can provide a customized set of labels that needs to be added to the namespace, because helm doesn't not deal with namespace creation or update.
Before installing the component, component.GetNamespaceLabels() is called and passed to UpdateNamespaceWithLabels function.

Fixes : #153 #647

ipochi · 2020-06-19T15:00:51Z

Error: rendered manifests contain a resource that already exists. Unable to continue with install:
existing resource conflict: kind: Namespace, namespace: , name: kube-system

knrt10

Nit

cli/cmd/cluster-apply.go

pkg/components/util/install.go

ipochi · 2020-06-26T09:58:11Z

@iaguis @invidian @knrt10 @surajssd Updated the PR

old: allow a generic label lokomotive.kinvolk.io/name:<namespace-name> to the kube-system and component namespace.

To go along with network policies we also need customized namespace labels such as metrics labels for prometheus operator, this was not possible in the current incarnation.

To mitigate that:

Add new method to the component interface GetNamespacelabels()
Each component can provide a customized set of labels that needs to be added to the namespace, because helm doesn't not deal with namespace creation or update.
Before installing the component, component.GetNamespaceLabels() is called and passed to UpdateNamespaceWithLabels function.

invidian · 2020-06-29T05:05:47Z

Each component can provide a customized set of labels that needs to be added to the namespace, because helm doesn't not deal with namespace creation or update.

Some questions about the spec of that:

What if 2 components wants to install into same namespace, or worse, they declare conflicting labels? If we implement it this way, we put the constraint on us, which we must be aware of, as it may lead to bugs.
What if we remove some label from the components? How do we handle updates? I'd like as to avoid adding code, which may in the future require manual intervention from the user, as that gives a feeling of the very poor automation. However, this could be implemented in the future, by maintaining the list of old labels, which should no longer exist. We could also define, that those labels are immutable and when we change them, it requires component re-installation. However, this should also be conscious and documented decision.

Before installing the component, component.GetNamespaceLabels() is called and passed to UpdateNamespaceWithLabels function.

Maybe we could just re-use Metadata(), so we keep single method for obtaining common information from the components and we leave other component methods "actionable" (like Render(), which may fail etc.)?

ipochi · 2020-07-22T14:31:58Z

@iaguis @invidian @knrt10 @surajssd

Completely reworked the PR to accommodate reviews and bugs found during testing.

Includes a PostInstall step for components., with each component doing whatever they need post installation.

A common task for post installation is to update the components release namespace with labels.

Even if we allow helm to take care of components release namespace creation, it only does on the fly creation of the namespace and not from the chart manifest(if namespace manifest is present)
Hence we cannot add Lokomotive specific labels to the chart manifest as helm complains of the resource already exists error

Special case for kube-system namespace as as we want the namespace to be updated post installation.
During components update, Kubernetes doesn't not keep the old labels, annotations that the user might have created, leading to a loss of such information when we update the component.

Before updating the release namespace, I retrieve the existing metadata of namespace and remove the labels specific to Lokomotive i.e labels with prefix lokomotive.kinvolk.io and delete them and merge the existing user metadata with new set of metadata that may/may not be included in the upgraded component.

I believe this same trick can be applied to all manifests in the chart that have Lokmotive specific labels that may/may not be removed in the updated version of the component (out of scope for this PR)

This also fixes Cert-manager Namespace label doesn't get applied when installing the component #647 as cert-manager component can now add its own specific set of labels the release namespace requires.

invidian

Thanks for the PR @ipochi. I left some comments with suggestions of what I think we should improve before we merge, as in my opinion there is way too much duplicated code in this PR, which can be handled in a better way.

pkg/components/metadata.go

pkg/components/aws-ebs-csi-driver/component.go

pkg/k8sutil/create.go

pkg/components/interface.go

cli/cmd/cluster-apply.go

test/components/cert-manager/cert-manager_test.go

ipochi · 2020-08-10T10:09:40Z

Mostly LGTM, I commented on some nits. One thing which I would change still is e2etests to make them shorter and not per-component, as mentioned in previous comments.

Yes, I am testing a new single method. for purposes of not messing it up. I have tests per component as part of the PR for now. Once the CI passes for the new test. I will update the e2e tests commit and remove the per component tests.

invidian

Just some last nits, otherwise LGTM 🎉

pkg/k8sutil/create_test.go

cli/cmd/cluster-apply.go

test/components/kubernetes/namespaces_labels_test.go

invidian

LGTM, except one nit.

test/components/util/util.go

This commit adds a new Namespace struct that holds metadata regarding the Name, Labels and Annotatations specific to the Lokomotive installation or components installation. Signed-off-by: Imran Pochi <imran@kinvolk.io>

Signed-off-by: Imran Pochi <imran@kinvolk.io>

To allow components specify metadata(labels, annotations) for release namespace. Adds the labels that are required to be present on the components release namespace. Updates references where needed due to this change. Signed-off-by: Imran Pochi <imran@kinvolk.io>

Move function `ensureNamespaceExists` to k8sutil package with new name CreateOrUpdateNamespace. This function creates/updates the namespace. Components use this function to create/update the release namespace. This function is also used to update the `kube-system` namespace after cluster installation. Includes another helper function in the `internal` package that merges two given maps. This is done to ensure that existing labels and annotations are retained and the new set of Labels/annotations overrides any existing ones. Signed-off-by: Imran Pochi <imran@kinvolk.io>

This commit adds the namespace label `lokomotive.kinvolk.io/name : <namespace_name>` to the release namespace. Signed-off-by: Imran Pochi <imran@kinvolk.io>

This commit updates the pre-installed namespaces in the cluster to include the labels provided. Signed-off-by: Imran Pochi <imran@kinvolk.io>

invidian

LGTM 🎉

test/components/util/util.go

Signed-off-by: knrt10 <tripathi.kautilya@gmail.com>

Signed-off-by: Imran Pochi <imran@kinvolk.io>

invidian

LGTM

surajssd

ok

pkg/k8sutil/create.go

knrt10

LGMT and great work @ipochi 🎉

ipochi requested review from iaguis, surajssd, invidian and knrt10 June 19, 2020 14:24

ipochi force-pushed the imran/add-labels-to-namespace branch from 81da759 to cd3166c Compare June 19, 2020 14:29

ipochi force-pushed the imran/add-labels-to-namespace branch 2 times, most recently from 373ab03 to c110753 Compare June 22, 2020 08:56

knrt10 reviewed Jun 22, 2020

View reviewed changes

cli/cmd/cluster-apply.go Outdated Show resolved Hide resolved

ipochi force-pushed the imran/add-labels-to-namespace branch 2 times, most recently from 5df427d to 8085908 Compare June 22, 2020 12:38

invidian reviewed Jun 22, 2020

View reviewed changes

cli/cmd/cluster-apply.go Outdated Show resolved Hide resolved

ipochi force-pushed the imran/add-labels-to-namespace branch from 8085908 to e1be658 Compare June 23, 2020 07:10

surajssd reviewed Jun 25, 2020

View reviewed changes

pkg/components/util/install.go Outdated Show resolved Hide resolved

ipochi force-pushed the imran/add-labels-to-namespace branch from e1be658 to 666ca3e Compare June 26, 2020 09:53

ipochi mentioned this pull request Jun 30, 2020

pkg/k8sutil: change NewClientset to load kubeconfig from file content, not from path #628

Merged

ipochi force-pushed the imran/add-labels-to-namespace branch 4 times, most recently from 8cbd52a to b5c3176 Compare July 22, 2020 14:19

ipochi force-pushed the imran/add-labels-to-namespace branch 3 times, most recently from 27cc2b7 to f63f5cb Compare July 23, 2020 06:24

ipochi requested review from surajssd and invidian July 23, 2020 07:42

ipochi mentioned this pull request Jul 23, 2020

components: Add istio-operator #686

Merged

6 tasks

invidian suggested changes Jul 23, 2020

View reviewed changes

ipochi force-pushed the imran/add-labels-to-namespace branch from a8bf9f4 to 6332379 Compare August 10, 2020 11:36

ipochi requested a review from invidian August 10, 2020 11:38

ipochi force-pushed the imran/add-labels-to-namespace branch from 6332379 to 3ed0efc Compare August 10, 2020 11:39

invidian suggested changes Aug 10, 2020

View reviewed changes

ipochi force-pushed the imran/add-labels-to-namespace branch 2 times, most recently from b7ae4d2 to d447aa3 Compare August 10, 2020 14:40

ipochi requested review from surajssd and invidian August 10, 2020 14:41

invidian suggested changes Aug 10, 2020

View reviewed changes

test/components/util/util.go Show resolved Hide resolved

ipochi added 6 commits August 11, 2020 13:11

k8sutil: Namespace struct for storing namespace metadata

f288aac

This commit adds a new Namespace struct that holds metadata regarding the Name, Labels and Annotatations specific to the Lokomotive installation or components installation. Signed-off-by: Imran Pochi <imran@kinvolk.io>

internal: add function to append name label to a given map

46ce909

Signed-off-by: Imran Pochi <imran@kinvolk.io>

components: add namespace label to release namespace.

ea36299

This commit adds the namespace label `lokomotive.kinvolk.io/name : <namespace_name>` to the release namespace. Signed-off-by: Imran Pochi <imran@kinvolk.io>

cluster: update pre installed namespaces with labels

667571d

This commit updates the pre-installed namespaces in the cluster to include the labels provided. Signed-off-by: Imran Pochi <imran@kinvolk.io>

ipochi force-pushed the imran/add-labels-to-namespace branch 2 times, most recently from ced9a7a to 8e4a55f Compare August 11, 2020 07:42

invidian previously approved these changes Aug 11, 2020

View reviewed changes

test/components/util/util.go Outdated Show resolved Hide resolved

knrt10 and others added 3 commits August 11, 2020 13:39

Add functionality to test namespace

b512bae

Signed-off-by: knrt10 <tripathi.kautilya@gmail.com>

test: use a consistent way to create test namespaces

e187820

Signed-off-by: Imran Pochi <imran@kinvolk.io>

e2etest: Add e2e test to check if labels were added correctly

a51b143

Signed-off-by: Imran Pochi <imran@kinvolk.io>

ipochi dismissed invidian’s stale review via a51b143 August 11, 2020 08:11

ipochi force-pushed the imran/add-labels-to-namespace branch from 8e4a55f to a51b143 Compare August 11, 2020 08:11

invidian approved these changes Aug 11, 2020

View reviewed changes

surajssd approved these changes Aug 11, 2020

View reviewed changes

knrt10 reviewed Aug 11, 2020

View reviewed changes

pkg/k8sutil/create.go Show resolved Hide resolved

knrt10 approved these changes Aug 11, 2020

View reviewed changes

ipochi merged commit aed5228 into master Aug 11, 2020

ipochi deleted the imran/add-labels-to-namespace branch August 11, 2020 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add labels to kube-system and components namespaces #646

Add labels to kube-system and components namespaces #646

ipochi commented Jun 19, 2020 •

edited

Loading

ipochi commented Jun 19, 2020 •

edited

Loading

knrt10 left a comment

ipochi commented Jun 26, 2020 •

edited

Loading

invidian commented Jun 29, 2020

ipochi commented Jul 22, 2020 •

edited

Loading

invidian left a comment

ipochi commented Aug 10, 2020

invidian left a comment

invidian left a comment

invidian left a comment

invidian left a comment

surajssd left a comment

knrt10 left a comment

Add labels to kube-system and components namespaces #646

Add labels to kube-system and components namespaces #646

Conversation

ipochi commented Jun 19, 2020 • edited Loading

ipochi commented Jun 19, 2020 • edited Loading

knrt10 left a comment

Choose a reason for hiding this comment

ipochi commented Jun 26, 2020 • edited Loading

invidian commented Jun 29, 2020

ipochi commented Jul 22, 2020 • edited Loading

invidian left a comment

Choose a reason for hiding this comment

ipochi commented Aug 10, 2020

invidian left a comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

surajssd left a comment

Choose a reason for hiding this comment

knrt10 left a comment

Choose a reason for hiding this comment

ipochi commented Jun 19, 2020 •

edited

Loading

ipochi commented Jun 19, 2020 •

edited

Loading

ipochi commented Jun 26, 2020 •

edited

Loading

ipochi commented Jul 22, 2020 •

edited

Loading